-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add transaction version 5 #1824
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good start, but needs some fixes.
Here's what I checked:
- version group id
- other consensus rules
- serialization and deserialization
- all instances of
V4
in the codebase, to make sure we added a matchingV5
Here's what I suggested:
- the specific change and ZIP for each
unimplemented!()
- add
unimplemented!()
instead of re-using v1-v3 code
Here's what I committed to your branch:
- update comment for V5 transactions 0ac10e2
- add V5 transactions to non_finalized_state 5269c2e
- add ZIPs to unimplemented messages c1af4a6
Here's what we still need to do:
- Make the version group id match the draft spec (typo)
- Add
zcash_deserialize
implementation for version5
- Make
Transaction::sprout_nullifiers
explicitly return empty forV1
andV5
- Add a
V5
case toTransaction::sapling_nullifiers
- Double-check all instances of
V4
in the codebase
Here are the follow-up tasks we should do:
- Use
v5_strategy
inArbitrary for Transaction
Usev5_strategy
inArbitrary for Transaction
#1826 - Work out how to create
V5
transactions before theNU5
activation height is set Usev5_strategy
inArbitrary for Transaction
#1826 - Split
WrongVersion
errors intoOutdatedVersion
andPreActivationVersion
SplitWrongVersion
errors intoOutdatedVersion
andPreActivationVersion
#1827 - Use
PreActivationVersion
forV5
ifNU5
hasn't activated yet SplitWrongVersion
errors intoOutdatedVersion
andPreActivationVersion
#1827 - Parse Sapling data in transaction version 5 Parse Sapling data in Transaction Version 5 #1829
Here is a low-priority optional follow-up:
- Move some code into methods on
Transaction
Move duplicateTransaction
code into methods #1828
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check any copied code against the draft NU5/Orchard specs.
There are a lot of small changes to field orders, constants, and other protocol features.
if id != TX_V5_VERSION_GROUP_ID { | ||
return Err(SerializationError::Parse("expected TX_V5_VERSION_GROUP_ID")); | ||
} | ||
let inputs = Vec::zcash_deserialize(&mut reader)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, the order of these fields should match the draft v5 transaction spec.
Currently these are all `unimplemented!(...)`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, we just need to:
- update the field order in v5_strategy, to avoid a nightly clippy lint
- double-check all instances of
V4
in the codebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @oxarbitrage we're almost there.
I made the following changes:
- We're still deciding if
V5
transactions supportSprout
, so I changed all theV5
sprout match arms tounimplemented!()
- This part of the spec is still changing
- I found another match without a
V5
arm, looks like we both missed that one
I just need you to:
- Check the changes I just pushed
- Double-check all instances of
V4
in the codebase, to make sure they have a matchingV5
- Unfortunately, IP addresses also have a
V4
variant - we don't need to fix those
- Unfortunately, IP addresses also have a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last changes are all ok. Double checked V4
/V5
instances, they are all good after 48fb77e
Motivation
Start the work in the next network upgrade transaction.
Solution
Add the V5 transaction to the enum and add some common fields that we know are going to be there. The "rest" of the transaction is represented in as
Vec<u8>
in arest
field.The prop tests are blocked on #1823
The code in this pull request has:
Review
Related Issues
Closes #1822
Closes #1826
Follow Up Work