highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-01-19 06:43:45 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#23804: validation: followups for de-duplication of packages

Browse Source 
3cd7f693d3 [unit test] package parents are a mix (glozow)
de075a98ea [validation] better handle errors in SubmitPackage (glozow)
9d88853e0c AcceptPackage fixups (glozow)
2db77cd3b8 [unit test] different witness in package submission (glozow)
9ad211c575 [doc] more detailed explanation for deduplication (glozow)
83d4fb7126 [packages] return DIFFERENT_WITNESS for same-txid-different-witness tx (glozow)

Pull request description:

  This addresses some comments from review on e12fafda2d 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 3cd7f693d3
  t-bast:
    LGTM, ACK 3cd7f693d3
  instagibbs:
    ACK 3cd7f693d3
  ariard:
    ACK 3cd7f69

Tree-SHA512: a5d86ca86edab80a5a05fcbb828901c058b3f2fa2552912ea52f2871e29c3cf4cc34020e7aac2217959c9c3a01856f4bd3d631d844635b98144f212f76c2f3ef
This commit is contained in:
fanquake
2022-01-25 10:13:41 +08:00
parent 9ec3991ad3 3cd7f693d3
commit 417e7503f8
4 changed files with 292 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

65
src/validation.cpp
View File

@@ -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;