[Core] Sapling transaction version #1955
Merged
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
A Shielded transaction is defined as having version >= SAPLING_VERSION and non-nullopt (and non-empty) sapData.
to avoid possible confusion with CTransaction::IsShieldedTx()
After v5, the version is enforced by consensus. Remove the check from IsStandardTx and move it to CheckTransaction
1 task
furszy
reviewed
Nov 7, 2020
1 task
random-zebra
added a commit
that referenced
this pull request
Nov 10, 2020
…isSaplingVersion fc9ffe5 SSPKM: fix GetShieldedChange, tx.isSapling migrating to tx.isSaplingVersion (furszy) Pull request description: Small and quick fix, master is currently not compiling due #1955 and #1952 getting merged without being based one on the other. the first PR renamed the tx `isSapling` method to `isSaplingVersion` and in the second PR, a new function was implemented `GetShieldedChange` that was using using `isSapling`. ACKs for top commit: Fuzzbawls: ACK fc9ffe5 random-zebra: utACK fc9ffe5 and merging... Tree-SHA512: 463df6059b756bf1301faad5b3780b74720e5520232686b225bb3af02288e441af70a44a19803e530c5b922d92fd7feb949ecfc59685f797d7943471b6137bc3
Fuzzbawls
added a commit
that referenced
this pull request
Nov 11, 2020
3f80e84 [Refactor] Remove BOOST_STATIC_ASSERT in CheckTransaction (random-zebra) c004841 [Test] Lower minrelaytxfee for feature_fee_estimation (random-zebra) f289a13 [BUG] Fix transaction size limits for sapling version (random-zebra) Pull request description: The size limits for a sapling transaction are currently checked in 3 places: 1- `CheckTransaction` (validation.cpp) 2- `CheckTransactionWithoutProofVerification` (called by `SaplingValidation::CheckTransaction`) 3- `SaplingValidation::ContextualCheckTransaction` While in (2) and (3) the size is correctly checked against `MAX_TX_SIZE_AFTER_SAPLING`, in (1) it is checked against the old `MAX_ZEROCOIN_TX_SIZE`. So a shielded transaction with size greater than `MAX_ZEROCOIN_TX_SIZE` would be wrongly rejected in `CheckTransaction`. Fix the check in (1), selecting the correct max size for shielded/non-shielded transactions. Move the size-check before calling SaplingValidation::CheckTransaction (as it's faster). Remove the checks in (2) and (3) that are just redundant. Based on top of: - [x] #1955 ACKs for top commit: furszy: ACK 3f80e84 Fuzzbawls: ACK 3f80e84 Tree-SHA512: e196f094cd3f33e21a83b2da2cf13a84dc4706547d8de4c61a17fbeb270052550b36ae0c5c8407aa090c011e16ab0381cdc80c7a555b7c92effbe9735b8c8392
furszy
added a commit
that referenced
this pull request
Nov 12, 2020
7e2d5a6 [Refactor] Migrate mapSaplingNullifiers to use CTransactionRef (random-zebra) 4813286 [Mempool] Remove txes spending from a disconnected root (random-zebra) 2173e16 [Trivial] Make CTxMemPool::checkNullifiers() private (random-zebra) 266134a [Validation] Reject txes if double spending nullifier in mempool (random-zebra) c74e86e [Mempool] Consistency check: nullifiers for notes spent in mempool txes (random-zebra) 3dfc4fd [Mempool][Sapling] Introduce CCoinsViewMemPool::GetNullifier (random-zebra) ef17855 [Mempool][Sapling] Introduce CTxMemPool::checkNullifiers (random-zebra) db86e92 [Mempool][Sapling] Update nullifiers for conflicted txes (random-zebra) ad82962 [Mempool] Update spent nullifiers in CTxMemPool::add[remove]Unchecked (random-zebra) c4035a9 [Mempool] Add nullifiers map and introduce CTxMemPool::nullifierExists (random-zebra) Pull request description: Keep track of nullifiers of notes spent by mempool transactions (preventing in-mempool double spends). Add consistency checks with coins view nullifiers. Also, if the sapling anchor changes after a disconnection, we must evict from mempool any transaction that spends from the now-invalid root. Built on top of: - [x] #1955 ACKs for top commit: Fuzzbawls: utACK 7e2d5a6 furszy: ACK 7e2d5a6 Tree-SHA512: 22fd990d7738860fe19a3a1776eea8a223cbcea76c9ffd3ce7e679c8770542a5b5c5f1a07cd88716ec6c0c75a61c738ac51846afed8ca5baaa4417c854d2d3b0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Uniform the definition of
isSaplingtransaction, which originally was meant only for txes with version equal to 2, and it was later changed to include also txes with version greater than 2 (but not updated everywhere).Add a
IsShieldedTxutility function defining "shielded tx" any transaction with isSapling version and with non-empty sapData.Use
GetTotalSizeto get the serialized size of a transaction (instead of calling GetSerializeSize withCTransaction::CURRENT_VERSION).Remove the possibility of having non-standard txes due to their version, after v5 enforcement.
Transactions with invalid version are now directly rejected in CheckTransaction.
Re-enables the IsStandardTx check on regtest.