From f9a703ed51c32034c6b51a77a058d7c797a60e51 Mon Sep 17 00:00:00 2001 From: Daira Hopwood Date: Thu, 31 Mar 2022 20:04:43 +0100 Subject: [PATCH 1/2] This check done for Sprout and Sapling (which is separate from consensus nullifier checks) was not being done for Orchard. Signed-off-by: Daira Hopwood --- src/txmempool.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 136b97f0c..3dcf1bec2 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -652,6 +652,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const checkNullifiers(SPROUT); checkNullifiers(SAPLING); + checkNullifiers(ORCHARD); assert(totalTxSize == checkTotal); assert(innerUsage == cachedInnerUsage); From 0130426deadd6677d63130bb8d59c913f25700c5 Mon Sep 17 00:00:00 2001 From: Daira Hopwood Date: Fri, 1 Apr 2022 13:19:18 +0100 Subject: [PATCH 2/2] AcceptToMemoryPool: Remove initial nullifier checks for Sprout and Sapling (within the mempool only), that are redundant with checks in HaveShieldedRequirements. The latter take into account nullifiers in the chain (using CCoinsViewMemPool::GetNullifier) and include Orchard. There is no meaningful DoS-protection motivation for the checks being removed here, nor do they simplify reasoning about the code (if anything the redundancy makes it more confusing). Signed-off-by: Daira Hopwood --- src/main.cpp | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 053514e8b..d9d4cae8c 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1823,7 +1823,13 @@ bool AcceptToMemoryPool( if (pool.exists(hash)) return state.Invalid(false, REJECT_ALREADY_KNOWN, "txn-already-in-mempool"); - // Check for conflicts with in-memory transactions + // Check for conflicts with in-memory transactions. + // This is redundant with the call to HaveInputs (for non-coinbase transactions) + // below, but we do it here for consistency with Bitcoin and because this check + // doesn't require loading the coins view. + // We don't do the corresponding check for nullifiers, because that would also + // be redundant, and we have no need to avoid semantic conflicts with Bitcoin + // backports in the case of the shielded protocol. for (unsigned int i = 0; i < tx.vin.size(); i++) { COutPoint outpoint = tx.vin[i].prevout; @@ -1833,18 +1839,6 @@ bool AcceptToMemoryPool( return state.Invalid(false, REJECT_CONFLICT, "txn-mempool-conflict"); } } - for (const JSDescription &joinsplit : tx.vJoinSplit) { - for (const uint256 &nf : joinsplit.nullifiers) { - if (pool.nullifierExists(nf, SPROUT)) { - return false; - } - } - } - for (const SpendDescription &spendDescription : tx.vShieldedSpend) { - if (pool.nullifierExists(spendDescription.nullifier, SAPLING)) { - return false; - } - } { CCoinsView dummy;