Skip to content

[Validation] v8 blocks, Sapling merkle tree inclusion. #1910

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
Merged
Fuzzbawls merged 7 commits into from
Oct 23, 2020
Merged

[Validation] v8 blocks, Sapling merkle tree inclusion. #1910

Fuzzbawls merged 7 commits into from
Oct 23, 2020

Conversation

@furszy
Copy link

@furszy furszy commented Oct 10, 2020 •
edited
Loading

Another decoupling from #1798, built on top of #1903 and #1915.

Containing three topics:

  1. Sapling merkle tree root hash inclusion in the block header.

    • Block version bumped to v8.
    • Miner crafting the new merkle tree root hash and setting it into the block.

  2. Track network total value entering and exiting the shielded pool. (*)

    • Similar to the previous, initial, zc failsafe flow. (this can be done much better, like we did with zerocoin supply)
    • Block negative shielded value pool balances. -- Network validation consensus rule --

  3. ChainTip signal implemented.

    • Signal to notify the listeners (wallet) about new blocks (or chain reorgs), broadcasting the new block and the current merkle tree so the listener can build upon it. -- Not connected to anything yet --

This PR is including the following commits decoupled from #1798.

  • Track net value entering and exiting the Sapling circuit.—> 8dd2045034b146d68eb6ef592433348852d54edd
  • Block: header hashFinalSaplingRoot inclusion.—> 5f55a76bd303e21ed31e1947eb12d44a999f667f
  • Sapling: missing nSaplingValue in LoadBlockIndexGuts added.—> fbe07613cf296ccc3f0a1b55f0015e86e2ff3d39
  • Miner: sapling tree hash connection.—> cb88fae28eaa760c7feb00ab5ccd6f1cb8e6db15
  • Implement ChainTip signal. —> 4e96e6c97d258c87910d2e9ab13cbb79c25ebda5
  • Signal ChainTip in ConnectTip and DisconnectTip methods —> 85f4435b4076c8d4c8e222a0e0d7b8f76465be63
  • Pass chainparams as argument in DisconnectTip, InvalidateBlock, ReprocessBlocks, DisconnectBlocks, DisconnectTip —> 1c809536b862e59554dc9ebee8651c56935a0f9a
  • Do not try to update tree anchor and witnesses if sapling is not active --> 1c64497d4f020d6c400c7aa836f75dce7d1517d4
  • Validation: Adapted the cached incremental witnesses update flow to the syncTransaction changes. —> 6e6372df9270dba2f76836d5b964c2153e28bda6

@furszy furszy self-assigned this Oct 10, 2020
@furszy furszy force-pushed the 2020_sapling_v8_blocks branch 3 times, most recently from 238a391 to 9faa809 Compare October 13, 2020 23:16
@furszy
Copy link
Author

furszy commented Oct 14, 2020

rebased this on top of #1915, making it depend on it.
The reason is that #1915 makes ChainTip signal flow much performant with cleaner code.

@furszy furszy force-pushed the 2020_sapling_v8_blocks branch from 9faa809 to 06f7f1d Compare October 17, 2020 18:34
Fuzzbawls
Fuzzbawls reviewed Oct 20, 2020
View reviewed changes
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

Some consistency comments left about using our optional.h wrapper around boost::optional, but otherwise looking good

@furszy furszy force-pushed the 2020_sapling_v8_blocks branch from 06f7f1d to d0b477e Compare October 20, 2020 06:13
random-zebra
random-zebra suggested changes Oct 20, 2020
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

There's an issue with the new RPC fields implementation (UniValue object elements must be a pair key-object, not plain objects).

Aside from this, we should consider if we really want to store in memory nChainSaplingValue for all blocks. That would add more than 20 MB to the runtime memory consumption at the current chain size (on mainnet).
Not so much... but still, we only need its value at the chain tip.
Probably fine to leave it as is in this PR, but something we might want to consider later.

@random-zebra random-zebra added Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes Sapling Upstream ZCash Validation labels Oct 20, 2020
@random-zebra random-zebra added this to the 5.0.0 milestone Oct 20, 2020
@furszy furszy force-pushed the 2020_sapling_v8_blocks branch from d0b477e to 883e679 Compare October 20, 2020 20:03
@furszy
Copy link
Author

furszy commented Oct 20, 2020

Updated per feedback.

@furszy furszy force-pushed the 2020_sapling_v8_blocks branch from 883e679 to 844baef Compare October 20, 2020 20:32
@furszy furszy mentioned this pull request Oct 21, 2020
Merged
@furszy furszy requested review from Fuzzbawls and removed request for Fuzzbawls October 22, 2020 02:01
random-zebra
random-zebra previously approved these changes Oct 22, 2020
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Just a minor nit for the help output.
Otherwise ACK.

furszy added 7 commits October 22, 2020 11:36
@furszy
Track net value entering and exiting the Sapling circuit.
e56982b 
Back port coming from zcash@ae97177c86a902d8f9b2976d9b578b5d236f1ce5
@furszy
Block: header hashFinalSaplingRoot inclusion.
e43380d
@furszy
Miner: sapling tree hash connection.
79dc604
@furszy
Pass chainparams as argument in DisconnectTip, InvalidateBlock, Repro…
c605d98 
…cessBlocks, DisconnectBlocks, DisconnectTip
@furszy
Implement ChainTip signal.
df71481
@furszy
Signal ChainTip in ConnectTip and DisconnectTip methods
2b6f96b 
Do not try to update tree anchor and witnesses if sapling is not active

Validation: Adapted the cached incremental witnesses update flow to the syncTransaction changes.

The witness update must be after the SyncTransaction signal.
@furszy
Add ZIP209 turnstile violation check.
1403918 
Reject blocks with negative shielded pool value.
@furszy furszy force-pushed the 2020_sapling_v8_blocks branch from 844baef to 1403918 Compare October 22, 2020 14:37
@furszy
Copy link
Author

furszy commented Oct 22, 2020

Done, updated per feedback.

random-zebra
random-zebra approved these changes Oct 22, 2020
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

utACK 1403918

Fuzzbawls
Fuzzbawls approved these changes Oct 23, 2020
View reviewed changes
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

utACK 1403918

@Fuzzbawls Fuzzbawls merged commit d28fef1 into PIVX-Project:master Oct 23, 2020
@random-zebra random-zebra removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Jan 3, 2021
@furszy furszy deleted the 2020_sapling_v8_blocks branch November 29, 2022 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fuzzbawls Fuzzbawls Fuzzbawls approved these changes

+1 more reviewer

@random-zebra random-zebra random-zebra approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@furszy furszy

Labels

Sapling Upstream ZCash Validation

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@furszy @Fuzzbawls @random-zebra