Skip to content

fix(utxo-lib): update zcash NU5_BRANCH_ID#2259

Merged
OttoAllmendinger merged 5 commits into
masterfrom
BG-47967.update-zcash
May 9, 2022
Merged

fix(utxo-lib): update zcash NU5_BRANCH_ID#2259
OttoAllmendinger merged 5 commits into
masterfrom
BG-47967.update-zcash

Conversation

@OttoAllmendinger

Copy link
Copy Markdown
Contributor

See zcash/zcash@77a971f

Use v4.7.0 as zcash reference node.

Issue: BG-47967 BREAKING CHANGE: signature validity of certain zcash
transaction is changed

6a161f2
chore(utxo-lib): add prettierignore, apply prettier

Issue: BG-47967

@OttoAllmendinger OttoAllmendinger force-pushed the BG-47967.update-zcash branch 2 times, most recently from c0903ad to 37bfff3 Compare May 9, 2022 11:24
@OttoAllmendinger OttoAllmendinger changed the title fix(utxo-lib)!: update zcash NU5_BRANCH_ID fix(utxo-lib): update zcash NU5_BRANCH_ID May 9, 2022
@OttoAllmendinger OttoAllmendinger force-pushed the BG-47967.update-zcash branch 2 times, most recently from 195cde9 to d7f735a Compare May 9, 2022 13:07
@codecov

codecov Bot commented May 9, 2022

Copy link
Copy Markdown

Codecov Report

Merging #2259 (a9b666b) into master (f7b96a0) will increase coverage by 52.61%.
The diff coverage is n/a.

❗ Current head a9b666b differs from pull request most recent head 68c615f. Consider uploading reports for the commit 68c615f to get more accurate results

@@             Coverage Diff             @@
##           master    #2259       +/-   ##
===========================================
+ Coverage        0   52.61%   +52.61%     
===========================================
  Files           0        6        +6     
  Lines           0      574      +574     
  Branches        0       94       +94     
===========================================
+ Hits            0      302      +302     
- Misses          0      262      +262     
- Partials        0       10       +10     
Flag Coverage Δ
unit 52.61% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
modules/express/src/config.ts 94.73% <0.00%> (ø)
modules/express/src/expressApp.ts 89.51% <0.00%> (ø)
modules/express/src/clientRoutes.ts 32.48% <0.00%> (ø)
modules/express/src/args.ts 100.00% <0.00%> (ø)
modules/express/src/retryPromise.ts 11.76% <0.00%> (ø)
modules/express/src/errors.ts 82.35% <0.00%> (ø)

@OttoAllmendinger OttoAllmendinger marked this pull request as ready for review May 9, 2022 13:21
@OttoAllmendinger OttoAllmendinger requested a review from a team as a code owner May 9, 2022 13:21
@OttoAllmendinger OttoAllmendinger requested review from a team and tylerlevine May 9, 2022 13:21
brandonblack
brandonblack previously approved these changes May 9, 2022

@brandonblack brandonblack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code changes look good to me, but I admit I don't fully understand the sequence of changes needed to stay in sync with ZCash.

See zcash/zcash@77a971f

Use v4.7.0 as zcash reference node.

BREAKING CHANGE:
signature validity of certain zcash transaction is changed

Issue: BG-47967
Unsurprisingly we have a lot of circular dependencies, which means
that certain constructors are not available at module init time
(On my system it was Avax, Sol and some others).

This change defers the initialization of the "constructor map" to the
time it is actually needed (the first `getInstance()` call)

Issue: BG-47967
Reflects consensusBranchId changes

Issue: BG-47967
const OVERWINTER_BRANCH_ID = 0x5ba81b19;
const CANOPY_BRANCH_ID = 0xe9ff75a6;
const NU5_BRANCH_ID = 0x37519621;
const NU5_BRANCH_ID = 0xc2d6d0b4;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So this changed between releases? Kind of weird to change constants like this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

agreed

@str4d str4d Jun 20, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The reason this constant changed is documented in ZIP 200:

The relationship between CONSENSUS_BRANCH_ID and ACTIVATION_HEIGHT is many-to-one: it is possible for many distinct consensus branches to descend from the same parent block (and thus have the same ACTIVATION_HEIGHT), but a specific consensus branch can only have one parent block. Concretely, this means that if the ACTIVATION_HEIGHT of a network upgrade is changed for any reason (e.g. security vulnerabilities or consensus bugs are discovered), the CONSENSUS_BRANCH_ID MUST also be changed.

The consensus rules were altered between 4.5.1 and 4.7.0, which required changing the activation height of NU5 on testnet, which therefore required changing the consensus branch ID. More generally, a consensus branch ID refers to a specific set of consensus rules; any change to that set requires a change to the consensus branch ID.

Comment thread modules/utxo-lib/test/integration_local_rpc/generate/fixtures.ts
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.

5 participants