Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Parse ConsensusBranchId into NetworkUpgrade for transaction v5 #2075

Merged
merged 13 commits into from
Apr 29, 2021
Merged

Parse ConsensusBranchId into NetworkUpgrade for transaction v5 #2075

merged 13 commits into from
Apr 29, 2021

Conversation

oxarbitrage
Copy link
Contributor

Motivation

nConsensusBranchId was added to the spec for transaction V5: https://github.com/zcash/zips/blob/master/zip-0225.rst#specification
We need to add this to Zebra.

Solution

In this PR we are adding the field, the serialization and the arbitrary tests but we are not doing validation.

The code in this pull request has:

  • Documentation Comments
  • Property Tests

Review

@teor2345

Related Issues

This is part of #2066

Follow Up Work

Validation should be done to complete #2066 but we need #2070 merged first.

A conflict is expected after #2070 just because the transaction_to_fake_v5 was moved.

@teor2345 teor2345 added the S-blocked Status: Blocked on other tasks label Apr 27, 2021
teor2345
teor2345 suggested changes Apr 27, 2021
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge #2070 and all the PRs that depend on it, before we merge this PR, to avoid conflicts

@teor2345 teor2345 marked this pull request as draft April 27, 2021 20:07
@oxarbitrage
Copy link
Contributor Author

Let's merge #2070 and all the PRs that depend on it, before we merge this PR, to avoid conflicts

Yea, i will rebase after that.

@oxarbitrage oxarbitrage changed the title Add consensus_branch_id field to transaction v5 Add network_upgrade field to transaction v5 Apr 27, 2021
@teor2345 teor2345 added A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement NU-5 Network Upgrade: NU5 specific tasks P-Medium and removed S-blocked Status: Blocked on other tasks labels Apr 28, 2021
@teor2345 teor2345 added this to the 2021 Sprint 8 milestone Apr 28, 2021
@teor2345 teor2345 changed the title Add network_upgrade field to transaction v5 Parse ConsensusBranchId into NetworkUpgrade for transaction v5 Apr 28, 2021
@teor2345 teor2345 marked this pull request as ready for review April 28, 2021 00:47
@teor2345 teor2345 mentioned this pull request Apr 28, 2021
Merged
teor2345
teor2345 suggested changes Apr 28, 2021
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

There are a few places where we could use standard library functions to simplify the code, so we don't need to write tests.

zebra-chain/src/parameters/network_upgrade.rs Outdated Show resolved Hide resolved
zebra-chain/src/parameters/network_upgrade.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction/arbitrary.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction/serialize.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction/tests/vectors.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction/serialize.rs Outdated Show resolved Hide resolved
@oxarbitrage
Copy link
Contributor Author

I made all the changes except for the derive of Arbitrary for NetworkUpgrade where i added a doc comment.

Will rebase after the PR is re-reviewed by @teor2345

teor2345
teor2345 suggested changes Apr 28, 2021
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use a custom strategy to return arbitrary network upgrades with branch IDs.

teor2345
teor2345 reviewed Apr 29, 2021
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, I added a missing TODO comment.

I've set it to auto-merge after CI passes.

teor2345
teor2345 previously approved these changes Apr 29, 2021
View reviewed changes
@teor2345
Copy link
Contributor

I merged the imports and moved code in the test module

@teor2345 teor2345 enabled auto-merge (squash) April 29, 2021 01:32
@oxarbitrage
Copy link
Contributor Author

I think something is wrong. the utility functions and the new constant should be in https://github.com/ZcashFoundation/zebra/blob/main/zebra-chain/src/transaction/arbitrary.rs#L327

@teor2345 teor2345 merged commit 9fc2388 into ZcashFoundation:main Apr 29, 2021
@teor2345
Copy link
Contributor

I think something is wrong. the utility functions and the new constant should be in https://github.com/ZcashFoundation/zebra/blob/main/zebra-chain/src/transaction/arbitrary.rs#L327

You're right, it looks like I did the merge wrong, and then auto-merge passed and merged the PR, even though it should have failed.

I'll open a PR to fix it up, and a ticket to deal with the GitHub issue

@teor2345 teor2345 mentioned this pull request Apr 29, 2021
Merged
teor2345 added a commit to teor2345/zebra that referenced this pull request Apr 29, 2021
@teor2345
Fix a bad merge in ZcashFoundation#2075
a66ed69 
Also tweak a constant name, an import, and a comment.
teor2345 added a commit that referenced this pull request Apr 29, 2021
@teor2345
Fix a bad merge in #2075 (#2085)
b52fbae 
Also tweak a constant name, an import, and a comment.
@mpguerra mpguerra mentioned this pull request Apr 30, 2021
Closed
2 tasks
@oxarbitrage oxarbitrage mentioned this pull request May 3, 2021
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 approved these changes

Assignees

@oxarbitrage oxarbitrage

Labels
A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Milestone
2021 Sprint 8
Development

Successfully merging this pull request may close these issues.
2 participants
@oxarbitrage @teor2345