59 changes: 59 additions & 0 deletions src/policy/v3_policy.h
@@ -0,0 +1,59 @@
// Copyright (c) 2022 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#ifndef BITCOIN_POLICY_V3_POLICY_H
#define BITCOIN_POLICY_V3_POLICY_H

#include <consensus/amount.h>
#include <policy/packages.h>
#include <policy/policy.h>
#include <primitives/transaction.h>
#include <txmempool.h>

#include <string>

// This module enforces rules for transactions with nVersion=3 ("V3 transactions") which help make
// RBF abilities more robust.

// V3 only allows 1 parent and 1 child.
/** Maximum number of transactions including an unconfirmed tx and its descendants. */
static constexpr unsigned int V3_DESCENDANT_LIMIT{2};
/** Maximum number of transactions including a V3 tx and all its mempool ancestors. */
static constexpr unsigned int V3_ANCESTOR_LIMIT{2};

/** Maximum weight of a tx which spends from an unconfirmed V3 transaction. */
static constexpr int64_t V3_CHILD_MAX_WEIGHT{4000};
// Since these limits are within the default ancestor/descendant limits, there is no need to
// additionally check ancestor/descendant limits for V3 transactions.
static_assert(V3_CHILD_MAX_WEIGHT + MAX_STANDARD_TX_WEIGHT <= DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * WITNESS_SCALE_FACTOR * 1000);
static_assert(V3_CHILD_MAX_WEIGHT + MAX_STANDARD_TX_WEIGHT <= DEFAULT_DESCENDANT_SIZE_LIMIT_KVB * WITNESS_SCALE_FACTOR * 1000);

/** Any two unconfirmed transactions with a dependency relationship must either both be V3 or both
* non-V3. Check this rule for any list of unconfirmed transactions.
* @returns a tuple (parent wtxid, child wtxid, bool) where one is V3 but the other is not, if at
* least one such pair exists. The bool represents whether the child is v3 or not. There may be
* other such pairs that are not returned.
* Otherwise std::nullopt.
*/
std::optional<std::tuple<uint256, uint256, bool>> CheckV3Inheritance(const Package& package);

/** Every transaction that spends an unconfirmed V3 transaction must also be V3. */
std::optional<std::string> CheckV3Inheritance(const CTransactionRef& ptx,
const CTxMemPool::setEntries& ancestors);

/** The following rules apply to V3 transactions:
* 1. Tx with all of its ancestors (including non-nVersion=3) must be within V3_ANCESTOR_SIZE_LIMIT_KVB.
* 2. Tx with all of its ancestors must be within V3_ANCESTOR_LIMIT.
*
* If a V3 tx has V3 ancestors,
* 1. Each V3 ancestor and its descendants must be within V3_DESCENDANT_LIMIT.
* 2. The tx must be within V3_CHILD_MAX_SIZE.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 2. The tx must be within V3_CHILD_MAX_SIZE.
* 2. The tx must be within V3_CHILD_MAX_WEIGHT

*
* @returns an error string if any V3 rule was violated, otherwise std::nullopt.
*/
std::optional<std::string> ApplyV3Rules(const CTransactionRef& ptx,
const CTxMemPool::setEntries& ancestors,
const std::set<uint256>& direct_conflicts);

#endif // BITCOIN_POLICY_V3_POLICY_H
136 changes: 96 additions & 40 deletions src/test/rbf_tests.cpp
Expand Up @@ -89,60 +89,62 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup)
const auto tx8 = make_tx(/*inputs=*/ {m_coinbase_txns[4]}, /*output_values=*/ {999 * CENT});
pool.addUnchecked(entry.Fee(high_fee).FromTx(tx8));

const auto entry1 = pool.GetIter(tx1->GetHash()).value();
const auto entry2 = pool.GetIter(tx2->GetHash()).value();
const auto entry3 = pool.GetIter(tx3->GetHash()).value();
const auto entry4 = pool.GetIter(tx4->GetHash()).value();
const auto entry5 = pool.GetIter(tx5->GetHash()).value();
const auto entry6 = pool.GetIter(tx6->GetHash()).value();
const auto entry7 = pool.GetIter(tx7->GetHash()).value();
const auto entry8 = pool.GetIter(tx8->GetHash()).value();

BOOST_CHECK_EQUAL(entry1->GetFee(), normal_fee);
BOOST_CHECK_EQUAL(entry2->GetFee(), normal_fee);
BOOST_CHECK_EQUAL(entry3->GetFee(), low_fee);
BOOST_CHECK_EQUAL(entry4->GetFee(), high_fee);
BOOST_CHECK_EQUAL(entry5->GetFee(), low_fee);
BOOST_CHECK_EQUAL(entry6->GetFee(), low_fee);
BOOST_CHECK_EQUAL(entry7->GetFee(), high_fee);
BOOST_CHECK_EQUAL(entry8->GetFee(), high_fee);

CTxMemPool::setEntries set_12_normal{entry1, entry2};
CTxMemPool::setEntries set_34_cpfp{entry3, entry4};
CTxMemPool::setEntries set_56_low{entry5, entry6};
CTxMemPool::setEntries all_entries{entry1, entry2, entry3, entry4, entry5, entry6, entry7, entry8};
const auto entry1_normal = pool.GetIter(tx1->GetHash()).value();
const auto entry2_normal = pool.GetIter(tx2->GetHash()).value();
const auto entry3_low = pool.GetIter(tx3->GetHash()).value();
const auto entry4_high = pool.GetIter(tx4->GetHash()).value();
const auto entry5_low = pool.GetIter(tx5->GetHash()).value();
const auto entry6_low_prioritised = pool.GetIter(tx6->GetHash()).value();
const auto entry7_high = pool.GetIter(tx7->GetHash()).value();
const auto entry8_high = pool.GetIter(tx8->GetHash()).value();

BOOST_CHECK_EQUAL(entry1_normal->GetFee(), normal_fee);
BOOST_CHECK_EQUAL(entry2_normal->GetFee(), normal_fee);
BOOST_CHECK_EQUAL(entry3_low->GetFee(), low_fee);
BOOST_CHECK_EQUAL(entry4_high->GetFee(), high_fee);
BOOST_CHECK_EQUAL(entry5_low->GetFee(), low_fee);
BOOST_CHECK_EQUAL(entry6_low_prioritised->GetFee(), low_fee);
BOOST_CHECK_EQUAL(entry7_high->GetFee(), high_fee);
BOOST_CHECK_EQUAL(entry8_high->GetFee(), high_fee);

CTxMemPool::setEntries set_12_normal{entry1_normal, entry2_normal};
CTxMemPool::setEntries set_34_cpfp{entry3_low, entry4_high};
CTxMemPool::setEntries set_56_low{entry5_low, entry6_low_prioritised};
CTxMemPool::setEntries set_78_high{entry7_high, entry8_high};
CTxMemPool::setEntries all_entries{entry1_normal, entry2_normal, entry3_low, entry4_high,
entry5_low, entry6_low_prioritised, entry7_high, entry8_high};
CTxMemPool::setEntries empty_set;

const auto unused_txid{GetRandHash()};

// Tests for PaysMoreThanConflicts
// These tests use feerate, not absolute fee.
BOOST_CHECK(PaysMoreThanConflicts(/*iters_conflicting=*/set_12_normal,
/*replacement_feerate=*/CFeeRate(entry1->GetModifiedFee() + 1, entry1->GetTxSize() + 2),
/*replacement_feerate=*/CFeeRate(entry1_normal->GetModifiedFee() + 1, entry1_normal->GetTxSize() + 2),
/*txid=*/unused_txid).has_value());
// Replacement must be strictly greater than the originals.
BOOST_CHECK(PaysMoreThanConflicts(set_12_normal, CFeeRate(entry1->GetModifiedFee(), entry1->GetTxSize()), unused_txid).has_value());
BOOST_CHECK(PaysMoreThanConflicts(set_12_normal, CFeeRate(entry1->GetModifiedFee() + 1, entry1->GetTxSize()), unused_txid) == std::nullopt);
BOOST_CHECK(PaysMoreThanConflicts(set_12_normal, CFeeRate(entry1_normal->GetModifiedFee(), entry1_normal->GetTxSize()), unused_txid).has_value());
BOOST_CHECK(PaysMoreThanConflicts(set_12_normal, CFeeRate(entry1_normal->GetModifiedFee() + 1, entry1_normal->GetTxSize()), unused_txid) == std::nullopt);
// These tests use modified fees (including prioritisation), not base fees.
BOOST_CHECK(PaysMoreThanConflicts({entry5}, CFeeRate(entry5->GetModifiedFee() + 1, entry5->GetTxSize()), unused_txid) == std::nullopt);
BOOST_CHECK(PaysMoreThanConflicts({entry6}, CFeeRate(entry6->GetFee() + 1, entry6->GetTxSize()), unused_txid).has_value());
BOOST_CHECK(PaysMoreThanConflicts({entry6}, CFeeRate(entry6->GetModifiedFee() + 1, entry6->GetTxSize()), unused_txid) == std::nullopt);
BOOST_CHECK(PaysMoreThanConflicts({entry5_low}, CFeeRate(entry5_low->GetModifiedFee() + 1, entry5_low->GetTxSize()), unused_txid) == std::nullopt);
BOOST_CHECK(PaysMoreThanConflicts({entry6_low_prioritised}, CFeeRate(entry6_low_prioritised->GetFee() + 1, entry6_low_prioritised->GetTxSize()), unused_txid).has_value());
BOOST_CHECK(PaysMoreThanConflicts({entry6_low_prioritised}, CFeeRate(entry6_low_prioritised->GetModifiedFee() + 1, entry6_low_prioritised->GetTxSize()), unused_txid) == std::nullopt);
// PaysMoreThanConflicts checks individual feerate, not ancestor feerate. This test compares
// replacement_feerate and entry4's feerate, which are the same. The replacement_feerate is
// considered too low even though entry4 has a low ancestor feerate.
BOOST_CHECK(PaysMoreThanConflicts(set_34_cpfp, CFeeRate(entry4->GetModifiedFee(), entry4->GetTxSize()), unused_txid).has_value());
// replacement_feerate and entry4_high's feerate, which are the same. The replacement_feerate is
// considered too low even though entry4_high has a low ancestor feerate.
BOOST_CHECK(PaysMoreThanConflicts(set_34_cpfp, CFeeRate(entry4_high->GetModifiedFee(), entry4_high->GetTxSize()), unused_txid).has_value());

// Tests for EntriesAndTxidsDisjoint
BOOST_CHECK(EntriesAndTxidsDisjoint(empty_set, {tx1->GetHash()}, unused_txid) == std::nullopt);
BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {tx3->GetHash()}, unused_txid) == std::nullopt);
// EntriesAndTxidsDisjoint uses txids, not wtxids.
BOOST_CHECK(EntriesAndTxidsDisjoint({entry2}, {tx2->GetWitnessHash()}, unused_txid) == std::nullopt);
BOOST_CHECK(EntriesAndTxidsDisjoint({entry2}, {tx2->GetHash()}, unused_txid).has_value());
BOOST_CHECK(EntriesAndTxidsDisjoint({entry2_normal}, {tx2->GetWitnessHash()}, unused_txid) == std::nullopt);
BOOST_CHECK(EntriesAndTxidsDisjoint({entry2_normal}, {tx2->GetHash()}, unused_txid).has_value());
BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {tx1->GetHash()}, unused_txid).has_value());
BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {tx2->GetHash()}, unused_txid).has_value());
// EntriesAndTxidsDisjoint does not calculate descendants of iters_conflicting; it uses whatever
// the caller passed in. As such, no error is returned even though entry2 is a descendant of tx1.
BOOST_CHECK(EntriesAndTxidsDisjoint({entry2}, {tx1->GetHash()}, unused_txid) == std::nullopt);
// the caller passed in. As such, no error is returned even though entry2_normal is a descendant of tx1.
BOOST_CHECK(EntriesAndTxidsDisjoint({entry2_normal}, {tx1->GetHash()}, unused_txid) == std::nullopt);

// Tests for PaysForRBF
const CFeeRate incremental_relay_feerate{DEFAULT_INCREMENTAL_RELAY_FEE};
Expand All @@ -165,8 +167,8 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup)
BOOST_CHECK(PaysForRBF(low_fee, high_fee + 99999999, 99999999, incremental_relay_feerate, unused_txid) == std::nullopt);

// Tests for GetEntriesForConflicts
CTxMemPool::setEntries all_parents{entry1, entry3, entry5, entry7, entry8};
CTxMemPool::setEntries all_children{entry2, entry4, entry6};
CTxMemPool::setEntries all_parents{entry1_normal, entry3_low, entry5_low, entry7_high, entry8_high};
CTxMemPool::setEntries all_children{entry2_normal, entry4_high, entry6_low_prioritised};
const std::vector<CTransactionRef> parent_inputs({m_coinbase_txns[0], m_coinbase_txns[1], m_coinbase_txns[2],
m_coinbase_txns[3], m_coinbase_txns[4]});
const auto conflicts_with_parents = make_tx(parent_inputs, {50 * CENT});
Expand Down Expand Up @@ -217,15 +219,69 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup)
BOOST_CHECK(HasNoNewUnconfirmed(/*tx=*/ *spends_unconfirmed.get(),
/*pool=*/ pool,
/*iters_conflicting=*/ all_entries) == std::nullopt);
BOOST_CHECK(HasNoNewUnconfirmed(*spends_unconfirmed.get(), pool, {entry2}) == std::nullopt);
BOOST_CHECK(HasNoNewUnconfirmed(*spends_unconfirmed.get(), pool, {entry2_normal}) == std::nullopt);
BOOST_CHECK(HasNoNewUnconfirmed(*spends_unconfirmed.get(), pool, empty_set).has_value());

const auto spends_new_unconfirmed = make_tx({tx1, tx8}, {36 * CENT});
BOOST_CHECK(HasNoNewUnconfirmed(*spends_new_unconfirmed.get(), pool, {entry2}).has_value());
BOOST_CHECK(HasNoNewUnconfirmed(*spends_new_unconfirmed.get(), pool, {entry2_normal}).has_value());
BOOST_CHECK(HasNoNewUnconfirmed(*spends_new_unconfirmed.get(), pool, all_entries).has_value());

const auto spends_conflicting_confirmed = make_tx({m_coinbase_txns[0], m_coinbase_txns[1]}, {45 * CENT});
BOOST_CHECK(HasNoNewUnconfirmed(*spends_conflicting_confirmed.get(), pool, {entry1, entry3}) == std::nullopt);
BOOST_CHECK(HasNoNewUnconfirmed(*spends_conflicting_confirmed.get(), pool, {entry1_normal, entry3_low}) == std::nullopt);

// Tests for CheckMinerScores
// Don't allow replacements with a low ancestor feerate.
BOOST_CHECK(CheckMinerScores(/*replacement_fees=*/entry1_normal->GetFee(),
/*replacement_vsize=*/entry1_normal->GetTxSize(),
/*ancestors=*/{entry5_low},
/*direct_conflicts=*/{entry1_normal},
/*original_transactions=*/set_12_normal).has_value());

BOOST_CHECK(CheckMinerScores(entry3_low->GetFee() + entry4_high->GetFee() + 10000,
entry3_low->GetTxSize() + entry4_high->GetTxSize(),
{entry5_low},
{entry3_low},
set_34_cpfp).has_value());

// These tests use modified fees (including prioritisation), not base fees.
BOOST_CHECK(CheckMinerScores(entry5_low->GetFee() + entry6_low_prioritised->GetFee() + 1,
entry5_low->GetTxSize() + entry6_low_prioritised->GetTxSize(),
{empty_set},
{entry5_low},
set_56_low).has_value());
BOOST_CHECK(CheckMinerScores(entry5_low->GetModifiedFee() + entry6_low_prioritised->GetModifiedFee() + 1,
entry5_low->GetTxSize() + entry6_low_prioritised->GetTxSize(),
{empty_set},
{entry5_low},
set_56_low) == std::nullopt);

// High-feerate ancestors don't help raise the replacement's miner score.
BOOST_CHECK(CheckMinerScores(entry1_normal->GetFee() - 1,
entry1_normal->GetTxSize(),
empty_set,
set_12_normal,
set_12_normal).has_value());

BOOST_CHECK(CheckMinerScores(entry1_normal->GetFee() - 1,
entry1_normal->GetTxSize(),
set_78_high,
set_12_normal,
set_12_normal).has_value());

// Replacement must be higher than the individual feerate of direct conflicts.
// Note entry4_high's individual feerate is higher than its ancestor feerate
BOOST_CHECK(CheckMinerScores(entry4_high->GetFee() - 1,
entry4_high->GetTxSize(),
empty_set,
{entry4_high},
{entry4_high}).has_value());

BOOST_CHECK(CheckMinerScores(entry4_high->GetFee() - 1,
entry4_high->GetTxSize(),
empty_set,
{entry3_low},
set_34_cpfp) == std::nullopt);

}

BOOST_AUTO_TEST_SUITE_END()
5 changes: 4 additions & 1 deletion src/test/transaction_tests.cpp
Expand Up @@ -794,7 +794,7 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
t.nVersion = 0;
CheckIsNotStandard(t, "version");

t.nVersion = 3;
t.nVersion = 4;
CheckIsNotStandard(t, "version");

// Allowed nVersion
Expand All @@ -804,6 +804,9 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
t.nVersion = 2;
CheckIsStandard(t);

t.nVersion = 3;
CheckIsStandard(t);

// Check dust with odd relay fee to verify rounding:
// nDustThreshold = 182 * 3702 / 1000
g_dust = CFeeRate(3702);
Expand Down
164 changes: 164 additions & 0 deletions src/test/txpackage_tests.cpp
Expand Up @@ -6,6 +6,7 @@
#include <key_io.h>
#include <policy/packages.h>
#include <policy/policy.h>
#include <policy/rbf.h>
#include <primitives/transaction.h>
#include <script/script.h>
#include <test/util/random.h>
Expand Down Expand Up @@ -854,4 +855,167 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
}
}

BOOST_FIXTURE_TEST_CASE(package_rbf_tests, TestChain100Setup)
{
mineBlocks(5);
LOCK(::cs_main);
size_t expected_pool_size = m_node.mempool->size();
CKey child_key;
child_key.MakeNewKey(true);
CScript parent_spk = GetScriptForDestination(WitnessV0KeyHash(child_key.GetPubKey()));
CKey grandchild_key;
grandchild_key.MakeNewKey(true);
CScript child_spk = GetScriptForDestination(WitnessV0KeyHash(grandchild_key.GetPubKey()));

const CAmount coinbase_value{50 * COIN};
// Test that de-duplication works and CheckMinerScores is not called when it's just 1 transaction.
{
// 1 parent paying 0sat, 1 child paying 300sat
Package package1;
// 1 parent paying 0sat, 1 child paying 500sat
Package package2;
// Package1 and package2 have the same parent. The children conflict.
auto mtx_parent = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[0], /*input_vout=*/0,
/*input_height=*/0, /*input_signing_key=*/coinbaseKey,
/*output_destination=*/parent_spk,
/*output_amount=*/coinbase_value, /*submit=*/false, /*version=*/3);
CTransactionRef tx_parent = MakeTransactionRef(mtx_parent);
package1.push_back(tx_parent);
package2.push_back(tx_parent);

CTransactionRef tx_child_1 = MakeTransactionRef(CreateValidMempoolTransaction(tx_parent, 0, 101, child_key, child_spk, coinbase_value - 300, false, 3));
package1.push_back(tx_child_1);
CTransactionRef tx_child_2 = MakeTransactionRef(CreateValidMempoolTransaction(tx_parent, 0, 101, child_key, child_spk, coinbase_value - 500, false, 3));
package2.push_back(tx_child_2);

LOCK(m_node.mempool->cs);
const auto submit1 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, /*test_accept=*/false);
BOOST_CHECK_MESSAGE(submit1.m_state.IsValid(), "Package validation unexpectedly failed" << submit1.m_state.GetRejectReason());
auto it_parent_1 = submit1.m_tx_results.find(tx_parent->GetWitnessHash());
auto it_child_1 = submit1.m_tx_results.find(tx_child_1->GetWitnessHash());
BOOST_CHECK(it_parent_1 != submit1.m_tx_results.end());
BOOST_CHECK(it_child_1 != submit1.m_tx_results.end());
BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
expected_pool_size += 2;
BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent->GetHash())));
BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_1->GetHash())));
const auto entry_parent = m_node.mempool->GetIter(tx_parent->GetHash()).value();
const auto entry_child1 = m_node.mempool->GetIter(tx_child_1->GetHash()).value();

// Second time around, parent should be deduplicated and child2 considered by itself.
// CheckMinerScores would fail if called with child2. But it would be unfair to compare
// child1's individual feerate with child2's ancestor feerate.
BOOST_CHECK(CheckMinerScores(/*replacement_fees=*/500,
/*replacement_vsize=*/GetVirtualTransactionSize(*tx_child_2),
/*ancestors=*/{entry_parent},
/*direct_conflicts=*/{entry_child1},
/*original_transactions=*/{entry_child1}).has_value());
const auto submit2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package2, /*test_accept=*/false);
auto it_parent_2 = submit2.m_tx_results.find(tx_parent->GetWitnessHash());
auto it_child_2 = submit2.m_tx_results.find(tx_child_2->GetWitnessHash());
BOOST_CHECK(it_parent_2 != submit2.m_tx_results.end());
BOOST_CHECK(it_child_2 != submit2.m_tx_results.end());
BOOST_CHECK_MESSAGE(submit2.m_state.IsValid(), "Package validation unexpectedly failed" << submit2.m_state.GetRejectReason());
BOOST_CHECK_EQUAL(it_parent_2->second.m_result_type, MempoolAcceptResult::ResultType::MEMPOOL_ENTRY);
BOOST_CHECK_EQUAL(it_child_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_2->GetHash())));
BOOST_CHECK(!m_node.mempool->exists(GenTxid::Txid(tx_child_1->GetHash())));
}

// Test that V3 is required for package RBF, and not required for regular RBF.
{
CTransactionRef tx_parent_1 = MakeTransactionRef(CreateValidMempoolTransaction(
m_coinbase_txns[1], /*input_vout=*/0, /*input_height=*/0,
coinbaseKey, parent_spk, coinbase_value - 200, /*submit=*/false, /*version=*/2));
CTransactionRef tx_child_1 = MakeTransactionRef(CreateValidMempoolTransaction(
tx_parent_1, /*input_vout=*/0, /*input_height=*/101,
child_key, child_spk, coinbase_value - 400, /*submit=*/false, /*version=*/2));

CTransactionRef tx_parent_2 = MakeTransactionRef(CreateValidMempoolTransaction(
m_coinbase_txns[1], /*input_vout=*/0, /*input_height=*/0,
coinbaseKey, parent_spk, coinbase_value - 800, /*submit=*/false, /*version=*/2));
CTransactionRef tx_child_2 = MakeTransactionRef(CreateValidMempoolTransaction(
tx_parent_2, /*input_vout=*/0, /*input_height=*/101,
child_key, child_spk, coinbase_value - 800 - 200, /*submit=*/false, /*version=*/2));

CTransactionRef tx_parent_3 = MakeTransactionRef(CreateValidMempoolTransaction(
m_coinbase_txns[1], /*input_vout=*/0, /*input_height=*/0,
coinbaseKey, parent_spk, coinbase_value - 200, /*submit=*/false, /*version=*/2));
CTransactionRef tx_child_3 = MakeTransactionRef(CreateValidMempoolTransaction(
tx_parent_3, /*input_vout=*/0, /*input_height=*/101,
child_key, child_spk, coinbase_value - 200 - 1300, /*submit=*/false, /*version=*/2));

CTransactionRef tx_parent_4 = MakeTransactionRef(CreateValidMempoolTransaction(
m_coinbase_txns[1], /*input_vout=*/0, /*input_height=*/0,
coinbaseKey, parent_spk, coinbase_value - 200, /*submit=*/false, /*version=*/3));
CTransactionRef tx_child_4 = MakeTransactionRef(CreateValidMempoolTransaction(
tx_parent_4, /*input_vout=*/0, /*input_height=*/101,
child_key, child_spk, coinbase_value - 200 - 1500, /*submit=*/false, /*version=*/3));

// 1 parent paying 200sat, 1 child paying 200sat. Both v2.
Package package1{tx_parent_1, tx_child_1};
// 1 parent paying 800sat, 1 child paying 200sat. Both v2.
Package package2{tx_parent_2, tx_child_2};
// 1 parent paying 200sat, 1 child paying 1300. Both v2.
Package package3{tx_parent_3, tx_child_3};
// 1 parent paying 200sat, 1 child paying 1300. Same as package3, but v3.
Package package4{tx_parent_4, tx_child_4};
// In all packages, the parents conflict with each other

const auto submit1 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, false);
BOOST_CHECK_MESSAGE(submit1.m_state.IsValid(), "Package validation unexpectedly failed" << submit1.m_state.GetRejectReason());
auto it_parent_1 = submit1.m_tx_results.find(tx_parent_1->GetWitnessHash());
auto it_child_1 = submit1.m_tx_results.find(tx_child_1->GetWitnessHash());
BOOST_CHECK(it_parent_1 != submit1.m_tx_results.end());
BOOST_CHECK(it_child_1 != submit1.m_tx_results.end());
BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
expected_pool_size += 2;
BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent_1->GetHash())));
BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_1->GetHash())));

// These transactions do not need to be V3 to replace their conflicts within
// ProcessNewPackage() because the fees used in RBF are their own; no package RBF is
// actually happening.
const auto submit2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package2, false);
BOOST_CHECK_MESSAGE(submit2.m_state.IsValid(), "Package validation unexpectedly failed" << submit2.m_state.GetRejectReason());
auto it_parent_2 = submit2.m_tx_results.find(tx_parent_2->GetWitnessHash());
auto it_child_2 = submit2.m_tx_results.find(tx_child_2->GetWitnessHash());
BOOST_CHECK(it_parent_2 != submit2.m_tx_results.end());
BOOST_CHECK(it_child_2 != submit2.m_tx_results.end());
BOOST_CHECK_EQUAL(it_parent_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
BOOST_CHECK_EQUAL(it_child_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent_2->GetHash())));
BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_2->GetHash())));

// Package RBF, in which the replacement transaction's child sponsors the fees to meet RBF
// rules, requires the replacement transactions to be V3.
const auto submit3 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package3, false);
BOOST_CHECK(submit3.m_state.IsInvalid());
BOOST_CHECK_EQUAL(submit3.m_state.GetRejectReason(), "transaction failed");
BOOST_CHECK(submit3.m_tx_results.at(tx_parent_3->GetWitnessHash()).m_state.IsInvalid());
BOOST_CHECK_EQUAL(submit3.m_tx_results.at(tx_parent_3->GetWitnessHash()).m_state.GetResult(), TxValidationResult::TX_MEMPOOL_POLICY);
BOOST_CHECK(submit3.m_tx_results.at(tx_child_3->GetWitnessHash()).m_state.IsInvalid());
BOOST_CHECK_EQUAL(submit3.m_tx_results.at(tx_child_3->GetWitnessHash()).m_state.GetResult(), TxValidationResult::TX_MISSING_INPUTS);

const auto submit4 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package4, false);
BOOST_CHECK_MESSAGE(submit4.m_state.IsValid(), "Package validation unexpectedly failed" << submit4.m_state.GetRejectReason());
auto it_parent_4 = submit4.m_tx_results.find(tx_parent_4->GetWitnessHash());
auto it_child_4 = submit4.m_tx_results.find(tx_child_4->GetWitnessHash());
BOOST_CHECK(it_parent_4 != submit4.m_tx_results.end());
BOOST_CHECK(it_child_4 != submit4.m_tx_results.end());
BOOST_CHECK_EQUAL(it_parent_4->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
BOOST_CHECK_EQUAL(it_child_4->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent_4->GetHash())));
BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child_4->GetHash())));
}

}
BOOST_AUTO_TEST_SUITE_END()
153 changes: 153 additions & 0 deletions src/test/txvalidation_tests.cpp
Expand Up @@ -4,11 +4,14 @@

#include <consensus/validation.h>
#include <key_io.h>
#include <policy/v3_policy.h>
#include <policy/packages.h>
#include <policy/policy.h>
#include <primitives/transaction.h>
#include <random.h>
#include <script/script.h>
#include <test/util/setup_common.h>
#include <test/util/txmempool.h>
#include <validation.h>

#include <boost/test/unit_test.hpp>
Expand Down Expand Up @@ -48,4 +51,154 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup)
BOOST_CHECK_EQUAL(result.m_state.GetRejectReason(), "coinbase");
BOOST_CHECK(result.m_state.GetResult() == TxValidationResult::TX_CONSENSUS);
}

// Generate a number of random, nonexistent outpoints.
static inline std::vector<COutPoint> random_outpoints(size_t num_outpoints) {
std::vector<COutPoint> outpoints;
outpoints.resize(num_outpoints);
for (size_t i{0}; i < num_outpoints; ++i) {
outpoints.emplace_back(GetRandHash(), 0);
}
return outpoints;
}

// Creates a placeholder tx (not valid) with 25 outputs. Specify the nVersion and the inputs.
static inline CTransactionRef make_tx(const std::vector<COutPoint>& inputs, int32_t version)
{
CMutableTransaction mtx = CMutableTransaction{};
mtx.nVersion = version;
mtx.vin.resize(inputs.size());
mtx.vout.resize(25);
for (size_t i{0}; i < inputs.size(); ++i) {
mtx.vin[i].prevout = inputs[i];
}
for (auto i{0}; i < 25; ++i) {
mtx.vout[i].scriptPubKey = CScript() << OP_TRUE;
mtx.vout[i].nValue = 10000;
}
return MakeTransactionRef(mtx);
}

BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup)
{
// Test V3 policy helper functions
CTxMemPool& pool = *Assert(m_node.mempool);
LOCK2(cs_main, pool.cs);
TestMemPoolEntryHelper entry;
std::set<uint256> empty_conflicts_set;

auto mempool_tx_v3 = make_tx(random_outpoints(1), /*version=*/3);
pool.addUnchecked(entry.FromTx(mempool_tx_v3));
auto mempool_tx_v2 = make_tx(random_outpoints(1), /*version=*/2);
pool.addUnchecked(entry.FromTx(mempool_tx_v2));
// These two transactions are unrelated, so CheckV3Inheritance should pass.
BOOST_CHECK(CheckV3Inheritance({mempool_tx_v2, mempool_tx_v3}) == std::nullopt);
// Default values.
CTxMemPool::Limits m_limits{};

// Cannot spend from an unconfirmed v3 transaction unless this tx is also v3.
{
auto tx_v2_from_v3 = make_tx({COutPoint{mempool_tx_v3->GetHash(), 0}}, /*version=*/2);
auto ancestors_v2_from_v3{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v2_from_v3), m_limits)};
BOOST_CHECK(CheckV3Inheritance(tx_v2_from_v3, *ancestors_v2_from_v3).has_value());
BOOST_CHECK(CheckV3Inheritance({mempool_tx_v3, tx_v2_from_v3}).has_value());
auto tx_v2_from_v2_and_v3 = make_tx({COutPoint{mempool_tx_v3->GetHash(), 0}, COutPoint{mempool_tx_v2->GetHash(), 0}}, /*version=*/2);
auto ancestors_v2_from_both{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v2_from_v2_and_v3), m_limits)};
BOOST_CHECK(CheckV3Inheritance(tx_v2_from_v2_and_v3, *ancestors_v2_from_both).has_value());
BOOST_CHECK(CheckV3Inheritance({mempool_tx_v2, mempool_tx_v3, tx_v2_from_v2_and_v3}).value() ==
std::make_tuple(mempool_tx_v3->GetWitnessHash(), tx_v2_from_v2_and_v3->GetWitnessHash(), false));
}

// V3 cannot spend from an unconfirmed non-v3 transaction.
{
auto tx_v3_from_v2 = make_tx({COutPoint{mempool_tx_v2->GetHash(), 0}}, /*version=*/3);
auto ancestors_v3_from_v2{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_from_v2), m_limits)};
BOOST_CHECK(CheckV3Inheritance(tx_v3_from_v2, *ancestors_v3_from_v2).has_value());
BOOST_CHECK(CheckV3Inheritance({mempool_tx_v2, tx_v3_from_v2}).value() ==
std::make_tuple(mempool_tx_v2->GetWitnessHash(), tx_v3_from_v2->GetWitnessHash(), true));
auto tx_v3_from_v2_and_v3 = make_tx({COutPoint{mempool_tx_v3->GetHash(), 0}, COutPoint{mempool_tx_v2->GetHash(), 0}}, /*version=*/3);
auto ancestors_v3_from_both{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_from_v2_and_v3), m_limits)};
BOOST_CHECK(CheckV3Inheritance(tx_v3_from_v2_and_v3, *ancestors_v3_from_both).has_value());
BOOST_CHECK(CheckV3Inheritance({mempool_tx_v2, mempool_tx_v3, tx_v3_from_v2_and_v3}).value() ==
std::make_tuple(mempool_tx_v2->GetWitnessHash(), tx_v3_from_v2_and_v3->GetWitnessHash(), true));
}
// V3 from V3 is ok, and non-V3 from non-V3 is ok.
{
auto tx_v3_from_v3 = make_tx({COutPoint{mempool_tx_v3->GetHash(), 0}}, /*version=*/3);
auto ancestors_v3{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_from_v3), m_limits)};
BOOST_CHECK(CheckV3Inheritance({tx_v3_from_v3, mempool_tx_v3}) == std::nullopt);
BOOST_CHECK(CheckV3Inheritance({mempool_tx_v3, tx_v3_from_v3}) == std::nullopt);
BOOST_CHECK(CheckV3Inheritance(tx_v3_from_v3, *ancestors_v3) == std::nullopt);

auto tx_v2_from_v2 = make_tx({COutPoint{mempool_tx_v2->GetHash(), 0}}, /*version=*/2);
auto ancestors_v2{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v2_from_v2), m_limits)};
BOOST_CHECK(CheckV3Inheritance({tx_v2_from_v2, mempool_tx_v2}) == std::nullopt);
BOOST_CHECK(CheckV3Inheritance({mempool_tx_v2, tx_v2_from_v2}) == std::nullopt);
BOOST_CHECK(CheckV3Inheritance(tx_v2_from_v2, *ancestors_v2) == std::nullopt);
}

// Tx spending v3 cannot have too many mempool ancestors
// Configuration where the tx has multiple direct parents.
{
std::vector<COutPoint> mempool_outpoints;
mempool_outpoints.emplace_back(mempool_tx_v3->GetHash(), 0);
mempool_outpoints.resize(2);
for (size_t i{0}; i < 2; ++i) {
auto mempool_tx = make_tx(random_outpoints(1), /*version=*/2);
pool.addUnchecked(entry.FromTx(mempool_tx));
mempool_outpoints.emplace_back(mempool_tx->GetHash(), 0);
}
auto tx_v3_multi_parent = make_tx(mempool_outpoints, /*version=*/3);
auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_multi_parent), m_limits)};
BOOST_CHECK(ApplyV3Rules(tx_v3_multi_parent, *ancestors, empty_conflicts_set).has_value());
}

// Configuration where the tx is in a multi-generation chain.
auto last_outpoint{random_outpoints(1)[0]};
for (size_t i{0}; i < 2; ++i) {
auto mempool_tx = make_tx({last_outpoint}, /*version=*/2);
pool.addUnchecked(entry.FromTx(mempool_tx));
last_outpoint = COutPoint{mempool_tx->GetHash(), 0};
}
{
auto tx_v3_multi_gen = make_tx({last_outpoint}, /*version=*/3);
auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_multi_gen), m_limits)};
BOOST_CHECK(ApplyV3Rules(tx_v3_multi_gen, *ancestors, empty_conflicts_set).has_value());
}

// Tx spending v3 cannot be too large
auto many_inputs{random_outpoints(100)};
many_inputs.emplace_back(mempool_tx_v3->GetHash(), 0);
{
auto tx_v3_child_big = make_tx(many_inputs, /*version=*/3);
BOOST_CHECK(GetTransactionWeight(*tx_v3_child_big) > V3_CHILD_MAX_WEIGHT);
auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_child_big), m_limits)};
BOOST_CHECK(ApplyV3Rules(tx_v3_child_big, *ancestors, empty_conflicts_set).has_value());
}

// Parent + child with v3 in the mempool. Child is allowed as long as it is under V3_CHILD_MAX_SIZE.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Parent + child with v3 in the mempool. Child is allowed as long as it is under V3_CHILD_MAX_SIZE.
// Parent + child with v3 in the mempool. Child is allowed as long as it is under V3_CHILD_MAX_WEIGHT.

auto tx_mempool_v3_child = make_tx({COutPoint{mempool_tx_v3->GetHash(), 0}}, /*version=*/3);
{
BOOST_CHECK(GetTransactionWeight(*tx_mempool_v3_child) <= V3_CHILD_MAX_WEIGHT);
auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_mempool_v3_child), m_limits)};
BOOST_CHECK(ApplyV3Rules(tx_mempool_v3_child, *ancestors, empty_conflicts_set) == std::nullopt);
pool.addUnchecked(entry.FromTx(tx_mempool_v3_child));
}

// A v3 transaction cannot have more than 1 descendant.
{
auto tx_v3_child2 = make_tx({COutPoint{mempool_tx_v3->GetHash(), 1}}, /*version=*/3);
auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_child2), m_limits)};
BOOST_CHECK(ApplyV3Rules(tx_v3_child2, *ancestors, empty_conflicts_set).has_value());
// If replacing the child, make sure there is no double-counting.
BOOST_CHECK(ApplyV3Rules(tx_v3_child2, *ancestors, {tx_mempool_v3_child->GetHash()}) == std::nullopt);
}

{
auto tx_v3_grandchild = make_tx({COutPoint{tx_mempool_v3_child->GetHash(), 0}}, /*version=*/3);
auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_grandchild), m_limits)};
BOOST_CHECK(ApplyV3Rules(tx_v3_grandchild, *ancestors, empty_conflicts_set).has_value());
}
}

BOOST_AUTO_TEST_SUITE_END()
15 changes: 10 additions & 5 deletions src/test/util/setup_common.cpp
Expand Up @@ -345,9 +345,11 @@ std::pair<CMutableTransaction, CAmount> TestChain100Setup::CreateValidTransactio
const std::vector<CKey>& input_signing_keys,
const std::vector<CTxOut>& outputs,
const std::optional<CFeeRate>& feerate,
const std::optional<uint32_t>& fee_output)
const std::optional<uint32_t>& fee_output,
uint32_t version)
{
CMutableTransaction mempool_txn;
mempool_txn.nVersion = version;
mempool_txn.vin.reserve(inputs.size());
mempool_txn.vout.reserve(outputs.size());

Expand Down Expand Up @@ -409,9 +411,10 @@ CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(const std::
int input_height,
const std::vector<CKey>& input_signing_keys,
const std::vector<CTxOut>& outputs,
bool submit)
bool submit,
uint32_t version)
{
CMutableTransaction mempool_txn = CreateValidTransaction(input_transactions, inputs, input_height, input_signing_keys, outputs, std::nullopt, std::nullopt).first;
CMutableTransaction mempool_txn = CreateValidTransaction(input_transactions, inputs, input_height, input_signing_keys, outputs, std::nullopt, std::nullopt, version).first;
// If submit=true, add transaction to the mempool.
if (submit) {
LOCK(cs_main);
Expand All @@ -427,7 +430,8 @@ CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactio
CKey input_signing_key,
CScript output_destination,
CAmount output_amount,
bool submit)
bool submit,
uint32_t version)
{
COutPoint input{input_transaction->GetHash(), input_vout};
CTxOut output{output_amount, output_destination};
Expand All @@ -436,7 +440,8 @@ CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactio
/*input_height=*/input_height,
/*input_signing_keys=*/{input_signing_key},
/*outputs=*/{output},
/*submit=*/submit);
/*submit=*/submit,
/*version=*/version);
}

std::vector<CTransactionRef> TestChain100Setup::PopulateMempool(FastRandomContext& det_rand, size_t num_transactions, bool submit)
Expand Down
9 changes: 6 additions & 3 deletions src/test/util/setup_common.h
Expand Up @@ -142,7 +142,8 @@ struct TestChain100Setup : public TestingSetup {
const std::vector<CKey>& input_signing_keys,
const std::vector<CTxOut>& outputs,
const std::optional<CFeeRate>& feerate,
const std::optional<uint32_t>& fee_output);
const std::optional<uint32_t>& fee_output,
uint32_t version);
/**
* Create a transaction and, optionally, submit to the mempool.
*
Expand All @@ -158,7 +159,8 @@ struct TestChain100Setup : public TestingSetup {
int input_height,
const std::vector<CKey>& input_signing_keys,
const std::vector<CTxOut>& outputs,
bool submit = true);
bool submit = true,
uint32_t version = 2);

/**
* Create a 1-in-1-out transaction and, optionally, submit to the mempool.
Expand All @@ -177,7 +179,8 @@ struct TestChain100Setup : public TestingSetup {
CKey input_signing_key,
CScript output_destination,
CAmount output_amount = CAmount(1 * COIN),
bool submit = true);
bool submit = true,
uint32_t version = 2);

/** Create transactions spending from m_coinbase_txns. These transactions will only spend coins
* that exist in the current chain, but may be premature coinbase spends, have missing
Expand Down
15 changes: 11 additions & 4 deletions src/txmempool.cpp
Expand Up @@ -1135,17 +1135,24 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector<COutPoint>* pvNoSpends

unsigned nTxnRemoved = 0;
CFeeRate maxFeeRateRemoved(0);
while (!mapTx.empty() && DynamicMemoryUsage() > sizelimit) {
while (!mapTx.empty()) {
indexed_transaction_set::index<descendant_score>::type::iterator it = mapTx.get<descendant_score>().begin();

// Unless min relay feerate is 0, skim away everything paying literally zero fees, regardless of mempool size.
// Above that feerate, just trim until memory is within limits.
if ((it->GetModFeesWithDescendants() > 0 || m_min_relay_feerate.GetFeePerK() == 0) &&
DynamicMemoryUsage() <= sizelimit) break;

// We set the new mempool min fee to the feerate of the removed set, plus the
// "minimum reasonable fee rate" (ie some value under which we consider txn
// to have 0 fee). This way, we don't allow txn to enter mempool with feerate
// equal to txn which were removed with no block in between.
CFeeRate removed(it->GetModFeesWithDescendants(), it->GetSizeWithDescendants());
removed += m_incremental_relay_feerate;
trackPackageRemoved(removed);
maxFeeRateRemoved = std::max(maxFeeRateRemoved, removed);
if (removed >= m_min_relay_feerate) {
removed += m_incremental_relay_feerate;
trackPackageRemoved(removed);
maxFeeRateRemoved = std::max(maxFeeRateRemoved, removed);
}

setEntries stage;
CalculateDescendants(mapTx.project<0>(it), stage);
Expand Down
262 changes: 197 additions & 65 deletions src/validation.cpp

Large diffs are not rendered by default.

11 changes: 6 additions & 5 deletions test/functional/feature_bip68_sequence.py
Expand Up @@ -266,22 +266,23 @@ def test_nonzero_locks(orig_tx, node, relayfee, use_height_lock):
test_nonzero_locks(tx2, self.nodes[0], self.relayfee, use_height_lock=False)

# Now mine some blocks, but make sure tx2 doesn't get mined.
# Use prioritisetransaction to lower the effective feerate to 0
# Use prioritisetransaction to lower the effective feerate to 0, removing it from mempool.
self.nodes[0].prioritisetransaction(txid=tx2.hash, fee_delta=int(-self.relayfee*COIN))
self.wallet.send_self_transfer(from_node=self.nodes[0])
cur_time = int(time.time())
for _ in range(10):
self.nodes[0].setmocktime(cur_time + 600)
self.generate(self.wallet, 1, sync_fun=self.no_op)
cur_time += 600

assert tx2.hash in self.nodes[0].getrawmempool()
assert tx2.hash not in self.nodes[0].getrawmempool()

# Resubmit and mine tx2, and then try again
self.nodes[0].prioritisetransaction(txid=tx2.hash, fee_delta=int(self.relayfee*COIN))
self.nodes[0].sendrawtransaction(tx2.serialize().hex())
test_nonzero_locks(tx2, self.nodes[0], self.relayfee, use_height_lock=True)
test_nonzero_locks(tx2, self.nodes[0], self.relayfee, use_height_lock=False)

# Mine tx2, and then try again
self.nodes[0].prioritisetransaction(txid=tx2.hash, fee_delta=int(self.relayfee*COIN))

# Advance the time on the node so that we can test timelocks
self.nodes[0].setmocktime(cur_time+600)
# Save block template now to use for the reorg later
Expand Down
2 changes: 1 addition & 1 deletion test/functional/mempool_accept.py
Expand Up @@ -271,7 +271,7 @@ def run_test(self):

self.log.info('Some nonstandard transactions')
tx = tx_from_hex(raw_tx_reference)
tx.nVersion = 3 # A version currently non-standard
tx.nVersion = 4 # A version currently non-standard
self.check_mempool_result(
result_expected=[{'txid': tx.rehash(), 'allowed': False, 'reject-reason': 'version'}],
rawtxs=[tx.serialize().hex()],
Expand Down
246 changes: 246 additions & 0 deletions test/functional/mempool_accept_v3.py
@@ -0,0 +1,246 @@
#!/usr/bin/env python3
# Copyright (c) 2021 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.

from test_framework.messages import (
MAX_BIP125_RBF_SEQUENCE,
)
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_equal,
assert_greater_than,
assert_greater_than_or_equal,
assert_raises_rpc_error,
)
from test_framework.wallet import (
DEFAULT_FEE,
MiniWallet,
)
def cleanup(func):
def wrapper(self):
try:
func(self)
finally:
# Clear mempool
self.generate(self.nodes[0], 1)
# Reset config options
self.restart_node(0)
return wrapper

class MempoolAcceptV3(BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 1
self.setup_clean_chain = True

def check_mempool(self, txids):
"""Assert exact contents of the node's mempool (by txid)."""
mempool_contents = self.nodes[0].getrawmempool()
assert_equal(len(txids), len(mempool_contents))
assert all([txid in txids for txid in mempool_contents])

@cleanup
def test_v3_acceptance(self):
node = self.nodes[0]
self.log.info("Test a child of a V3 transaction cannot be more than 1000vB")
self.restart_node(0, extra_args=["-datacarriersize=1000"])
tx_v3_parent_normal = self.wallet.send_self_transfer(from_node=node, version=3)
self.check_mempool([tx_v3_parent_normal["txid"]])
tx_v3_child_heavy = self.wallet.create_self_transfer(
utxo_to_spend=tx_v3_parent_normal["new_utxo"],
target_weight=4004,
version=3
)
assert_greater_than_or_equal(tx_v3_child_heavy["tx"].get_vsize(), 1000)
assert_raises_rpc_error(-26, "v3-tx-nonstandard, v3 child tx is too big", node.sendrawtransaction, tx_v3_child_heavy["hex"])
self.check_mempool([tx_v3_parent_normal["txid"]])
# tx has no descendants
assert_equal(node.getmempoolentry(tx_v3_parent_normal["txid"])["descendantcount"], 1)

self.log.info("Test that, during replacements, only the new transaction counts for V3 descendant limit")
tx_v3_child_almost_heavy = self.wallet.send_self_transfer(
from_node=node,
fee_rate=DEFAULT_FEE,
utxo_to_spend=tx_v3_parent_normal["new_utxo"],
target_weight=3987,
version=3
)
assert_greater_than_or_equal(1000, tx_v3_child_almost_heavy["tx"].get_vsize())
self.check_mempool([tx_v3_parent_normal["txid"], tx_v3_child_almost_heavy["txid"]])
assert_equal(node.getmempoolentry(tx_v3_parent_normal["txid"])["descendantcount"], 2)
tx_v3_child_almost_heavy_rbf = self.wallet.send_self_transfer(
from_node=node,
fee_rate=DEFAULT_FEE * 2,
utxo_to_spend=tx_v3_parent_normal["new_utxo"],
target_weight=3500,
version=3
)
assert_greater_than_or_equal(tx_v3_child_almost_heavy["tx"].get_vsize() + tx_v3_child_almost_heavy_rbf["tx"].get_vsize(), 1000)
self.check_mempool([tx_v3_parent_normal["txid"], tx_v3_child_almost_heavy_rbf["txid"]])
assert_equal(node.getmempoolentry(tx_v3_parent_normal["txid"])["descendantcount"], 2)

@cleanup
def test_v3_replacement(self):
node = self.nodes[0]
self.log.info("Test V3 transactions may be replaced by V3 transactions")
glozow marked this conversation as resolved.
Show resolved Hide resolved
utxo_v3_bip125 = self.wallet.get_utxo()
tx_v3_bip125 = self.wallet.send_self_transfer(
from_node=node,
fee_rate=DEFAULT_FEE,
utxo_to_spend=utxo_v3_bip125,
sequence=MAX_BIP125_RBF_SEQUENCE,
version=3
)
self.check_mempool([tx_v3_bip125["txid"]])

tx_v3_bip125_rbf = self.wallet.send_self_transfer(
from_node=node,
fee_rate=DEFAULT_FEE * 2,
utxo_to_spend=utxo_v3_bip125,
version=3
)
self.check_mempool([tx_v3_bip125_rbf["txid"]])

self.log.info("Test V3 transactions may be replaced by V2 transactions")
tx_v3_bip125_rbf_v2 = self.wallet.send_self_transfer(
from_node=node,
fee_rate=DEFAULT_FEE * 3,
utxo_to_spend=utxo_v3_bip125,
version=2
)
self.check_mempool([tx_v3_bip125_rbf_v2["txid"]])

self.log.info("Test that replacements cannot cause violation of inherited V3")
utxo_v3_parent = self.wallet.get_utxo()
tx_v3_parent = self.wallet.send_self_transfer(
from_node=node,
fee_rate=DEFAULT_FEE,
utxo_to_spend=utxo_v3_parent,
version=3
)
tx_v3_child = self.wallet.send_self_transfer(
from_node=node,
fee_rate=DEFAULT_FEE,
utxo_to_spend=tx_v3_parent["new_utxo"],
version=3
)
self.check_mempool([tx_v3_bip125_rbf_v2["txid"], tx_v3_parent["txid"], tx_v3_child["txid"]])

tx_v3_child_rbf_v2 = self.wallet.create_self_transfer(
fee_rate=DEFAULT_FEE * 2,
utxo_to_spend=tx_v3_parent["new_utxo"],
version=2
)
assert_raises_rpc_error(-26, "non-v3-tx-spends-v3", node.sendrawtransaction, tx_v3_child_rbf_v2["hex"])
self.check_mempool([tx_v3_bip125_rbf_v2["txid"], tx_v3_parent["txid"], tx_v3_child["txid"]])


@cleanup
def test_v3_bip125(self):
node = self.nodes[0]
self.log.info("Test V3 transactions that don't signal BIP125 are replaceable")
glozow marked this conversation as resolved.
Show resolved Hide resolved
assert_equal(node.getmempoolinfo()["fullrbf"], False)
utxo_v3_no_bip125 = self.wallet.get_utxo()
tx_v3_no_bip125 = self.wallet.send_self_transfer(
from_node=node,
fee_rate=DEFAULT_FEE,
utxo_to_spend=utxo_v3_no_bip125,
sequence=MAX_BIP125_RBF_SEQUENCE + 1,
version=3
)

self.check_mempool([tx_v3_no_bip125["txid"]])
assert not node.getmempoolentry(tx_v3_no_bip125["txid"])["bip125-replaceable"]
tx_v3_no_bip125_rbf = self.wallet.send_self_transfer(
from_node=node,
fee_rate=DEFAULT_FEE * 2,
utxo_to_spend=utxo_v3_no_bip125,
version=3
)
self.check_mempool([tx_v3_no_bip125_rbf["txid"]])

@cleanup
def test_v3_reorg(self):
node = self.nodes[0]
self.restart_node(0, extra_args=["-datacarriersize=40000"])
self.log.info("Test that, during a reorg, transactions that now violate v3 rules are evicted")
tx_v2_block = self.wallet.send_self_transfer(from_node=node, version=2)
tx_v3_block = self.wallet.send_self_transfer(from_node=node, version=3)
tx_v3_block2 = self.wallet.send_self_transfer(from_node=node, version=3)
assert_equal(set(node.getrawmempool()), set([tx_v3_block["txid"], tx_v2_block["txid"], tx_v3_block2["txid"]]))

block = self.generate(node, 1)
assert_equal(node.getrawmempool(), [])
tx_v2_from_v3 = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=tx_v3_block["new_utxo"], version=2)
tx_v3_from_v2 = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=tx_v2_block["new_utxo"], version=3)
tx_v3_child_large = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=tx_v3_block2["new_utxo"], target_weight=5000, version=3)
assert_greater_than(node.getmempoolentry(tx_v3_child_large["txid"])["vsize"], 1000)
assert_equal(set(node.getrawmempool()), set([tx_v2_from_v3["txid"], tx_v3_from_v2["txid"], tx_v3_child_large["txid"]]))
node.invalidateblock(block[0])
assert_equal(set(node.getrawmempool()), set([tx_v3_block["txid"], tx_v2_block["txid"], tx_v3_block2["txid"]]))
# This is needed because generate() will create the exact same block again.
node.reconsiderblock(block[0])


@cleanup
def test_nondefault_package_limits(self):
"""
Max standard tx size + V3 rules imply the ancestor/descendant rules (at their default
values), but those checks must not be skipped. Ensure both sets of checks are done by
changing the ancestor/descendant limit configurations.
"""
node = self.nodes[0]
self.log.info("Test that a decreased limitdescendantsize also applies to V3 child")
self.restart_node(0, extra_args=["-limitdescendantsize=10", "-datacarriersize=40000"])
tx_v3_parent_large1 = self.wallet.send_self_transfer(from_node=node, target_weight=99900, version=3)
tx_v3_child_large1 = self.wallet.create_self_transfer(utxo_to_spend=tx_v3_parent_large1["new_utxo"], version=3)
# Child is within V3 limits
assert_greater_than(1000, tx_v3_child_large1["tx"].get_vsize())
assert_raises_rpc_error(-26, "too-long-mempool-chain", node.sendrawtransaction,
tx_v3_child_large1["hex"])
self.check_mempool([tx_v3_parent_large1["txid"]])
assert_equal(node.getmempoolentry(tx_v3_parent_large1["txid"])["descendantcount"], 1)
self.generate(node, 1)

self.log.info("Test that a decreased limitancestorsize also applies to V3 parent")
self.restart_node(0, extra_args=["-limitancestorsize=10", "-datacarriersize=40000"])
tx_v3_parent_large2 = self.wallet.send_self_transfer(from_node=node, target_weight=99900, version=3)
tx_v3_child_large2 = self.wallet.create_self_transfer(utxo_to_spend=tx_v3_parent_large2["new_utxo"], version=3)
# Child is within V3 limits
assert_greater_than_or_equal(1000, tx_v3_child_large2["tx"].get_vsize())
assert_raises_rpc_error(-26, "too-long-mempool-chain", node.sendrawtransaction,
tx_v3_child_large2["hex"])
self.check_mempool([tx_v3_parent_large2["txid"]])

@cleanup
def test_fee_dependency_replacements(self):
"""
Since v3 introduces the possibility of 0-fee (i.e. below min relay feerate) transactions in
the mempool, it's possible for these transactions' sponsors to disappear due to RBF. In
those situations, the 0-fee transaction must be evicted along with the replacements.
"""
node = self.nodes[0]
self.log.info("Test that below-min-relay-feerate transactions are removed in RBF")
tx_0fee_parent = self.wallet.create_self_transfer(fee=0, fee_rate=0, version=3)
utxo_confirmed = self.wallet.get_utxo()
tx_child_replacee = self.wallet.create_self_transfer_multi(utxos_to_spend=[tx_0fee_parent["new_utxo"], utxo_confirmed], version=3)
node.submitpackage([tx_0fee_parent["hex"], tx_child_replacee["hex"]])
self.check_mempool([tx_0fee_parent["txid"], tx_child_replacee["txid"]])
tx_replacer = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=utxo_confirmed, fee_rate=DEFAULT_FEE * 10)
self.check_mempool([tx_replacer["txid"]])

def run_test(self):
self.log.info("Generate blocks to create UTXOs")
node = self.nodes[0]
self.wallet = MiniWallet(node)
self.generate(self.wallet, 110)
self.test_v3_acceptance()
self.test_v3_replacement()
self.test_v3_bip125()
self.test_v3_reorg()
self.test_nondefault_package_limits()
self.test_fee_dependency_replacements()


if __name__ == "__main__":
MempoolAcceptV3().main()
12 changes: 9 additions & 3 deletions test/functional/mempool_package_onemore.py
Expand Up @@ -55,14 +55,20 @@ def run_test(self):
assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[0], second_chain])
# ...especially if its > 40k weight
assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[0]], num_outputs=350)
# ...not if it's submitted with other transactions
replacable_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=[chain[0]])
txns = [replacable_tx["hex"], self.wallet.create_self_transfer_multi(utxos_to_spend=replacable_tx["new_utxos"])["hex"]]
assert_equal(self.nodes[0].testmempoolaccept(txns)[0]["reject-reason"], "too-long-mempool-chain")
assert_raises_rpc_error(-26, "too-long-mempool-chain", self.nodes[0].submitpackage, txns)
# But not if it chains directly off the first transaction
replacable_tx = self.wallet.send_self_transfer_multi(from_node=self.nodes[0], utxos_to_spend=[chain[0]])['tx']
self.nodes[0].sendrawtransaction(replacable_tx["hex"])
# and the second chain should work just fine
self.chain_tx([second_chain])

# Make sure we can RBF the chain which used our carve-out rule
replacable_tx.vout[0].nValue -= 1000000
self.nodes[0].sendrawtransaction(replacable_tx.serialize().hex())
replacement_tx = replacable_tx["tx"]
replacement_tx.vout[0].nValue -= 1000000
self.nodes[0].sendrawtransaction(replacement_tx.serialize().hex())

# Finally, check that we added two transactions
assert_equal(len(self.nodes[0].getrawmempool()), DEFAULT_ANCESTOR_LIMIT + 3)
Expand Down
233 changes: 233 additions & 0 deletions test/functional/mempool_package_rbf.py
@@ -0,0 +1,233 @@
#!/usr/bin/env python3
# Copyright (c) 2021 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.

from decimal import Decimal

from test_framework.messages import (
COIN,
MAX_BIP125_RBF_SEQUENCE,
)
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_greater_than_or_equal,
assert_raises_rpc_error,
)
from test_framework.wallet import (
DEFAULT_FEE,
MiniWallet,
)

class PackageRBFTest(BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 1
self.setup_clean_chain = True

def assert_mempool_contents(self, expected=None, unexpected=None):
"""Assert that all transactions in expected are in the mempool,
and all transactions in unexpected are not in the mempool.
"""
if not expected:
expected = []
if not unexpected:
unexpected = []
assert set(unexpected).isdisjoint(expected)
mempool = self.nodes[0].getrawmempool(verbose=False)
for tx in expected:
assert tx.rehash() in mempool
for tx in unexpected:
assert tx.rehash() not in mempool

def create_simple_package(self, parent_coin, parent_fee=0, child_fee=DEFAULT_FEE, heavy_child=False, version=3):
"""Create a 1 parent 1 child package using the coin passed in as the parent's input. The
parent has 1 output, used to fund 1 child transaction.
All transactions signal BIP125 replaceability, but nSequence changes based on self.ctr. This
prevents identical txids between packages when the parents spend the same coin and have the
same fee (i.e. 0sat).
returns tuple (hex serialized txns, CTransaction objects)
"""
self.ctr += 1
# Use fee_rate=0 because create_self_transfer will use the default fee_rate value otherwise.
# Passing in fee>0 overrides fee_rate, so this still works for non-zero parent_fee.
parent_result = self.wallet.create_self_transfer(
fee_rate=0,
fee=parent_fee,
utxo_to_spend=parent_coin,
sequence=MAX_BIP125_RBF_SEQUENCE - self.ctr,
version=version
)

num_child_outputs = 10 if heavy_child else 1
child_result = self.wallet.create_self_transfer_multi(
utxos_to_spend=[parent_result["new_utxo"]],
num_outputs=num_child_outputs,
fee_per_output=int(child_fee * COIN // num_child_outputs),
sequence=MAX_BIP125_RBF_SEQUENCE - self.ctr,
version=version
)
package_hex = [parent_result["hex"], child_result["hex"]]
package_txns = [parent_result["tx"], child_result["tx"]]
return package_hex, package_txns

def run_test(self):
# Counter used to count the number of times we constructed packages. Since we're constructing parent transactions with the same
# coins (to create conflicts), and giving them the same fee (i.e. 0, since their respective children are paying), we might
# accidentally just create the exact same transaction again. To prevent this, set nSequences to MAX_BIP125_RBF_SEQUENCE - self.ctr.
self.ctr = 0

self.log.info("Generate blocks to create UTXOs")
node = self.nodes[0]
self.wallet = MiniWallet(node)
self.generate(self.wallet, 160)
self.coins = self.wallet.get_utxos(mark_as_spent=False)
# Mature coinbase transactions
self.generate(self.wallet, 100)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary. get_utxos already filters for mature coinbases, and you're making 60 of those above.

self.address = self.wallet.get_address()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this is not used anywhere:

Suggested change
self.address = self.wallet.get_address()


self.test_package_rbf_basic()
self.test_package_rbf_signaling()
self.test_package_rbf_additional_fees()
self.test_package_rbf_max_conflicts()
self.test_package_rbf_conflicting_conflicts()
self.test_package_rbf_partial()

def test_package_rbf_basic(self):
self.log.info("Test that a child can pay to replace its parents' conflicts")
node = self.nodes[0]
# Reuse the same coins so that the transactions conflict with one another.
parent_coin = self.coins.pop()
package_hex1, package_txns1 = self.create_simple_package(parent_coin, DEFAULT_FEE, DEFAULT_FEE)
package_hex2, package_txns2 = self.create_simple_package(parent_coin, 0, DEFAULT_FEE * 5)
node.submitpackage(package_hex1)
self.assert_mempool_contents(expected=package_txns1, unexpected=package_txns2)

submitres = node.submitpackage(package_hex2)
submitres["replaced-transactions"] == [tx.rehash() for tx in package_txns1]
self.assert_mempool_contents(expected=package_txns2, unexpected=package_txns1)
self.generate(node, 1)

def test_package_rbf_signaling(self):
node = self.nodes[0]
self.log.info("Test that V3 transactions not signaling BIP125 are replaceable")
# Create single transaction that doesn't signal BIP125 but has nVersion=3
coin = self.coins.pop()

tx_v3_no_bip125 = self.wallet.create_self_transfer(
fee=DEFAULT_FEE,
utxo_to_spend=coin,
sequence=MAX_BIP125_RBF_SEQUENCE + 1,
version=3
)
node.sendrawtransaction(tx_v3_no_bip125["hex"])
self.assert_mempool_contents(expected=[tx_v3_no_bip125["tx"]])

self.log.info("Test that non-V3 transactions signaling BIP125 are replaceable")
coin = self.coins[0]
del self.coins[0]
Comment on lines +127 to +128
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
coin = self.coins[0]
del self.coins[0]
coin = self.coins.pop(0)

# This transaction signals BIP125 but isn't V3
tx_bip125_v2 = self.wallet.create_self_transfer(
fee=DEFAULT_FEE,
utxo_to_spend=coin,
version=2
)
node.sendrawtransaction(tx_bip125_v2["hex"])

self.assert_mempool_contents(expected=[tx_bip125_v2["tx"]])
assert node.getmempoolentry(tx_bip125_v2["tx"].rehash())["bip125-replaceable"]
assert tx_bip125_v2["tx"].nVersion == 2
package_hex_v3, package_txns_v3 = self.create_simple_package(coin, parent_fee=0, child_fee=DEFAULT_FEE * 3, version=3)
assert all([tx.nVersion == 3 for tx in package_txns_v3])
node.submitpackage(package_hex_v3)
self.assert_mempool_contents(expected=package_txns_v3, unexpected=[tx_bip125_v2["tx"]])
self.generate(node, 1)

def test_package_rbf_additional_fees(self):
self.log.info("Check Package RBF must increase the absolute fee")
node = self.nodes[0]
coin = self.coins.pop()
package_hex1, package_txns1 = self.create_simple_package(coin, parent_fee=DEFAULT_FEE, child_fee=DEFAULT_FEE, heavy_child=True)
assert_greater_than_or_equal(1000, package_txns1[-1].get_vsize())
node.submitpackage(package_hex1)
self.assert_mempool_contents(expected=package_txns1)
# Package 2 has a higher feerate but lower absolute fee
package_fees1 = DEFAULT_FEE * 2
package_hex2, package_txns2 = self.create_simple_package(coin, parent_fee=0, child_fee=package_fees1 - Decimal("0.000000001"))
assert_raises_rpc_error(-25, "package RBF failed: insufficient fee", node.submitpackage, package_hex2)
self.assert_mempool_contents(expected=package_txns1, unexpected=package_txns2)
# Package 3 has a higher feerate and absolute fee
package_hex3, package_txns3 = self.create_simple_package(coin, parent_fee=0, child_fee=package_fees1 * 3)
node.submitpackage(package_hex3)
self.assert_mempool_contents(expected=package_txns3, unexpected=package_txns1 + package_txns2)
self.generate(node, 1)

self.log.info("Check Package RBF must pay for the entire package's bandwidth")
coin = self.coins.pop()
package_hex1, package_txns1 = self.create_simple_package(coin, parent_fee=DEFAULT_FEE, child_fee=DEFAULT_FEE)
package_fees1 = 2 * DEFAULT_FEE
node.submitpackage(package_hex1)
self.assert_mempool_contents(expected=package_txns1, unexpected=[])
package_hex2, package_txns2 = self.create_simple_package(coin, child_fee=package_fees1 + Decimal("0.000000001"))
assert_raises_rpc_error(-25, "package RBF failed: insufficient fee", node.submitpackage, package_hex2)
self.assert_mempool_contents(expected=package_txns1, unexpected=package_txns2)
self.generate(node, 1)

def test_package_rbf_max_conflicts(self):
node = self.nodes[0]
self.log.info("Check Package RBF cannot replace more than 100 transactions")
num_coins = 5
parent_coins = self.coins[:num_coins]
del self.coins[:num_coins]
# Original transactions: 5 transactions with 24 descendants each.
for coin in parent_coins:
self.wallet.send_self_transfer_chain(from_node=node, chain_length=25, utxo_to_spend=coin)

# Replacement package: 1 parent which conflicts with 5 * (1 + 24) = 125 mempool transactions.
package_parent = self.wallet.create_self_transfer_multi(utxos_to_spend=parent_coins, version=3)
package_child = self.wallet.create_self_transfer(fee_rate=50*DEFAULT_FEE, utxo_to_spend=package_parent["new_utxos"][0], version=3)

assert_raises_rpc_error(-25, "package RBF failed: too many potential replacements",
node.submitpackage, [package_parent["hex"], package_child["hex"]])
self.generate(node, 1)

def test_package_rbf_conflicting_conflicts(self):
node = self.nodes[0]
self.log.info("Check that different package transactions cannot share the same conflicts")
coin = self.coins.pop()
package_hex1, package_txns1 = self.create_simple_package(coin, DEFAULT_FEE, DEFAULT_FEE)
package_hex2, package_txns2 = self.create_simple_package(coin, Decimal("0.00009"), DEFAULT_FEE * 2)
package_hex3, package_txns3 = self.create_simple_package(coin, 0, DEFAULT_FEE * 5)
node.submitpackage(package_hex1)
self.assert_mempool_contents(expected=package_txns1)
# The first two transactions have the same conflicts
package_duplicate_conflicts_hex = [package_hex2[0]] + package_hex3
# Note that this won't actually go into the RBF logic, because static package checks will
# detect that two package transactions conflict with each other. Either way, this must fail.
assert_raises_rpc_error(-25, "package topology disallowed", node.submitpackage, package_duplicate_conflicts_hex)
self.assert_mempool_contents(expected=package_txns1, unexpected=package_txns2 + package_txns3)
# The RBFs should otherwise work.
submitres2 = node.submitpackage(package_hex2)
submitres2["replaced-transactions"] == [tx.rehash() for tx in package_txns1]
self.assert_mempool_contents(expected=package_txns2, unexpected=package_txns1)
submitres3 = node.submitpackage(package_hex3)
submitres3["replaced-transactions"] == [tx.rehash() for tx in package_txns2]
Comment on lines +211 to +214
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing assertions for the replaced-tx checks:

Suggested change
submitres2["replaced-transactions"] == [tx.rehash() for tx in package_txns1]
self.assert_mempool_contents(expected=package_txns2, unexpected=package_txns1)
submitres3 = node.submitpackage(package_hex3)
submitres3["replaced-transactions"] == [tx.rehash() for tx in package_txns2]
assert submitres2["replaced-transactions"] == [tx.rehash() for tx in package_txns1]
self.assert_mempool_contents(expected=package_txns2, unexpected=package_txns1)
submitres3 = node.submitpackage(package_hex3)
assert submitres3["replaced-transactions"] == [tx.rehash() for tx in package_txns2]

self.assert_mempool_contents(expected=package_txns3, unexpected=package_txns2)

def test_package_rbf_partial(self):
self.log.info("Test that package RBF works when a transaction was already submitted")
node = self.nodes[0]
coin = self.coins.pop()
package_hex1, package_txns1 = self.create_simple_package(coin, DEFAULT_FEE, DEFAULT_FEE)
package_hex2, package_txns2 = self.create_simple_package(coin, DEFAULT_FEE * 3, DEFAULT_FEE * 3)
node.submitpackage(package_hex1)
self.assert_mempool_contents(expected=package_txns1, unexpected=package_txns2)
# Submit parent on its own. It should have no trouble replacing the previous
# transaction(s) because the fee is tripled.
node.sendrawtransaction(package_hex2[0])
node.submitpackage(package_hex2)
self.assert_mempool_contents(expected=package_txns2, unexpected=package_txns1)
self.generate(node, 1)

if __name__ == "__main__":
PackageRBFTest().main()
16 changes: 13 additions & 3 deletions test/functional/test_framework/wallet.py
Expand Up @@ -291,7 +291,8 @@ def create_self_transfer_multi(
sequence=0,
fee_per_output=1000,
target_weight=0,
confirmed_only=False
confirmed_only=False,
version=2
):
"""
Create and return a transaction that spends the given UTXOs and creates a
Expand All @@ -315,6 +316,7 @@ def create_self_transfer_multi(
tx.vin = [CTxIn(COutPoint(int(utxo_to_spend['txid'], 16), utxo_to_spend['vout']), nSequence=seq) for utxo_to_spend, seq in zip(utxos_to_spend, sequence)]
tx.vout = [CTxOut(amount_per_output, bytearray(self._scriptPubKey)) for _ in range(num_outputs)]
tx.nLockTime = locktime
tx.nVersion = version

self.sign_tx(tx)

Expand Down Expand Up @@ -345,7 +347,8 @@ def create_self_transfer(self, *,
locktime=0,
sequence=0,
target_weight=0,
confirmed_only=False
confirmed_only=False,
version=2
):
"""Create and return a tx with the specified fee. If fee is 0, use fee_rate, where the resulting fee may be exact or at most one satoshi higher than needed."""
utxo_to_spend = utxo_to_spend or self.get_utxo(confirmed_only=confirmed_only)
Expand All @@ -361,7 +364,14 @@ def create_self_transfer(self, *,
send_value = utxo_to_spend["value"] - (fee or (fee_rate * vsize / 1000))

# create tx
tx = self.create_self_transfer_multi(utxos_to_spend=[utxo_to_spend], locktime=locktime, sequence=sequence, amount_per_output=int(COIN * send_value), target_weight=target_weight)
tx = self.create_self_transfer_multi(
utxos_to_spend=[utxo_to_spend],
locktime=locktime,
sequence=sequence,
amount_per_output=int(COIN * send_value),
target_weight=target_weight,
version=version
)
if not target_weight:
assert_equal(tx["tx"].get_vsize(), vsize)
tx["new_utxo"] = tx.pop("new_utxos")[0]
Expand Down
2 changes: 2 additions & 0 deletions test/functional/test_runner.py
Expand Up @@ -258,6 +258,7 @@
'p2p_invalid_tx.py --v2transport',
'p2p_v2_transport.py',
'example_test.py',
'mempool_accept_v3.py',
'wallet_txn_doublespend.py --legacy-wallet',
'wallet_multisig_descriptor_psbt.py --descriptors',
'wallet_txn_doublespend.py --descriptors',
Expand All @@ -273,6 +274,7 @@
'mempool_packages.py',
'mempool_package_onemore.py',
'mempool_package_limits.py',
'mempool_package_rbf.py',
'feature_versionbits_warning.py',
'rpc_preciousblock.py',
'wallet_importprunedfunds.py --legacy-wallet',
Expand Down