From 47c0c653266ab533e82401f00bfc9648d06b3e8f Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 23 Oct 2020 19:12:43 +0100 Subject: [PATCH 1/4] Improve reject reasons for unmet shielded requirements These reject messages end up bubbling up to users via the RPC interface. Distinguishing between the various failure cases will help users figure out why their transaction is being rejected. Uses operator* instead of std::optional::value because the latter was introduced in macOS 10.14, and our current minimum is 10.12. Closes zcash/zcash#3114. --- src/coins.cpp | 15 +++++++------- src/coins.h | 10 ++++++++- src/main.cpp | 57 +++++++++++++++++++++++++++++++++++++++++---------- 3 files changed, 63 insertions(+), 19 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index f2999df13..0145c225f 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -914,7 +914,7 @@ CAmount CCoinsViewCache::GetValueIn(const CTransaction& tx) const return nResult; } -bool CCoinsViewCache::HaveShieldedRequirements(const CTransaction& tx) const +std::optional CCoinsViewCache::HaveShieldedRequirements(const CTransaction& tx) const { boost::unordered_map intermediates; @@ -925,7 +925,7 @@ bool CCoinsViewCache::HaveShieldedRequirements(const CTransaction& tx) const if (GetNullifier(nullifier, SPROUT)) { // If the nullifier is set, this transaction // double-spends! - return false; + return UnsatisfiedShieldedReq::SproutDuplicateNullifier; } } @@ -934,7 +934,7 @@ bool CCoinsViewCache::HaveShieldedRequirements(const CTransaction& tx) const if (it != intermediates.end()) { tree = it->second; } else if (!GetSproutAnchorAt(joinsplit.anchor, tree)) { - return false; + return UnsatisfiedShieldedReq::SproutUnknownAnchor; } BOOST_FOREACH(const uint256& commitment, joinsplit.commitments) @@ -946,16 +946,17 @@ bool CCoinsViewCache::HaveShieldedRequirements(const CTransaction& tx) const } for (const SpendDescription &spendDescription : tx.vShieldedSpend) { - if (GetNullifier(spendDescription.nullifier, SAPLING)) // Prevent double spends - return false; + if (GetNullifier(spendDescription.nullifier, SAPLING)) { // Prevent double spends + return UnsatisfiedShieldedReq::SaplingDuplicateNullifier; + } SaplingMerkleTree tree; if (!GetSaplingAnchorAt(spendDescription.anchor, tree)) { - return false; + return UnsatisfiedShieldedReq::SaplingUnknownAnchor; } } - return true; + return std::nullopt; } bool CCoinsViewCache::HaveInputs(const CTransaction& tx) const diff --git a/src/coins.h b/src/coins.h index 10999e8d9..67cba90e8 100644 --- a/src/coins.h +++ b/src/coins.h @@ -447,6 +447,14 @@ public: friend class CCoinsViewCache; }; +/** The set of shielded requirements that might be unsatisfied. */ +enum class UnsatisfiedShieldedReq { + SproutDuplicateNullifier, + SproutUnknownAnchor, + SaplingDuplicateNullifier, + SaplingUnknownAnchor, +}; + /** CCoinsView that adds a memory cache for transactions to another CCoinsView */ class CCoinsViewCache : public CCoinsViewBacked { @@ -566,7 +574,7 @@ public: bool HaveInputs(const CTransaction& tx) const; //! Check whether all joinsplit and sapling spend requirements (anchors/nullifiers) are satisfied - bool HaveShieldedRequirements(const CTransaction& tx) const; + std::optional HaveShieldedRequirements(const CTransaction& tx) const; //! Return priority of tx at height nHeight double GetPriority(const CTransaction &tx, int nHeight) const; diff --git a/src/main.cpp b/src/main.cpp index 2d43a1263..4fe2df808 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -117,6 +117,28 @@ const string strMessageMagic = "Zcash Signed Message:\n"; // Internal stuff namespace { + unsigned char ShieldedReqRejectCode(UnsatisfiedShieldedReq shieldedReq) + { + switch (shieldedReq) { + case UnsatisfiedShieldedReq::SproutDuplicateNullifier: + case UnsatisfiedShieldedReq::SaplingDuplicateNullifier: + return REJECT_DUPLICATE; + case UnsatisfiedShieldedReq::SproutUnknownAnchor: + case UnsatisfiedShieldedReq::SaplingUnknownAnchor: + return REJECT_INVALID; + } + } + + std::string ShieldedReqRejectReason(UnsatisfiedShieldedReq shieldedReq) + { + switch (shieldedReq) { + case UnsatisfiedShieldedReq::SproutDuplicateNullifier: return "bad-txns-sprout-duplicate-nullifier"; + case UnsatisfiedShieldedReq::SproutUnknownAnchor: return "bad-txns-sprout-unknown-anchor"; + case UnsatisfiedShieldedReq::SaplingDuplicateNullifier: return "bad-txns-sapling-duplicate-nullifier"; + case UnsatisfiedShieldedReq::SaplingUnknownAnchor: return "bad-txns-sapling-unknown-anchor"; + } + } + /** Abort with a message */ bool AbortNode(const std::string& strMessage, const std::string& userMessage="") { @@ -1542,10 +1564,14 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa return state.Invalid(error("AcceptToMemoryPool: inputs already spent"), REJECT_DUPLICATE, "bad-txns-inputs-spent"); - // are the joinsplits' and sapling spends' requirements met in tx(valid anchors/nullifiers)? - if (!view.HaveShieldedRequirements(tx)) - return state.Invalid(error("AcceptToMemoryPool: shielded requirements not met"), - REJECT_DUPLICATE, "bad-txns-shielded-requirements-not-met"); + // Are the shielded spends' requirements met? + auto unmetShieldedReq = view.HaveShieldedRequirements(tx); + if (unmetShieldedReq) { + auto rejectCode = ShieldedReqRejectCode(*unmetShieldedReq); + auto rejectReason = ShieldedReqRejectReason(*unmetShieldedReq); + return state.Invalid(error("AcceptToMemoryPool: shielded requirements not met (%s)", rejectReason), + rejectCode, rejectReason); + } // Bring the best block into scope view.GetBestBlock(); @@ -2176,9 +2202,14 @@ bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoins if (!inputs.HaveInputs(tx)) return state.Invalid(error("CheckInputs(): %s inputs unavailable", tx.GetHash().ToString())); - // are the JoinSplit's requirements met? - if (!inputs.HaveShieldedRequirements(tx)) - return state.Invalid(error("CheckInputs(): %s JoinSplit requirements not met", tx.GetHash().ToString())); + // Are the shielded spends' requirements met? + auto unmetShieldedReq = inputs.HaveShieldedRequirements(tx); + if (unmetShieldedReq) { + auto rejectCode = ShieldedReqRejectCode(*unmetShieldedReq); + auto rejectReason = ShieldedReqRejectReason(*unmetShieldedReq); + return state.Invalid(error("CheckInputs(): %s shielded requirements not met (%s)", tx.GetHash().ToString(), rejectReason), + rejectCode, rejectReason); + } CAmount nValueIn = 0; CAmount nFees = 0; @@ -2861,10 +2892,14 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin return state.DoS(100, error("ConnectBlock(): inputs missing/spent"), REJECT_INVALID, "bad-txns-inputs-missingorspent"); - // are the JoinSplit's requirements met? - if (!view.HaveShieldedRequirements(tx)) - return state.DoS(100, error("ConnectBlock(): JoinSplit requirements not met"), - REJECT_INVALID, "bad-txns-joinsplit-requirements-not-met"); + // Are the shielded spends' requirements met? + auto unmetShieldedReq = view.HaveShieldedRequirements(tx); + if (unmetShieldedReq) { + auto rejectCode = ShieldedReqRejectCode(*unmetShieldedReq); + auto rejectReason = ShieldedReqRejectReason(*unmetShieldedReq); + return state.DoS(100, error("ConnectBlock(): shielded requirements not met (%s)", rejectReason), + rejectCode, rejectReason); + } // insightexplorer // https://github.com/bitpay/bitcoin/commit/017f548ea6d89423ef568117447e61dd5707ec42#diff-7ec3c68a81efff79b6ca22ac1f1eabbaR2597 From 6fa2d1b73e445dc899f4ee310952b9bf25841e23 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 23 Oct 2020 19:28:02 +0100 Subject: [PATCH 2/4] Add logging to CCoinsViewCache::HaveShieldedRequirements Enable with -debug=consensus --- src/coins.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/coins.cpp b/src/coins.cpp index 0145c225f..e568e7674 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -11,6 +11,8 @@ #include +#include + /** * calculate number of bytes for the bitmask, and its number of non-zero bytes * each bit in the bitmask represents the availability of one output, but the @@ -925,6 +927,11 @@ std::optional CCoinsViewCache::HaveShieldedRequirements( if (GetNullifier(nullifier, SPROUT)) { // If the nullifier is set, this transaction // double-spends! + auto txid = tx.GetHash().ToString(); + auto nf = nullifier.ToString(); + TracingWarn("consensus", "Sprout double-spend detected", + "txid", txid.c_str(), + "nf", nf.c_str()); return UnsatisfiedShieldedReq::SproutDuplicateNullifier; } } @@ -934,6 +941,11 @@ std::optional CCoinsViewCache::HaveShieldedRequirements( if (it != intermediates.end()) { tree = it->second; } else if (!GetSproutAnchorAt(joinsplit.anchor, tree)) { + auto txid = tx.GetHash().ToString(); + auto anchor = joinsplit.anchor.ToString(); + TracingWarn("consensus", "Transaction uses unknown Sprout anchor", + "txid", txid.c_str(), + "anchor", anchor.c_str()); return UnsatisfiedShieldedReq::SproutUnknownAnchor; } @@ -947,11 +959,21 @@ std::optional CCoinsViewCache::HaveShieldedRequirements( for (const SpendDescription &spendDescription : tx.vShieldedSpend) { if (GetNullifier(spendDescription.nullifier, SAPLING)) { // Prevent double spends + auto txid = tx.GetHash().ToString(); + auto nf = spendDescription.nullifier.ToString(); + TracingWarn("consensus", "Sapling double-spend detected", + "txid", txid.c_str(), + "nf", nf.c_str()); return UnsatisfiedShieldedReq::SaplingDuplicateNullifier; } SaplingMerkleTree tree; if (!GetSaplingAnchorAt(spendDescription.anchor, tree)) { + auto txid = tx.GetHash().ToString(); + auto anchor = spendDescription.anchor.ToString(); + TracingWarn("consensus", "Transaction uses unknown Sapling anchor", + "txid", txid.c_str(), + "anchor", anchor.c_str()); return UnsatisfiedShieldedReq::SaplingUnknownAnchor; } } From 8f56306359a1f5870c8339d0b1363b9d483074eb Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 26 Oct 2020 14:12:57 +0000 Subject: [PATCH 3/4] Add txid to "shielded requirements not met" messages --- src/main.cpp | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 4fe2df808..f06bf7e87 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1567,10 +1567,14 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa // Are the shielded spends' requirements met? auto unmetShieldedReq = view.HaveShieldedRequirements(tx); if (unmetShieldedReq) { + auto txid = tx.GetHash().ToString(); auto rejectCode = ShieldedReqRejectCode(*unmetShieldedReq); auto rejectReason = ShieldedReqRejectReason(*unmetShieldedReq); - return state.Invalid(error("AcceptToMemoryPool: shielded requirements not met (%s)", rejectReason), - rejectCode, rejectReason); + TracingError( + "main", "AcceptToMemoryPool(): shielded requirements not met", + "txid", txid.c_str(), + "reason", rejectReason.c_str()); + return state.Invalid(false, rejectCode, rejectReason); } // Bring the best block into scope @@ -2205,10 +2209,14 @@ bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoins // Are the shielded spends' requirements met? auto unmetShieldedReq = inputs.HaveShieldedRequirements(tx); if (unmetShieldedReq) { + auto txid = tx.GetHash().ToString(); auto rejectCode = ShieldedReqRejectCode(*unmetShieldedReq); auto rejectReason = ShieldedReqRejectReason(*unmetShieldedReq); - return state.Invalid(error("CheckInputs(): %s shielded requirements not met (%s)", tx.GetHash().ToString(), rejectReason), - rejectCode, rejectReason); + TracingError( + "main", "CheckInputs(): shielded requirements not met", + "txid", txid.c_str(), + "reason", rejectReason.c_str()); + return state.Invalid(false, rejectCode, rejectReason); } CAmount nValueIn = 0; @@ -2895,10 +2903,14 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin // Are the shielded spends' requirements met? auto unmetShieldedReq = view.HaveShieldedRequirements(tx); if (unmetShieldedReq) { + auto txid = tx.GetHash().ToString(); auto rejectCode = ShieldedReqRejectCode(*unmetShieldedReq); auto rejectReason = ShieldedReqRejectReason(*unmetShieldedReq); - return state.DoS(100, error("ConnectBlock(): shielded requirements not met (%s)", rejectReason), - rejectCode, rejectReason); + TracingError( + "main", "ConnectBlock(): shielded requirements not met", + "txid", txid.c_str(), + "reason", rejectReason.c_str()); + return state.DoS(100, false, rejectCode, rejectReason); } // insightexplorer From 1b42734b9f054df0981091c1cbea18a845e6f6e5 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 26 Oct 2020 23:46:19 +0000 Subject: [PATCH 4/4] tests: Update chained_joinsplits test for HaveShieldedRequirements API change --- src/test/coins_tests.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 01a4bf818..a092ca03e 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -644,7 +644,7 @@ BOOST_AUTO_TEST_CASE(chained_joinsplits) CMutableTransaction mtx; mtx.vJoinSplit.push_back(js2); - BOOST_CHECK(!cache.HaveShieldedRequirements(mtx)); + BOOST_CHECK(cache.HaveShieldedRequirements(mtx) == std::optional(UnsatisfiedShieldedReq::SproutUnknownAnchor)); } { @@ -654,7 +654,7 @@ BOOST_AUTO_TEST_CASE(chained_joinsplits) mtx.vJoinSplit.push_back(js2); mtx.vJoinSplit.push_back(js1); - BOOST_CHECK(!cache.HaveShieldedRequirements(mtx)); + BOOST_CHECK(cache.HaveShieldedRequirements(mtx) == std::optional(UnsatisfiedShieldedReq::SproutUnknownAnchor)); } { @@ -662,7 +662,7 @@ BOOST_AUTO_TEST_CASE(chained_joinsplits) mtx.vJoinSplit.push_back(js1); mtx.vJoinSplit.push_back(js2); - BOOST_CHECK(cache.HaveShieldedRequirements(mtx)); + BOOST_CHECK(cache.HaveShieldedRequirements(mtx) == std::nullopt); } { @@ -671,7 +671,7 @@ BOOST_AUTO_TEST_CASE(chained_joinsplits) mtx.vJoinSplit.push_back(js2); mtx.vJoinSplit.push_back(js3); - BOOST_CHECK(cache.HaveShieldedRequirements(mtx)); + BOOST_CHECK(cache.HaveShieldedRequirements(mtx) == std::nullopt); } { @@ -681,7 +681,7 @@ BOOST_AUTO_TEST_CASE(chained_joinsplits) mtx.vJoinSplit.push_back(js2); mtx.vJoinSplit.push_back(js3); - BOOST_CHECK(cache.HaveShieldedRequirements(mtx)); + BOOST_CHECK(cache.HaveShieldedRequirements(mtx) == std::nullopt); } }