Skip to content

Commit

Permalink
relax child-with-unconfirmed-parents rule
Browse files Browse the repository at this point in the history
This rule was originally introduced along with a very early proposal for
package relay as a way to verify that the "correct"
child-with-unconfirmed-parents package was provided for a transaction,
where correctness was defined as all of the transactions unconfirmed
parents. However, we are not planning to introduce a protocol where
peers would be asked to send these packages.

This rule has downsides: if a transaction has multiple parents but only
1 that requires package CPFP to be accepted, the current rule prevents
us from accepting that package. Even if the other parents are already in
mempool, the p2p logic will only submit the 1p1c package, which fails
this check. See the test in p2p_1p1c_network.py
  • Loading branch information
glozow committed Nov 28, 2024
1 parent 32fc597 commit dad2872
Showing 7 changed files with 16 additions and 77 deletions.
8 changes: 1 addition & 7 deletions doc/policy/packages.md
Original file line number Diff line number Diff line change
@@ -8,12 +8,6 @@ Graph (a directed edge exists between a transaction that spends the output of an
For every transaction `t` in a **topologically sorted** package, if any of its parents are present
in the package, they appear somewhere in the list before `t`.

A **child-with-unconfirmed-parents** package is a topologically sorted package that consists of
exactly one child and all of its unconfirmed parents (no other transactions may be present).
The last transaction in the package is the child, and its package can be canonically defined based
on the current state: each of its inputs must be available in the UTXO set as of the current chain
tip or some preceding transaction in the package.

## Package Mempool Acceptance Rules

The following rules are enforced for all packages:
@@ -75,7 +69,7 @@ The following rules are enforced for all packages:
The following rules are only enforced for packages to be submitted to the mempool (not
enforced for test accepts):

* Packages must be child-with-unconfirmed-parents packages. This also means packages must contain at
* Packages must be child-with-parents packages. This also means packages must contain at
least 1 transaction. (#31096)

- *Rationale*: This allows for fee-bumping by CPFP. Allowing multiple parents makes it possible
2 changes: 1 addition & 1 deletion src/policy/packages.h
Original file line number Diff line number Diff line change
@@ -26,7 +26,7 @@ static_assert(MAX_PACKAGE_WEIGHT >= MAX_STANDARD_TX_WEIGHT);

// If a package is to be evaluated, it must be at least as large as the mempool's ancestor/descendant limits,
// otherwise transactions that would be individually accepted may be rejected in a package erroneously.
// Since a submitted package must be child-with-unconfirmed-parents (all of the transactions are an ancestor
// Since a submitted package must be child-with-parents (all of the transactions are an ancestor
// of the child), package limits are ultimately bounded by mempool package limits. Ensure that the
// defaults reflect this constraint.
static_assert(DEFAULT_DESCENDANT_LIMIT >= MAX_PACKAGE_COUNT);
14 changes: 0 additions & 14 deletions src/test/txpackage_tests.cpp
Original file line number Diff line number Diff line change
@@ -441,20 +441,6 @@ BOOST_AUTO_TEST_CASE(package_submission_tests)
BOOST_CHECK_EQUAL(result_quit_early.m_state.GetResult(), PackageValidationResult::PCKG_TX);
}

// Child with missing parent.
mtx_child.vin.emplace_back(COutPoint(package_unrelated[0]->GetHash(), 0));
Package package_missing_parent;
package_missing_parent.push_back(tx_parent);
package_missing_parent.push_back(MakeTransactionRef(mtx_child));
{
const auto result_missing_parent = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
package_missing_parent, /*test_accept=*/false, /*client_maxfeerate=*/{});
BOOST_CHECK(result_missing_parent.m_state.IsInvalid());
BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetResult(), PackageValidationResult::PCKG_POLICY);
BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetRejectReason(), "package-not-child-with-unconfirmed-parents");
BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
}

// Submit package with parent + child.
{
const auto submit_parent_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
48 changes: 5 additions & 43 deletions src/validation.cpp
Original file line number Diff line number Diff line change
@@ -512,7 +512,7 @@ class MemPoolAccept
};
}

/** Parameters for child-with-unconfirmed-parents package validation. */
/** Parameters for child-with-parents package validation. */
static ATMPArgs PackageChildWithParents(const CChainParams& chainparams, int64_t accept_time,
std::vector<COutPoint>& coins_to_uncache, const std::optional<CFeeRate>& client_maxfeerate) {
return ATMPArgs{/* m_chainparams */ chainparams,
@@ -1679,7 +1679,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,

// There are two topologies we are able to handle through this function:
// (1) A single transaction
// (2) A child-with-unconfirmed-parents package.
// (2) A child-with-parents package.
// Check that the package is well-formed. If it isn't, we won't try to validate any of the
// transactions and thus won't return any MempoolAcceptResults, just a package-wide error.

@@ -1688,49 +1688,11 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
return PackageMempoolAcceptResult(package_state_quit_early, {});
}

if (package.size() > 1) {
if (package.size() > 1 && !IsChildWithParents(package)) {
// All transactions in the package must be a parent of the last transaction. This is just an
// opportunity for us to fail fast on a context-free check without taking the mempool lock.
if (!IsChildWithParents(package)) {
package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-parents");
return PackageMempoolAcceptResult(package_state_quit_early, {});
}

// 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.cend() - 1,
std::inserter(unconfirmed_parent_txids, unconfirmed_parent_txids.end()),
[](const auto& tx) { return tx->GetHash(); });

// All child inputs must refer to a preceding package transaction or a confirmed UTXO. The only
// way to verify this is to look up the child's inputs in our current coins view (not including
// mempool), and enforce that all parents not present in the package be available at chain tip.
// Since this check can bring new coins into the coins cache, keep track of these coins and
// uncache them if we don't end up submitting this package to the mempool.
const CCoinsViewCache& coins_tip_cache = m_active_chainstate.CoinsTip();
for (const auto& input : child->vin) {
if (!coins_tip_cache.HaveCoinInCache(input.prevout)) {
args.m_coins_to_uncache.push_back(input.prevout);
}
}
// Using the MemPoolAccept m_view cache allows us to look up these same coins faster later.
// This should be connecting directly to CoinsTip, not to m_viewmempool, because we specifically
// require inputs to be confirmed if they aren't in the package.
m_view.SetBackend(m_active_chainstate.CoinsTip());
const auto package_or_confirmed = [this, &unconfirmed_parent_txids](const auto& input) {
return unconfirmed_parent_txids.count(input.prevout.hash) > 0 || m_view.HaveCoin(input.prevout);
};
if (!std::all_of(child->vin.cbegin(), child->vin.cend(), package_or_confirmed)) {
package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-unconfirmed-parents");
return PackageMempoolAcceptResult(package_state_quit_early, {});
}
// Protect against bugs where we pull more inputs from disk that miss being added to
// coins_to_uncache. The backend will be connected again when needed in PreChecks.
m_view.SetBackend(m_dummy);
package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-parents");
return PackageMempoolAcceptResult(package_state_quit_early, {});
}

LOCK(m_pool.cs);
2 changes: 1 addition & 1 deletion test/functional/p2p_1p1c_network.py
Original file line number Diff line number Diff line change
@@ -103,7 +103,7 @@ def create_packages(self):
package_hex_2, parent_2, child_2 = self.create_basic_1p1c(self.wallet_nonsegwit)

# 3: 2-parent-1-child package. Both parents are above mempool min feerate. No package submission happens.
# We require packages to be child-with-unconfirmed-parents and only allow 1-parent-1-child packages.
# We require packages to be child-with-parents and only allow 1-parent-1-child packages.
package_hex_3, parent_31, _parent_32, child_3 = self.create_package_2p1c(self.wallet)

# 4: parent + child package where the child spends 2 different outputs from the parent.
13 changes: 4 additions & 9 deletions test/functional/p2p_opportunistic_1p1c.py
Original file line number Diff line number Diff line change
@@ -340,7 +340,7 @@ def test_multiple_parents(self):

@cleanup
def test_other_parent_in_mempool(self):
self.log.info("Check opportunistic 1p1c fails if child already has another parent in mempool")
self.log.info("Check opportunistic 1p1c works even if child already has another parent in mempool")
node = self.nodes[0]

# This parent needs CPFP
@@ -361,20 +361,15 @@ def test_other_parent_in_mempool(self):
# 2. Send child.
peer_sender.send_and_ping(msg_tx(child["tx"]))

# 3. Node requests parent_low. However, 1p1c fails because package-not-child-with-unconfirmed-parents
# 3. Node requests parent_low.
parent_low_txid_int = int(parent_low["txid"], 16)
peer_sender.wait_for_getdata([parent_low_txid_int])
peer_sender.send_and_ping(msg_tx(parent_low["tx"]))

node_mempool = node.getrawmempool()
assert parent_high["txid"] in node_mempool
assert parent_low["txid"] not in node_mempool
assert child["txid"] not in node_mempool

# Same error if submitted through submitpackage without parent_high
package_hex_missing_parent = [parent_low["hex"], child["hex"]]
result_missing_parent = node.submitpackage(package_hex_missing_parent)
assert_equal(result_missing_parent["package_msg"], "package-not-child-with-unconfirmed-parents")
assert parent_low["txid"] in node_mempool
assert child["txid"] in node_mempool

def run_test(self):
node = self.nodes[0]
6 changes: 4 additions & 2 deletions test/functional/rpc_packages.py
Original file line number Diff line number Diff line change
@@ -267,9 +267,11 @@ def test_conflicting(self):
assert_equal(submitres, {'package_msg': 'conflict-in-package', 'tx-results': {}, 'replaced-transactions': []})
assert tx_child["txid"] not in node.getrawmempool()

# ... and without the in-mempool ancestor tx1 included in the call
# without the in-mempool ancestor tx1 included in the call, tx2 can be submitted, but
# tx_child is missing an input.
submitres = node.submitpackage([tx2["hex"], tx_child["hex"]])
assert_equal(submitres, {'package_msg': 'package-not-child-with-unconfirmed-parents', 'tx-results': {}, 'replaced-transactions': []})
assert_equal(submitres["tx-results"][tx_child["wtxid"]], {"txid": tx_child["txid"], "error": "bad-txns-inputs-missingorspent"})
assert tx2["txid"] in node.getrawmempool()

# Regardless of error type, the child can never enter the mempool
assert tx_child["txid"] not in node.getrawmempool()

0 comments on commit dad2872

Please sign in to comment.