pczt: Define the structure and semantics of the PCZT format #1577
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1577 +/- ##
==========================================
- Coverage 56.38% 54.58% -1.80%
==========================================
Files 149 168 +19
Lines 19476 20371 +895
==========================================
+ Hits 10981 11119 +138
- Misses 8495 9252 +757 ☔ View full report in Codecov by Sentry. |
|
For ZSA issuance I think we need the fields required for computing the |
|
Force-pushed to fill in some more parts:
|
|
I'm currently inclined to leave out both FROST support and ZSA issuance support from the first version of PCZTs, as we don't have experience with either of them yet, and trying to get them right at the same time as the base PCZT format is likely to end in sorrow. As @conradoplg notes, we can in some cases have FROST internally pass around a PCZT, but that won't necessarily help for all possible PCZT use cases, so we'll likely want a v2 that adds those fields. That would also be the right time to adjust the core fields if we find weaknesses in the v1 PCZT format. |
|
I'm happy with leaving FROST out of v1, we can make it work for now by sending additional data along with the PCZT. |
|
Force-pushed with more changes:
|
Use of the Constructor role interacts heavily with the per-protocol crates, so I'm now working on the production impl of those, both to fix the format bugs and get us to a working client-side implementation. |
I have a couple more changes to make to the |
| transparent = ["dep:secp256k1", "dep:zcash_primitives", "dep:zcash_protocol"] | ||
| zcp-builder = ["dep:zcash_primitives"] | ||
| io-finalizer = ["orchard", "sapling"] | ||
| prover = ["dep:rand_core", "sapling?/temporary-zcashd"] |
There was a problem hiding this comment.
Is the sapling/temporary-zcashd feature dependency strictly necessary? Consider filing an issue (if there isn't one already) against sapling to make the necessary APIs stable.
There was a problem hiding this comment.
This was previously necessary due to a use of SpendValidatingKey::temporary_zcash_from_bytes, but that appears to have been removed in a recent push, and this branch now compiles for me without the temporary-zcashd feature dependency (using --all-targets --all-features).
| prover = ["dep:rand_core", "sapling?/temporary-zcashd"] | |
| prover = ["dep:rand_core"] |
There was a problem hiding this comment.
After having read BIP 370 thoroughly, I just don't understand the design of this API. BIP 370 says, for the Constructor:
Before any input or output may be added, the constructor must check the
PSBT_GLOBAL_TX_MODIFIABLEfield. Inputs may only be added if the Inputs Modifiable flag is True. Outputs may only be added if the Outputs Modifiable flag is True.
That is not enforced by the API. If it's supposed to be a client of this API that is responsible for enforcing it, then it doesn't have enough information to do so in general for the adaptation of the above rule to Zcash. The adapted rule would require that we not make any changes to effecting data outside the transparent inputs and outputs after signing any input. That isn't captured by the flags as they stand, and so a client would have to track that independently of the flags.
I can accept that we have little time to make any major design changes at this point. But please document the limitations (and maybe fix the most obvious loopholes that are easy to fix without a major design change).
We don't provide an independent Constructor implementation, and thus have nowhere that needs to enforce this requirement. (If we did, we'd indeed need to enforce this requirement in that API.) What we provide in our implemented API is |
- The Inputs Modifiable Flag and Outputs Modifiable Flag are now only for transparent inputs and outputs. - Bit 7 is now the Shielded Modifiable Flag (a single flag bit is fine because all shielded effects are always committed to at once). - Fixed a bug in `Creator::build_from_parts` where the Has `SIGHASH_SINGLE` Flag was not being set correctly. - The Signer role now updates `tx_modifiable` in case the IO Finalizer role has not yet run (which might be possible in some scenarios).
| if ((self.tx_modifiable & !FLAG_SHIELDED_MODIFIABLE) >> 3) != 0 | ||
| || ((tx_modifiable & !FLAG_SHIELDED_MODIFIABLE) >> 3) != 0 | ||
| { |
There was a problem hiding this comment.
| if ((self.tx_modifiable & !FLAG_SHIELDED_MODIFIABLE) >> 3) != 0 | |
| || ((tx_modifiable & !FLAG_SHIELDED_MODIFIABLE) >> 3) != 0 | |
| { | |
| if ((self.tx_modifiable & !(FLAG_TRANSPARENT_INPUTS_MODIFIABLE | FLAG_TRANSPARENT_OUTPUTS_MODIFIABLE | FLAG_SHIELDED_MODIFIABLE)) != 0) { |
Non-blocking.
No description provided.