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.

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

zcash_protocol: Set the ZFuture consensus branch ID to agree with zcashd #1480

Closed
wants to merge 1 commit into from

Conversation

nuttycom
Copy link
Contributor

@nuttycom nuttycom commented Aug 1, 2024

No description provided.

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.82%. Comparing base (7f7b685) to head (c6cf5e5).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1480   +/-   ##
=======================================
  Coverage   60.82%   60.82%           
=======================================
  Files         140      140           
  Lines       16472    16472           
=======================================
  Hits        10019    10019           
  Misses       6453     6453           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -593,7 +593,7 @@ impl From<BranchId> for u32 {
#[cfg(zcash_unstable = "nu6")]
BranchId::Nu6 => 0xc8e7_1055,
#[cfg(zcash_unstable = "zfuture")]
BranchId::ZFuture => 0xffff_ffff,
BranchId::ZFuture => 0x7fff_ffff,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the git record:

So for some reason, both consensus branch IDs came out of the same TZE work but ended up differing by a single bit.

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK. This has no effect on consensus for current mainnet and testnet, where "zfuture" must not be enabled.

@nuttycom
Copy link
Contributor Author

nuttycom commented Aug 6, 2024

This is completely wrong; there was a confusion between nProtocolVersion for ZFuture and the consensus branch ID.

@nuttycom nuttycom closed this Aug 6, 2024
@nuttycom nuttycom deleted the zfuture_consensus_id branch August 6, 2024 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants