[BUG] Zerocoin: fix spend validation and remove obsolete sporks #2484
Conversation
This file contains hidden or 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
random-zebra
force-pushed
the
202107_fix_zerocoin-connectblock
branch
from
c50eb44 to
810d589
Compare
considering that a tx can contain multiple spends
as this was meant to be checked only if IsBlockChainSynced. As zerocoin public spends are no longer accepted (both in main-net/testnet) we can just remove it.
Zerocoin spends values are already returned by GetValueIn below
SPORK_16 value is still checked in: - AcceptToMemoryPool (but there's a static check at lines 418-421) - ConnectBlock (but there's a height-based check at lines 1562-1564, and an additional height-based check inside CheckBlock)
...calls in ConnectBlock when possible. The check at line 2798 in CheckBlock ensures that no zc tx (either mint or spend) is accepted in the chain after v5 activation. `ContainsZerocoins`/`HasZerocoinSpendInputs` are expensive functions, especially when they return `false` (as they need to go through all inputs and outputs). Therefore avoid the duplicated check in ConnectBlock, and, instead, use the shortcut operator to skip HasZerocoinSpendInputs call after v5 enforcement.
as we already checking it via ContextualCheckBlock --> ContextualCheckTransaction --> ContextualCheckZerocoinTx
random-zebra
force-pushed
the
202107_fix_zerocoin-connectblock
branch
from
810d589 to
f7a93cd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
#2425 introduced two bugs which are preventing a successful chain sync from scratch on
master:It abstracted a piece of code in
ParseAndValidateZerocoinSpend, which is only parsing/validating a single zerocoin spend per tx, while a transaction could contain multiple spends. Fix it by returning an optional list ofCoinSpendValue, instead of a singleton.It re-introduced in the validation the (unused-at-the-time) function
CheckPublicCoinSpendVersion, which is now failing at the first block with (version 3) public spends.As all spork-based checks, this was supposed to be skipped when
fInitialBlockDownload.Zerocoin transactions are no longer accepted (in both nets) in the mempool, as well as inside blocks (after v5 enforcement), so we can just remove this obsolete function.
The third commit fixes a very old bug: a zerocoin spend tx
nValueInis counted twice in ConnectBlock (first directly, after the coinspend parsing/validation, and then viaCCoinsViewCache::GetValueIn).Then, since we are at it, we can deprecate
SPORK_16_ZEROCOIN_MAINTENANCE_MODEandSPORK_18_ZEROCOIN_PUBLICSPEND_V4, which are now unused/unneeded.The last three commits are performance optimizations to avoid multiple calls to
ContainsZerocoins()/HasZerocoinSpendInputs()inConnectBlock, when possible.