diff options
author | fanquake <fanquake@gmail.com> | 2022-01-25 10:13:41 +0800 |
---|---|---|
committer | fanquake <fanquake@gmail.com> | 2022-01-25 10:44:51 +0800 |
commit | 417e7503f80b8b9cfccd43131f313de8defc0ad5 (patch) | |
tree | 50d2db5d1a6ddf4768e4d8868b327afe01061f66 /src/validation.cpp | |
parent | 9ec3991ad3b2ae91997c696a4c2f187fe538eff0 (diff) | |
parent | 3cd7f693d3ed1bb7cf9ba3e0c482174df3684972 (diff) | |
download | bitcoin-417e7503f80b8b9cfccd43131f313de8defc0ad5.tar.xz |
Merge bitcoin/bitcoin#23804: validation: followups for de-duplication of packages
3cd7f693d3ed1bb7cf9ba3e0c482174df3684972 [unit test] package parents are a mix (glozow)
de075a98eaf0b3f7676c5c78b50b66902202b34c [validation] better handle errors in SubmitPackage (glozow)
9d88853e0c85f765f7d982b15e8122ede50110ed AcceptPackage fixups (glozow)
2db77cd3b835d052de678755bcdde5a645ce2d65 [unit test] different witness in package submission (glozow)
9ad211c5753dbd148ba6f0ed56854f6364362ca8 [doc] more detailed explanation for deduplication (glozow)
83d4fb71260f268abd41d083fb3458476aed83ce [packages] return DIFFERENT_WITNESS for same-txid-different-witness tx (glozow)
Pull request description:
This addresses some comments from review on e12fafda2dfbbdf63f125e5af797ecfaa6488f66 from #22674.
- Improve documentation about de-duplication: [comment](https://github.com/bitcoin/bitcoin/pull/22674/files#r770156708)
- Fix code looking up same-txid-different-witness transaction in mempool: [comment](https://github.com/bitcoin/bitcoin/pull/22674/files#r770804029)
- Improve the interface for when a same-txid-different-witness transaction is swapped: [comment](https://github.com/bitcoin/bitcoin/pull/22674/files#r770782822)
- Add a test for witness swapping: [comment](https://github.com/bitcoin/bitcoin/pull/22674/files#r770804029)
- Add a test for packages with a mix of duplicate/different witness/new parents: [comment](https://github.com/bitcoin/bitcoin/pull/22674#discussion_r773037608)
- Fix issue with not notifying `CValidationInterface` when there's a partial submission due to fail-fast: [comment](https://github.com/bitcoin/bitcoin/pull/22674#discussion_r773013162)
ACKs for top commit:
achow101:
ACK 3cd7f693d3ed1bb7cf9ba3e0c482174df3684972
t-bast:
LGTM, ACK https://github.com/bitcoin/bitcoin/pull/23804/commits/3cd7f693d3ed1bb7cf9ba3e0c482174df3684972
instagibbs:
ACK 3cd7f693d3ed1bb7cf9ba3e0c482174df3684972
ariard:
ACK 3cd7f69
Tree-SHA512: a5d86ca86edab80a5a05fcbb828901c058b3f2fa2552912ea52f2871e29c3cf4cc34020e7aac2217959c9c3a01856f4bd3d631d844635b98144f212f76c2f3ef
Diffstat (limited to 'src/validation.cpp')
-rw-r--r-- | src/validation.cpp | 65 |
1 files changed, 42 insertions, 23 deletions
diff --git a/src/validation.cpp b/src/validation.cpp index b3d989e150..72e0b6218e 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -619,10 +619,10 @@ private: // Submit all transactions to the mempool and call ConsensusScriptChecks to add to the script // cache - should only be called after successful validation of all transactions in the package. - // The package may end up partially-submitted after size limitting; returns true if all + // The package may end up partially-submitted after size limiting; returns true if all // transactions are successfully added to the mempool, false otherwise. - bool FinalizePackage(const ATMPArgs& args, std::vector<Workspace>& workspaces, PackageValidationState& package_state, - std::map<const uint256, const MempoolAcceptResult>& results) + bool SubmitPackage(const ATMPArgs& args, std::vector<Workspace>& workspaces, PackageValidationState& package_state, + std::map<const uint256, const MempoolAcceptResult>& results) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Compare a package's feerate against minimum allowed. @@ -1061,12 +1061,17 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) return true; } -bool MemPoolAccept::FinalizePackage(const ATMPArgs& args, std::vector<Workspace>& workspaces, - PackageValidationState& package_state, - std::map<const uint256, const MempoolAcceptResult>& results) +bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>& workspaces, + PackageValidationState& package_state, + std::map<const uint256, const MempoolAcceptResult>& results) { AssertLockHeld(cs_main); AssertLockHeld(m_pool.cs); + // Sanity check: none of the transactions should be in the mempool, and none of the transactions + // should have a same-txid-different-witness equivalent in the mempool. + assert(std::all_of(workspaces.cbegin(), workspaces.cend(), [this](const auto& ws){ + return !m_pool.exists(GenTxid::Txid(ws.m_ptx->GetHash())); })); + bool all_submitted = true; // ConsensusScriptChecks adds to the script cache and is therefore consensus-critical; // CheckInputsFromMempoolAndCache asserts that transactions only spend coins available from the @@ -1076,18 +1081,24 @@ bool MemPoolAccept::FinalizePackage(const ATMPArgs& args, std::vector<Workspace> if (!ConsensusScriptChecks(args, ws)) { results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); // Since PolicyScriptChecks() passed, this should never fail. - all_submitted = Assume(false); + all_submitted = false; + package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR, + strprintf("BUG! PolicyScriptChecks succeeded but ConsensusScriptChecks failed: %s", + ws.m_ptx->GetHash().ToString())); } // Re-calculate mempool ancestors to call addUnchecked(). They may have changed since the // last calculation done in PreChecks, since package ancestors have already been submitted. - std::string err_string; + std::string unused_err_string; if(!m_pool.CalculateMemPoolAncestors(*ws.m_entry, ws.m_ancestors, m_limit_ancestors, m_limit_ancestor_size, m_limit_descendants, - m_limit_descendant_size, err_string)) { + m_limit_descendant_size, unused_err_string)) { results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); // Since PreChecks() and PackageMempoolChecks() both enforce limits, this should never fail. - all_submitted = Assume(false); + all_submitted = false; + package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR, + strprintf("BUG! Mempool ancestors or descendants were underestimated: %s", + ws.m_ptx->GetHash().ToString())); } // If we call LimitMempoolSize() for each individual Finalize(), the mempool will not take // the transaction's descendant feerate into account because it hasn't seen them yet. Also, @@ -1097,7 +1108,9 @@ bool MemPoolAccept::FinalizePackage(const ATMPArgs& args, std::vector<Workspace> if (!Finalize(args, ws)) { results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); // Since LimitMempoolSize() won't be called, this should never fail. - all_submitted = Assume(false); + all_submitted = false; + package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR, + strprintf("BUG! Adding to mempool failed: %s", ws.m_ptx->GetHash().ToString())); } } @@ -1106,7 +1119,6 @@ bool MemPoolAccept::FinalizePackage(const ATMPArgs& args, std::vector<Workspace> LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip(), gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000, std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}); - if (!all_submitted) return false; // Find the wtxids of the transactions that made it into the mempool. Allow partial submission, // but don't report success unless they all made it into the mempool. @@ -1211,8 +1223,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: if (args.m_test_accept) return PackageMempoolAcceptResult(package_state, std::move(results)); - if (!FinalizePackage(args, workspaces, package_state, results)) { - package_state.Invalid(PackageValidationResult::PCKG_TX, "submission failed"); + if (!SubmitPackage(args, workspaces, package_state, results)) { + // PackageValidationState filled in by SubmitPackage(). return PackageMempoolAcceptResult(package_state, std::move(results)); } @@ -1237,11 +1249,13 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, return PackageMempoolAcceptResult(package_state, {}); } - const auto& child = package[package.size() - 1]; + // IsChildWithParents() guarantees the package is > 1 transactions. + assert(package.size() > 1); // The package must be 1 child with all of its unconfirmed parents. The package is expected to // be sorted, so the last transaction is the child. + const auto& child = package.back(); std::unordered_set<uint256, SaltedTxidHasher> unconfirmed_parent_txids; - std::transform(package.cbegin(), package.end() - 1, + std::transform(package.cbegin(), package.cend() - 1, std::inserter(unconfirmed_parent_txids, unconfirmed_parent_txids.end()), [](const auto& tx) { return tx->GetHash(); }); @@ -1273,10 +1287,14 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, LOCK(m_pool.cs); std::map<const uint256, const MempoolAcceptResult> results; - // As node operators are free to set their mempool policies however they please, it's possible - // for package transaction(s) to already be in the mempool, and we don't want to reject the - // entire package in that case (as that could be a censorship vector). Filter the transactions - // that are already in mempool and add their information to results, since we already have them. + // Node operators are free to set their mempool policies however they please, nodes may receive + // transactions in different orders, and malicious counterparties may try to take advantage of + // policy differences to pin or delay propagation of transactions. As such, it's possible for + // some package transaction(s) to already be in the mempool, and we don't want to reject the + // entire package in that case (as that could be a censorship vector). De-duplicate the + // transactions that are already in the mempool, and only call AcceptMultipleTransactions() with + // the new transactions. This ensures we don't double-count transaction counts and sizes when + // checking ancestor/descendant limits, or double-count transaction fees for fee-related policy. std::vector<CTransactionRef> txns_new; for (const auto& tx : package) { const auto& wtxid = tx->GetWitnessHash(); @@ -1297,9 +1315,10 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // transaction for the mempool one. Note that we are ignoring the validity of the // package transaction passed in. // TODO: allow witness replacement in packages. - auto iter = m_pool.GetIter(wtxid); + auto iter = m_pool.GetIter(txid); assert(iter != std::nullopt); - results.emplace(txid, MempoolAcceptResult::MempoolTx(iter.value()->GetTxSize(), iter.value()->GetFee())); + // Provide the wtxid of the mempool tx so that the caller can look it up in the mempool. + results.emplace(wtxid, MempoolAcceptResult::MempoolTxDifferentWitness(iter.value()->GetTx().GetWitnessHash())); } else { // Transaction does not already exist in the mempool. txns_new.push_back(tx); @@ -1366,12 +1385,12 @@ PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTx }(); // Uncache coins pertaining to transactions that were not submitted to the mempool. - // Ensure the coins cache is still within limits. if (test_accept || result.m_state.IsInvalid()) { for (const COutPoint& hashTx : coins_to_uncache) { active_chainstate.CoinsTip().Uncache(hashTx); } } + // Ensure the coins cache is still within limits. BlockValidationState state_dummy; active_chainstate.FlushStateToDisk(state_dummy, FlushStateMode::PERIODIC); return result; |