[Sapling] Anchors and nullifiers integrated into the chain state view layered cache. #1903
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
furszy
force-pushed
the
2020_sapling_anchorAndNullifier_view_integration
branch
from
1337393
to
7a5b812
Compare
furszy
force-pushed
the
2020_sapling_anchorAndNullifier_view_integration
branch
from
08f0e8e
to
6d6da7f
Compare
|
Rebased on top of master, conflicts solved. Ready for review |
There was a problem hiding this comment.
Pretty good. Code ACK with few notes.
Just an idea for the future:
In the tests here we usually do:
CCoinsViewTest base;
CCoinsViewCache cache1(&base);and then operate on the CCoinsViewCache object cache1.
It would be good, instead, to layer CCoinsViewCacheTest objects on top of the base CCoinsViewTest, and check also the dynamic memory usage with anchors and nullifiers.
src/coins.cpp
Outdated
Comment on lines
354
to
357
| // todo: Move field to fees.h when the class gets back ported (this is needed for the mempool unit tests) | ||
| static const double MAX_PRIORITY = 1e16; |
There was a problem hiding this comment.
The "max" priority is actually INF_PRIORITY (1e25).
Though, we have to consider that, if high-priority txes fill a block up to nBlockPrioritySize (which is customizable by the staker), then the remaining txes are ordered by fee.
[Sapling] CCoinsViewCache add missing SetNullifiers method
coins_tests: remove not needed BOOST_TEST_CONTEXT. We only have Sapling tests.
furszy
force-pushed
the
2020_sapling_anchorAndNullifier_view_integration
branch
from
6d6da7f
to
ee8ed26
Compare
furszy
force-pushed
the
2020_sapling_anchorAndNullifier_view_integration
branch
from
ee8ed26
to
3b4ad0d
Compare
|
updated per feedback. |
Fuzzbawls
approved these changes
Oct 13, 2020
Fuzzbawls
added a commit
that referenced
this pull request
Oct 23, 2020
1403918 Add ZIP209 turnstile violation check. (furszy) 2b6f96b Signal ChainTip in ConnectTip and DisconnectTip methods (furszy) df71481 Implement ChainTip signal. (furszy) c605d98 Pass chainparams as argument in DisconnectTip, InvalidateBlock, ReprocessBlocks, DisconnectBlocks, DisconnectTip (furszy) 79dc604 Miner: sapling tree hash connection. (furszy) e43380d Block: header hashFinalSaplingRoot inclusion. (furszy) e56982b Track net value entering and exiting the Sapling circuit. (furszy) Pull request description: 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.—> 8dd2045 * Block: header hashFinalSaplingRoot inclusion.—> 5f55a76 * Sapling: missing nSaplingValue in LoadBlockIndexGuts added.—> fbe0761 * Miner: sapling tree hash connection.—> cb88fae * Implement ChainTip signal. —> 4e96e6c * Signal ChainTip in ConnectTip and DisconnectTip methods —> 85f4435 * Pass chainparams as argument in DisconnectTip, InvalidateBlock, ReprocessBlocks, DisconnectBlocks, DisconnectTip —> 1c80953 * Do not try to update tree anchor and witnesses if sapling is not active --> 1c64497 * Validation: Adapted the cached incremental witnesses update flow to the syncTransaction changes. —> 6e6372d ACKs for top commit: random-zebra: utACK 1403918 Fuzzbawls: utACK 1403918 Tree-SHA512: c90ef1f358fb7a6e5a87b7b39985d65859048bb023dc8fd66020ee1c3e6ab4635ff595262beeaee93b12f4d83e21fe0613eb490b4a63d7bc96c61e4726ac1720
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One more PR decoupled from #1798 to make the review process faster.
Essentially, the chain view state will be storing and caching the best anchor hash and 2 new entry types: (1) Anchors, (2) Nullifiers (Each of them with their entry class and map).
Note: This PR is introducing the new capabilities and validating the functionality correct behavior with a good amount of unit tests (test coverage inside
coins_tests.cpp), it's not connected to the network flow.Note2: There is a functional tests in 1798 (
sapling_wallet_anchorfork.py) that wasn't able to decouple here due larger features requirements (new RPC calls introduced down 1798 commits line) but can be easily run there.List of commits gathered here from #1798: