v5 transaction format #5202
v5 transaction format #5202
Conversation
str4d
added
the
C-upstream-port
|
Closing so I can reopen this targeting the correct branch. |
|
Replaced by #5211. |
|
v4.1.1 is released, so we can merge into master again. Hopefully Homu doesn't mind we closed this... |
The majority of the parser is in C++, but Orchard bundles are parsed exclusively by Rust. The ZIP 244 test vectors are brought in here so we can start by testing round-trip serialization.
This also fixes a bug in `CTransaction::SerializationOp` where `CTransaction::UpdateHash` was not being called for v5 transactions.
|
@zkbot try |
|
⌛ Trying commit 36874e7 with merge 6fb94b516af85b2a5483f94e8168d99e40b45974... |
| if (ser_action.ForRead()) { | ||
| SaplingBundle saplingBundle; | ||
| READWRITE(saplingBundle); | ||
| *const_cast<std::vector<SpendDescription>*>(&vShieldedSpend) = |
There was a problem hiding this comment.
This, and the other casts like it, are undefined behaviour. I know it was undefined behaviour before this PR, but every time we change this code, we have a new chance for it to be miscompiled. It should have been fixed long ago (#967). For anyone who isn't familiar with why it's undefined behaviour and why the const declarations don't help in the first place, see #967 (comment) .
There was a problem hiding this comment.
I don't think we can fix this in this PR. However, given that for Orchard we are parsing everything in Rust and then using calls to Rust to inspect Orchard data, we require accessors for those, and so we can make a start towards fixing this by doing the same for all new variables going forward.
I will move nConsensusBranchId and orchardBundle to be non-const private member variables.
|
☀️ Test successful - pr-try |
Using `const_cast` to serialize into an otherwise-constant field is undefined behaviour: zcash#967 (comment) Instead, we should make CTransaction's members non-const and private, and provide accessors. It's not practical to make this change everywhere yet, but we can start by only introducing new fields in this way. We will need to provide accessors for orchardBundle's properties in any case, since we need to call across the Rust FFI.
str4d
added
A-rust-ffi
|
@zkbot r+ |
|
📌 Commit 1ef8181 has been approved by |
Include Orchard bundle in transaction dynamic usage This was missed in #5202.
Includes a new wrapper that enables passing C++ streams across to Rust.
Closes #5022.