diff options
-rwxr-xr-x | ci/lint/04_install.sh | 6 | ||||
-rw-r--r-- | configure.ac | 4 | ||||
-rw-r--r-- | doc/build-openbsd.md | 2 | ||||
-rw-r--r-- | doc/developer-notes.md | 19 | ||||
-rw-r--r-- | doc/policy/packages.md | 45 | ||||
-rw-r--r-- | src/bench/rollingbloom.cpp | 13 | ||||
-rw-r--r-- | src/logging.cpp | 2 | ||||
-rw-r--r-- | src/logging.h | 2 | ||||
-rw-r--r-- | src/net.h | 2 | ||||
-rw-r--r-- | src/qt/bitcoingui.h | 2 | ||||
-rw-r--r-- | src/rpc/blockchain.cpp | 4 | ||||
-rw-r--r-- | src/sync.h | 5 | ||||
-rw-r--r-- | src/test/checkqueue_tests.cpp | 10 | ||||
-rw-r--r-- | src/test/fuzz/tx_pool.cpp | 14 | ||||
-rw-r--r-- | src/test/txpackage_tests.cpp | 227 | ||||
-rw-r--r-- | src/validation.cpp | 89 | ||||
-rw-r--r-- | src/validation.h | 8 | ||||
-rw-r--r-- | src/wallet/coinselection.h | 2 | ||||
-rwxr-xr-x | test/functional/rpc_misc.py | 3 | ||||
-rwxr-xr-x | test/lint/lint-qt.sh | 20 |
20 files changed, 413 insertions, 66 deletions
diff --git a/ci/lint/04_install.sh b/ci/lint/04_install.sh index 1518c033f4..8330df87eb 100755 --- a/ci/lint/04_install.sh +++ b/ci/lint/04_install.sh @@ -11,9 +11,9 @@ ${CI_RETRY_EXE} apt-get install -y clang-format-9 python3-pip curl git gawk jq update-alternatives --install /usr/bin/clang-format clang-format "$(which clang-format-9 )" 100 update-alternatives --install /usr/bin/clang-format-diff clang-format-diff "$(which clang-format-diff-9)" 100 -${CI_RETRY_EXE} pip3 install codespell==2.0.0 -${CI_RETRY_EXE} pip3 install flake8==3.8.3 -${CI_RETRY_EXE} pip3 install mypy==0.910 +${CI_RETRY_EXE} pip3 install codespell==2.1.0 +${CI_RETRY_EXE} pip3 install flake8==4.0.1 +${CI_RETRY_EXE} pip3 install mypy==0.942 ${CI_RETRY_EXE} pip3 install pyzmq==22.3.0 ${CI_RETRY_EXE} pip3 install vulture==2.3 diff --git a/configure.ac b/configure.ac index afec18ff83..c6ece881ac 100644 --- a/configure.ac +++ b/configure.ac @@ -361,7 +361,7 @@ case $host in esac if test "$enable_debug" = "yes"; then - dnl If debugging is enabled, and the user hasn't overriden CXXFLAGS, clear + dnl If debugging is enabled, and the user hasn't overridden CXXFLAGS, clear dnl them, to prevent autoconfs "-g -O2" being added. Otherwise we'd end up dnl with "-O0 -g3 -g -O2". if test "$CXXFLAGS_overridden" = "no"; then @@ -857,7 +857,7 @@ if test "$use_lcov" = "yes"; then [AC_MSG_ERROR([lcov testing requested but --coverage linker flag does not work])]) AX_CHECK_COMPILE_FLAG([--coverage],[CORE_CXXFLAGS="$CORE_CXXFLAGS --coverage"], [AC_MSG_ERROR([lcov testing requested but --coverage flag does not work])]) - dnl If coverage is enabled, and the user hasn't overriden CXXFLAGS, clear + dnl If coverage is enabled, and the user hasn't overridden CXXFLAGS, clear dnl them, to prevent autoconfs "-g -O2" being added. Otherwise we'd end up dnl with "--coverage -Og -O0 -g -O2". if test "$CXXFLAGS_overridden" = "no"; then diff --git a/doc/build-openbsd.md b/doc/build-openbsd.md index 1d8da1143e..99daf73c86 100644 --- a/doc/build-openbsd.md +++ b/doc/build-openbsd.md @@ -11,7 +11,7 @@ Run the following as root to install the base dependencies for building. ```bash pkg_add bash git gmake libevent libtool boost -# Select the newest version of the follow packages: +# Select the newest version of the following packages: pkg_add autoconf automake python ``` diff --git a/doc/developer-notes.md b/doc/developer-notes.md index c3ab3fa953..9b1026a375 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -17,6 +17,7 @@ Developer Notes - [`debug.log`](#debuglog) - [Signet, testnet, and regtest modes](#signet-testnet-and-regtest-modes) - [DEBUG_LOCKORDER](#debug_lockorder) + - [DEBUG_LOCKCONTENTION](#debug_lockcontention) - [Valgrind suppressions file](#valgrind-suppressions-file) - [Compiling for test coverage](#compiling-for-test-coverage) - [Performance profiling with perf](#performance-profiling-with-perf) @@ -362,6 +363,19 @@ configure option adds `-DDEBUG_LOCKORDER` to the compiler flags. This inserts run-time checks to keep track of which locks are held and adds warnings to the `debug.log` file if inconsistencies are detected. +### DEBUG_LOCKCONTENTION + +Defining `DEBUG_LOCKCONTENTION` adds a "lock" logging category to the logging +RPC that, when enabled, logs the location and duration of each lock contention +to the `debug.log` file. + +To enable it, run configure with `-DDEBUG_LOCKCONTENTION` added to your +CPPFLAGS, e.g. `CPPFLAGS="-DDEBUG_LOCKCONTENTION"`, then build and run bitcoind. + +You can then use the `-debug=lock` configuration option at bitcoind startup or +`bitcoin-cli logging '["lock"]'` at runtime to turn on lock contention logging. +It can be toggled off again with `bitcoin-cli logging [] '["lock"]'`. + ### Assertions and Checks The util file `src/util/check.h` offers helpers to protect against coding and @@ -1206,7 +1220,10 @@ A few guidelines for introducing and reviewing new RPC interfaces: - *Rationale*: Consistency with the existing interface. -- Argument naming: use snake case `fee_delta` (and not, e.g. camel case `feeDelta`) +- Argument and field naming: please consider whether there is already a naming + style or spelling convention in the API for the type of object in question + (`blockhash`, for example), and if so, try to use that. If not, use snake case + `fee_delta` (and not, e.g. `feedelta` or camel case `feeDelta`). - *Rationale*: Consistency with the existing interface. diff --git a/doc/policy/packages.md b/doc/policy/packages.md index 7f7fbe18cd..f2a3d6517e 100644 --- a/doc/policy/packages.md +++ b/doc/policy/packages.md @@ -72,3 +72,48 @@ test accepts): a competing package or transaction with a mutated witness, even though the two same-txid-different-witness transactions are conflicting and cannot replace each other, the honest package should still be considered for acceptance. + +### Package Fees and Feerate + +*Package Feerate* is the total modified fees (base fees + any fee delta from +`prioritisetransaction`) divided by the total virtual size of all transactions in the package. +If any transactions in the package are already in the mempool, they are not submitted again +("deduplicated") and are thus excluded from this calculation. + +To meet the two feerate requirements of a mempool, i.e., the pre-configured minimum relay feerate +(`minRelayTxFee`) and the dynamic mempool minimum feerate, the total package feerate is used instead +of the individual feerate. The individual transactions are allowed to be below the feerate +requirements if the package meets the feerate requirements. For example, the parent(s) in the +package can pay no fees but be paid for by the child. + +*Rationale*: This can be thought of as "CPFP within a package," solving the issue of a parent not +meeting minimum fees on its own. This would allow contracting applications to adjust their fees at +broadcast time instead of overshooting or risking becoming stuck or pinned. + +*Rationale*: It would be incorrect to use the fees of transactions that are already in the mempool, as +we do not want a transaction's fees to be double-counted. + +Implementation Note: Transactions within a package are always validated individually first, and +package validation is used for the transactions that failed. Since package feerate is only +calculated using transactions that are not in the mempool, this implementation detail affects the +outcome of package validation. + +*Rationale*: Packages are intended for incentive-compatible fee-bumping: transaction B is a +"legitimate" fee-bump for transaction A only if B is a descendant of A and has a *higher* feerate +than A. We want to prevent "parents pay for children" behavior; fees of parents should not help +their children, since the parents can be mined without the child. More generally, if transaction A +is not needed in order for transaction B to be mined, A's fees cannot help B. In a +child-with-parents package, simply excluding any parent transactions that meet feerate requirements +individually is sufficient to ensure this. + +*Rationale*: We must not allow a low-feerate child to prevent its parent from being accepted; fees +of children should not negatively impact their parents, since they are not necessary for the parents +to be mined. More generally, if transaction B is not needed in order for transaction A to be mined, +B's fees cannot harm A. In a child-with-parents package, simply validating parents individually +first is sufficient to ensure this. + +*Rationale*: As a principle, we want to avoid accidentally restricting policy in order to be +backward-compatible for users and applications that rely on p2p transaction relay. Concretely, +package validation should not prevent the acceptance of a transaction that would otherwise be +policy-valid on its own. By always accepting a transaction that passes individual validation before +trying package validation, we prevent any unintentional restriction of policy. diff --git a/src/bench/rollingbloom.cpp b/src/bench/rollingbloom.cpp index 9f1679aa84..8f05e3bad0 100644 --- a/src/bench/rollingbloom.cpp +++ b/src/bench/rollingbloom.cpp @@ -5,6 +5,9 @@ #include <bench/bench.h> #include <common/bloom.h> +#include <crypto/common.h> + +#include <vector> static void RollingBloom(benchmark::Bench& bench) { @@ -13,16 +16,10 @@ static void RollingBloom(benchmark::Bench& bench) uint32_t count = 0; bench.run([&] { count++; - data[0] = count & 0xFF; - data[1] = (count >> 8) & 0xFF; - data[2] = (count >> 16) & 0xFF; - data[3] = (count >> 24) & 0xFF; + WriteLE32(data.data(), count); filter.insert(data); - data[0] = (count >> 24) & 0xFF; - data[1] = (count >> 16) & 0xFF; - data[2] = (count >> 8) & 0xFF; - data[3] = count & 0xFF; + WriteBE32(data.data(), count); filter.contains(data); }); } diff --git a/src/logging.cpp b/src/logging.cpp index 764941c8ea..a5e5fdb4cd 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -160,7 +160,9 @@ const CLogCategoryDesc LogCategories[] = {BCLog::VALIDATION, "validation"}, {BCLog::I2P, "i2p"}, {BCLog::IPC, "ipc"}, +#ifdef DEBUG_LOCKCONTENTION {BCLog::LOCK, "lock"}, +#endif {BCLog::UTIL, "util"}, {BCLog::BLOCKSTORE, "blockstorage"}, {BCLog::ALL, "1"}, diff --git a/src/logging.h b/src/logging.h index 710e6c4c32..ae8cad906c 100644 --- a/src/logging.h +++ b/src/logging.h @@ -60,7 +60,9 @@ namespace BCLog { VALIDATION = (1 << 21), I2P = (1 << 22), IPC = (1 << 23), +#ifdef DEBUG_LOCKCONTENTION LOCK = (1 << 24), +#endif UTIL = (1 << 25), BLOCKSTORE = (1 << 26), ALL = ~(uint32_t)0, @@ -13,6 +13,7 @@ #include <crypto/siphash.h> #include <hash.h> #include <i2p.h> +#include <logging.h> #include <net_permissions.h> #include <netaddress.h> #include <netbase.h> @@ -32,6 +33,7 @@ #include <cstdint> #include <deque> #include <functional> +#include <list> #include <map> #include <memory> #include <optional> diff --git a/src/qt/bitcoingui.h b/src/qt/bitcoingui.h index 0ae5f7331e..d2b29ba27b 100644 --- a/src/qt/bitcoingui.h +++ b/src/qt/bitcoingui.h @@ -303,7 +303,7 @@ public Q_SLOTS: /** Show window if hidden, unminimize when minimized, rise when obscured or show if hidden and fToggleHidden is true */ void showNormalIfMinimized() { showNormalIfMinimized(false); } void showNormalIfMinimized(bool fToggleHidden); - /** Simply calls showNormalIfMinimized(true) for use in SLOT() macro */ + /** Simply calls showNormalIfMinimized(true) */ void toggleHidden(); /** called by a timer to check if ShutdownRequested() has been set **/ diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index cf72af1012..a124e0267c 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -430,7 +430,7 @@ static RPCHelpMan getblockfrompeer() "Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.\n\n" "Returns an empty JSON object if the request was successfully scheduled.", { - {"block_hash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash to try to fetch"}, + {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash to try to fetch"}, {"peer_id", RPCArg::Type::NUM, RPCArg::Optional::NO, "The peer to fetch it from (see getpeerinfo for peer IDs)"}, }, RPCResult{RPCResult::Type::OBJ, "", /*optional=*/false, "", {}}, @@ -444,7 +444,7 @@ static RPCHelpMan getblockfrompeer() ChainstateManager& chainman = EnsureChainman(node); PeerManager& peerman = EnsurePeerman(node); - const uint256& block_hash{ParseHashV(request.params[0], "block_hash")}; + const uint256& block_hash{ParseHashV(request.params[0], "blockhash")}; const NodeId peer_id{request.params[1].get_int64()}; const CBlockIndex* const index = WITH_LOCK(cs_main, return chainman.m_blockman.LookupBlockIndex(block_hash);); diff --git a/src/sync.h b/src/sync.h index 85000a9151..af7595e6fa 100644 --- a/src/sync.h +++ b/src/sync.h @@ -6,8 +6,11 @@ #ifndef BITCOIN_SYNC_H #define BITCOIN_SYNC_H +#ifdef DEBUG_LOCKCONTENTION #include <logging.h> #include <logging/timer.h> +#endif + #include <threadsafety.h> #include <util/macros.h> @@ -136,8 +139,10 @@ private: void Enter(const char* pszName, const char* pszFile, int nLine) { EnterCritical(pszName, pszFile, nLine, Base::mutex()); +#ifdef DEBUG_LOCKCONTENTION if (Base::try_lock()) return; LOG_TIME_MICROS_WITH_CATEGORY(strprintf("lock contention %s, %s:%d", pszName, pszFile, nLine), BCLog::LOCK); +#endif Base::lock(); } diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index 153ccd984b..0d95bd7670 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -19,13 +19,17 @@ #include <vector> /** - * Identical to TestingSetup but excludes lock contention logging, as some of - * these tests are designed to be heavily contested to trigger race conditions - * or other issues. + * Identical to TestingSetup but excludes lock contention logging if + * `DEBUG_LOCKCONTENTION` is defined, as some of these tests are designed to be + * heavily contested to trigger race conditions or other issues. */ struct NoLockLoggingTestingSetup : public TestingSetup { NoLockLoggingTestingSetup() +#ifdef DEBUG_LOCKCONTENTION : TestingSetup{CBaseChainParams::MAIN, /*extra_args=*/{"-debugexclude=lock"}} {} +#else + : TestingSetup{CBaseChainParams::MAIN} {} +#endif }; BOOST_FIXTURE_TEST_SUITE(checkqueue_tests, NoLockLoggingTestingSetup) diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index df5b271d06..f686f4fd86 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -234,14 +234,18 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool) const bool bypass_limits = fuzzed_data_provider.ConsumeBool(); ::fRequireStandard = fuzzed_data_provider.ConsumeBool(); - // Make sure ProcessNewPackage on one transaction works and always fully validates the transaction. + // Make sure ProcessNewPackage on one transaction works. // The result is not guaranteed to be the same as what is returned by ATMP. const auto result_package = WITH_LOCK(::cs_main, return ProcessNewPackage(chainstate, tx_pool, {tx}, true)); - auto it = result_package.m_tx_results.find(tx->GetWitnessHash()); - Assert(it != result_package.m_tx_results.end()); - Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID || - it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID); + // If something went wrong due to a package-specific policy, it might not return a + // validation result for the transaction. + if (result_package.m_state.GetResult() != PackageValidationResult::PCKG_POLICY) { + auto it = result_package.m_tx_results.find(tx->GetWitnessHash()); + Assert(it != result_package.m_tx_results.end()); + Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID || + it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID); + } const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, tx, GetTime(), bypass_limits, /*test_accept=*/false)); const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID; diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 55919b7f95..079b753304 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -98,7 +98,9 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) BOOST_CHECK(it_child != result_parent_child.m_tx_results.end()); BOOST_CHECK_MESSAGE(it_child->second.m_state.IsValid(), "Package validation unexpectedly failed: " << it_child->second.m_state.GetRejectReason()); - + BOOST_CHECK(result_parent_child.m_package_feerate.has_value()); + BOOST_CHECK(result_parent_child.m_package_feerate.value() == + CFeeRate(2 * COIN, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child))); // A single, giant transaction submitted through ProcessNewPackage fails on single tx policy. CTransactionRef giant_ptx = create_placeholder_tx(999, 999); @@ -110,6 +112,7 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) auto it_giant_tx = result_single_large.m_tx_results.find(giant_ptx->GetWitnessHash()); BOOST_CHECK(it_giant_tx != result_single_large.m_tx_results.end()); BOOST_CHECK_EQUAL(it_giant_tx->second.m_state.GetRejectReason(), "tx-size"); + BOOST_CHECK(result_single_large.m_package_feerate == std::nullopt); // Check that mempool size hasn't changed. BOOST_CHECK_EQUAL(m_node.mempool->size(), initialPoolSize); @@ -230,6 +233,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) BOOST_CHECK_EQUAL(result_unrelated_submit.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(result_unrelated_submit.m_state.GetRejectReason(), "package-not-child-with-parents"); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + BOOST_CHECK(result_unrelated_submit.m_package_feerate == std::nullopt); // Parent and Child (and Grandchild) Package Package package_parent_child; @@ -271,6 +275,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) BOOST_CHECK_EQUAL(result_3gen_submit.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(result_3gen_submit.m_state.GetRejectReason(), "package-not-child-with-parents"); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + BOOST_CHECK(result_3gen_submit.m_package_feerate == std::nullopt); } // Child with missing parent. @@ -286,6 +291,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) 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); + BOOST_CHECK(result_missing_parent.m_package_feerate == std::nullopt); } // Submit package with parent + child. @@ -305,6 +311,10 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) 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->GetHash()))); + + // Since both transactions have high feerates, they each passed validation individually. + // Package validation was unnecessary, so there is no package feerate. + BOOST_CHECK(submit_parent_child.m_package_feerate == std::nullopt); } // Already-in-mempool transactions should be detected and de-duplicated. @@ -325,6 +335,8 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) 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->GetHash()))); + + BOOST_CHECK(submit_deduped.m_package_feerate == std::nullopt); } } @@ -399,8 +411,12 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_parent->GetHash()))); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_child1->GetHash()))); + // Child2 would have been validated individually. + BOOST_CHECK(submit_witness1.m_package_feerate == std::nullopt); + const auto submit_witness2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, {ptx_parent, ptx_child2}, /*test_accept=*/false); + BOOST_CHECK(submit_witness2.m_package_feerate == std::nullopt); BOOST_CHECK_MESSAGE(submit_witness2.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_witness2.m_state.GetRejectReason()); auto it_parent2_deduped = submit_witness2.m_tx_results.find(ptx_parent->GetWitnessHash()); @@ -424,6 +440,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) auto it_child_dup = submit_segwit_dedup.m_tx_results.find(ptx_child1->GetWitnessHash()); BOOST_CHECK(it_parent_dup->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY); BOOST_CHECK(it_child_dup->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY); + BOOST_CHECK(submit_witness2.m_package_feerate == std::nullopt); } // Try submitting Package1{child2, grandchild} where child2 is same-txid-different-witness as @@ -458,6 +475,9 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_child2->GetHash()))); BOOST_CHECK(!m_node.mempool->exists(GenTxid::Wtxid(ptx_child2->GetWitnessHash()))); BOOST_CHECK(m_node.mempool->exists(GenTxid::Wtxid(ptx_grandchild->GetWitnessHash()))); + + // Since child2 is ignored, grandchild would be validated individually. + BOOST_CHECK(submit_spend_ignored.m_package_feerate == std::nullopt); } // A package Package{parent1, parent2, parent3, child} where the parents are a mixture of @@ -516,11 +536,11 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) BOOST_CHECK(parent2_v2_result.m_result_type == MempoolAcceptResult::ResultType::VALID); package_mixed.push_back(ptx_parent2_v1); - // parent3 will be a new transaction + // parent3 will be a new transaction. Put 0 fees on it to make it invalid on its own. auto mtx_parent3 = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[3], /*input_vout=*/0, /*input_height=*/0, /*input_signing_key=*/coinbaseKey, /*output_destination=*/acs_spk, - /*output_amount=*/CAmount(49 * COIN), /*submit=*/false); + /*output_amount=*/CAmount(50 * COIN), /*submit=*/false); CTransactionRef ptx_parent3 = MakeTransactionRef(mtx_parent3); package_mixed.push_back(ptx_parent3); @@ -536,7 +556,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) mtx_mixed_child.vin[0].scriptWitness = acs_witness; mtx_mixed_child.vin[1].scriptWitness = acs_witness; mtx_mixed_child.vin[2].scriptWitness = acs_witness; - mtx_mixed_child.vout.push_back(CTxOut(145 * COIN, mixed_child_spk)); + mtx_mixed_child.vout.push_back(CTxOut((48 + 49 + 50 - 1) * COIN, mixed_child_spk)); CTransactionRef ptx_mixed_child = MakeTransactionRef(mtx_mixed_child); package_mixed.push_back(ptx_mixed_child); @@ -568,6 +588,205 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) BOOST_CHECK(!m_node.mempool->exists(GenTxid::Wtxid(ptx_parent2_v1->GetWitnessHash()))); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_parent3->GetHash()))); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_mixed_child->GetHash()))); + + // package feerate should include parent3 and child. It should not include parent1 or parent2_v1. + BOOST_CHECK(mixed_result.m_package_feerate.has_value()); + const CFeeRate expected_feerate(1 * COIN, GetVirtualTransactionSize(*ptx_parent3) + GetVirtualTransactionSize(*ptx_mixed_child)); + BOOST_CHECK_MESSAGE(mixed_result.m_package_feerate.value() == expected_feerate, + strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), + mixed_result.m_package_feerate.value().ToString())); + } +} + +BOOST_FIXTURE_TEST_CASE(package_cpfp_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())); + + // zero-fee parent and high-fee child package + const CAmount coinbase_value{50 * COIN}; + const CAmount parent_value{coinbase_value - 0}; + const CAmount child_value{parent_value - COIN}; + + Package package_cpfp; + 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=*/parent_value, /*submit=*/false); + CTransactionRef tx_parent = MakeTransactionRef(mtx_parent); + package_cpfp.push_back(tx_parent); + + auto mtx_child = CreateValidMempoolTransaction(/*input_transaction=*/tx_parent, /*input_vout=*/0, + /*input_height=*/101, /*input_signing_key=*/child_key, + /*output_destination=*/child_spk, + /*output_amount=*/child_value, /*submit=*/false); + CTransactionRef tx_child = MakeTransactionRef(mtx_child); + package_cpfp.push_back(tx_child); + + // Package feerate is calculated using modified fees, and prioritisetransaction accepts negative + // fee deltas. This should be taken into account. De-prioritise the parent transaction by -1BTC, + // bringing the package feerate to 0. + m_node.mempool->PrioritiseTransaction(tx_parent->GetHash(), -1 * COIN); + { + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + const auto submit_cpfp_deprio = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + package_cpfp, /*test_accept=*/ false); + BOOST_CHECK_MESSAGE(submit_cpfp_deprio.m_state.IsInvalid(), + "Package validation unexpectedly succeeded: " << submit_cpfp_deprio.m_state.GetRejectReason()); + BOOST_CHECK(submit_cpfp_deprio.m_tx_results.empty()); + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + const CFeeRate expected_feerate(0, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child)); + BOOST_CHECK(submit_cpfp_deprio.m_package_feerate.has_value()); + BOOST_CHECK(submit_cpfp_deprio.m_package_feerate.value() == CFeeRate{0}); + BOOST_CHECK_MESSAGE(submit_cpfp_deprio.m_package_feerate.value() == expected_feerate, + strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), + submit_cpfp_deprio.m_package_feerate.value().ToString())); + } + + // Clear the prioritisation of the parent transaction. + WITH_LOCK(m_node.mempool->cs, m_node.mempool->ClearPrioritisation(tx_parent->GetHash())); + + // Package CPFP: Even though the parent pays 0 absolute fees, the child pays 1 BTC which is + // enough for the package feerate to meet the threshold. + { + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + const auto submit_cpfp = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + package_cpfp, /*test_accept=*/ false); + expected_pool_size += 2; + BOOST_CHECK_MESSAGE(submit_cpfp.m_state.IsValid(), + "Package validation unexpectedly failed: " << submit_cpfp.m_state.GetRejectReason()); + auto it_parent = submit_cpfp.m_tx_results.find(tx_parent->GetWitnessHash()); + auto it_child = submit_cpfp.m_tx_results.find(tx_child->GetWitnessHash()); + BOOST_CHECK(it_parent != submit_cpfp.m_tx_results.end()); + BOOST_CHECK(it_parent->second.m_result_type == MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK(it_parent->second.m_base_fees.value() == 0); + BOOST_CHECK(it_child != submit_cpfp.m_tx_results.end()); + BOOST_CHECK(it_child->second.m_result_type == MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK(it_child->second.m_base_fees.value() == COIN); + + 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->GetHash()))); + + const CFeeRate expected_feerate(coinbase_value - child_value, + GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child)); + BOOST_CHECK(expected_feerate.GetFeePerK() > 1000); + BOOST_CHECK(submit_cpfp.m_package_feerate.has_value()); + BOOST_CHECK_MESSAGE(submit_cpfp.m_package_feerate.value() == expected_feerate, + strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), + submit_cpfp.m_package_feerate.value().ToString())); + } + + // Just because we allow low-fee parents doesn't mean we allow low-feerate packages. + // This package just pays 200 satoshis total. This would be enough to pay for the child alone, + // but isn't enough for the entire package to meet the 1sat/vbyte minimum. + Package package_still_too_low; + auto mtx_parent_cheap = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[1], /*input_vout=*/0, + /*input_height=*/0, /*input_signing_key=*/coinbaseKey, + /*output_destination=*/parent_spk, + /*output_amount=*/coinbase_value, /*submit=*/false); + CTransactionRef tx_parent_cheap = MakeTransactionRef(mtx_parent_cheap); + package_still_too_low.push_back(tx_parent_cheap); + + auto mtx_child_cheap = CreateValidMempoolTransaction(/*input_transaction=*/tx_parent_cheap, /*input_vout=*/0, + /*input_height=*/101, /*input_signing_key=*/child_key, + /*output_destination=*/child_spk, + /*output_amount=*/coinbase_value - 200, /*submit=*/false); + CTransactionRef tx_child_cheap = MakeTransactionRef(mtx_child_cheap); + package_still_too_low.push_back(tx_child_cheap); + + // Cheap package should fail with package-fee-too-low. + { + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + const auto submit_package_too_low = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + package_still_too_low, /*test_accept=*/false); + BOOST_CHECK_MESSAGE(submit_package_too_low.m_state.IsInvalid(), "Package validation unexpectedly succeeded"); + BOOST_CHECK_EQUAL(submit_package_too_low.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); + BOOST_CHECK_EQUAL(submit_package_too_low.m_state.GetRejectReason(), "package-fee-too-low"); + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + const CFeeRate child_feerate(200, GetVirtualTransactionSize(*tx_child_cheap)); + BOOST_CHECK(child_feerate.GetFeePerK() > 1000); + const CFeeRate expected_feerate(200, + GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap)); + BOOST_CHECK(expected_feerate.GetFeePerK() < 1000); + BOOST_CHECK(submit_package_too_low.m_package_feerate.has_value()); + BOOST_CHECK_MESSAGE(submit_package_too_low.m_package_feerate.value() == expected_feerate, + strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), + submit_package_too_low.m_package_feerate.value().ToString())); + } + + // Package feerate includes the modified fees of the transactions. + // This means a child with its fee delta from prioritisetransaction can pay for a parent. + m_node.mempool->PrioritiseTransaction(tx_child_cheap->GetHash(), 1 * COIN); + // Now that the child's fees have "increased" by 1 BTC, the cheap package should succeed. + { + const auto submit_prioritised_package = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + package_still_too_low, /*test_accept=*/false); + expected_pool_size += 2; + BOOST_CHECK_MESSAGE(submit_prioritised_package.m_state.IsValid(), + "Package validation unexpectedly failed" << submit_prioritised_package.m_state.GetRejectReason()); + const CFeeRate expected_feerate(1 * COIN + 200, + GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap)); + BOOST_CHECK(submit_prioritised_package.m_package_feerate.has_value()); + BOOST_CHECK_MESSAGE(submit_prioritised_package.m_package_feerate.value() == expected_feerate, + strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), + submit_prioritised_package.m_package_feerate.value().ToString())); + } + + // Package feerate is calculated without topology in mind; it's just aggregating fees and sizes. + // However, this should not allow parents to pay for children. Each transaction should be + // validated individually first, eliminating sufficient-feerate parents before they are unfairly + // included in the package feerate. It's also important that the low-fee child doesn't prevent + // the parent from being accepted. + Package package_rich_parent; + const CAmount high_parent_fee{1 * COIN}; + auto mtx_parent_rich = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[2], /*input_vout=*/0, + /*input_height=*/0, /*input_signing_key=*/coinbaseKey, + /*output_destination=*/parent_spk, + /*output_amount=*/coinbase_value - high_parent_fee, /*submit=*/false); + CTransactionRef tx_parent_rich = MakeTransactionRef(mtx_parent_rich); + package_rich_parent.push_back(tx_parent_rich); + + auto mtx_child_poor = CreateValidMempoolTransaction(/*input_transaction=*/tx_parent_rich, /*input_vout=*/0, + /*input_height=*/101, /*input_signing_key=*/child_key, + /*output_destination=*/child_spk, + /*output_amount=*/coinbase_value - high_parent_fee, /*submit=*/false); + CTransactionRef tx_child_poor = MakeTransactionRef(mtx_child_poor); + package_rich_parent.push_back(tx_child_poor); + + // Parent pays 1 BTC and child pays none. The parent should be accepted without the child. + { + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + const auto submit_rich_parent = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + package_rich_parent, /*test_accept=*/false); + expected_pool_size += 1; + BOOST_CHECK_MESSAGE(submit_rich_parent.m_state.IsInvalid(), "Package validation unexpectedly succeeded"); + + // The child would have been validated on its own and failed, then submitted as a "package" of 1. + // The package feerate is just the child's feerate, which is 0sat/vb. + BOOST_CHECK(submit_rich_parent.m_package_feerate.has_value()); + BOOST_CHECK_MESSAGE(submit_rich_parent.m_package_feerate.value() == CFeeRate(), + "expected 0, got " << submit_rich_parent.m_package_feerate.value().ToString()); + BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); + BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetRejectReason(), "package-fee-too-low"); + + auto it_parent = submit_rich_parent.m_tx_results.find(tx_parent_rich->GetWitnessHash()); + BOOST_CHECK(it_parent != submit_rich_parent.m_tx_results.end()); + BOOST_CHECK(it_parent->second.m_result_type == MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK(it_parent->second.m_state.GetRejectReason() == ""); + BOOST_CHECK_MESSAGE(it_parent->second.m_base_fees.value() == high_parent_fee, + strprintf("rich parent: expected fee %s, got %s", high_parent_fee, it_parent->second.m_base_fees.value())); + + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent_rich->GetHash()))); + BOOST_CHECK(!m_node.mempool->exists(GenTxid::Txid(tx_child_poor->GetHash()))); } } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/validation.cpp b/src/validation.cpp index c971b020ae..f4b316f67a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -460,6 +460,10 @@ public: * partially submitted. */ const bool m_package_submission; + /** When true, use package feerates instead of individual transaction feerates for fee-based + * policies such as mempool min fee and min relay fee. + */ + const bool m_package_feerates; /** Parameters for single transaction mempool validation. */ static ATMPArgs SingleAccept(const CChainParams& chainparams, int64_t accept_time, @@ -472,6 +476,7 @@ public: /* m_test_accept */ test_accept, /* m_allow_bip125_replacement */ true, /* m_package_submission */ false, + /* m_package_feerates */ false, }; } @@ -485,6 +490,7 @@ public: /* m_test_accept */ true, /* m_allow_bip125_replacement */ false, /* m_package_submission */ false, // not submitting to mempool + /* m_package_feerates */ false, }; } @@ -498,6 +504,20 @@ public: /* m_test_accept */ false, /* m_allow_bip125_replacement */ false, /* m_package_submission */ true, + /* m_package_feerates */ true, + }; + } + + /** Parameters for a single transaction within a package. */ + static ATMPArgs SingleInPackageAccept(const ATMPArgs& package_args) { + return ATMPArgs{/* m_chainparams */ package_args.m_chainparams, + /* m_accept_time */ package_args.m_accept_time, + /* m_bypass_limits */ false, + /* m_coins_to_uncache */ package_args.m_coins_to_uncache, + /* m_test_accept */ package_args.m_test_accept, + /* m_allow_bip125_replacement */ true, + /* m_package_submission */ false, + /* m_package_feerates */ false, // only 1 transaction }; } @@ -510,14 +530,16 @@ public: std::vector<COutPoint>& coins_to_uncache, bool test_accept, bool allow_bip125_replacement, - bool package_submission) + bool package_submission, + bool package_feerates) : m_chainparams{chainparams}, m_accept_time{accept_time}, m_bypass_limits{bypass_limits}, m_coins_to_uncache{coins_to_uncache}, m_test_accept{test_accept}, m_allow_bip125_replacement{allow_bip125_replacement}, - m_package_submission{package_submission} + m_package_submission{package_submission}, + m_package_feerates{package_feerates} { } }; @@ -819,9 +841,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "bad-txns-too-many-sigops", strprintf("%d", nSigOpsCost)); - // No transactions are allowed below minRelayTxFee except from disconnected - // blocks - if (!bypass_limits && !CheckFeeRate(ws.m_vsize, ws.m_modified_fees, state)) return false; + // No individual transactions are allowed below minRelayTxFee and mempool min fee except from + // disconnected blocks and transactions in a package. Package transactions will be checked using + // package feerate later. + if (!bypass_limits && !args.m_package_feerates && !CheckFeeRate(ws.m_vsize, ws.m_modified_fees, state)) return false; ws.m_iters_conflicting = m_pool.GetIterSet(ws.m_conflicts); // Calculate in-mempool ancestors, up to a limit. @@ -1205,12 +1228,27 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: m_viewmempool.PackageAddTransaction(ws.m_ptx); } + // Transactions must meet two minimum feerates: the mempool minimum fee and min relay fee. + // For transactions consisting of exactly one child and its parents, it suffices to use the + // package feerate (total modified fees / total virtual size) to check this requirement. + const auto m_total_vsize = std::accumulate(workspaces.cbegin(), workspaces.cend(), int64_t{0}, + [](int64_t sum, auto& ws) { return sum + ws.m_vsize; }); + const auto m_total_modified_fees = std::accumulate(workspaces.cbegin(), workspaces.cend(), CAmount{0}, + [](CAmount sum, auto& ws) { return sum + ws.m_modified_fees; }); + const CFeeRate package_feerate(m_total_modified_fees, m_total_vsize); + TxValidationState placeholder_state; + if (args.m_package_feerates && + !CheckFeeRate(m_total_vsize, m_total_modified_fees, placeholder_state)) { + package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-fee-too-low"); + return PackageMempoolAcceptResult(package_state, package_feerate, {}); + } + // Apply package mempool ancestor/descendant limits. Skip if there is only one transaction, // because it's unnecessary. Also, CPFP carve out can increase the limit for individual // transactions, but this exemption is not extended to packages in CheckPackageLimits(). std::string err_string; if (txns.size() > 1 && !PackageMempoolChecks(txns, package_state)) { - return PackageMempoolAcceptResult(package_state, std::move(results)); + return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); } for (Workspace& ws : workspaces) { @@ -1218,7 +1256,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // Exit early to avoid doing pointless work. Update the failed tx result; the rest are unfinished. package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); - return PackageMempoolAcceptResult(package_state, std::move(results)); + return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); } if (args.m_test_accept) { // When test_accept=true, transactions that pass PolicyScriptChecks are valid because there are @@ -1229,14 +1267,14 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: } } - if (args.m_test_accept) return PackageMempoolAcceptResult(package_state, std::move(results)); + if (args.m_test_accept) return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); if (!SubmitPackage(args, workspaces, package_state, results)) { // PackageValidationState filled in by SubmitPackage(). - return PackageMempoolAcceptResult(package_state, std::move(results)); + return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); } - return PackageMempoolAcceptResult(package_state, std::move(results)); + return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); } PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, ATMPArgs& args) @@ -1303,6 +1341,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // 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. + ATMPArgs single_args = ATMPArgs::SingleInPackageAccept(args); + bool quit_early{false}; std::vector<CTransactionRef> txns_new; for (const auto& tx : package) { const auto& wtxid = tx->GetWitnessHash(); @@ -1329,18 +1369,43 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, results.emplace(wtxid, MempoolAcceptResult::MempoolTxDifferentWitness(iter.value()->GetTx().GetWitnessHash())); } else { // Transaction does not already exist in the mempool. - txns_new.push_back(tx); + // Try submitting the transaction on its own. + const auto single_res = AcceptSingleTransaction(tx, single_args); + if (single_res.m_result_type == MempoolAcceptResult::ResultType::VALID) { + // The transaction succeeded on its own and is now in the mempool. Don't include it + // in package validation, because its fees should only be "used" once. + assert(m_pool.exists(GenTxid::Wtxid(wtxid))); + results.emplace(wtxid, single_res); + } else if (single_res.m_state.GetResult() != TxValidationResult::TX_MEMPOOL_POLICY && + single_res.m_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { + // Package validation policy only differs from individual policy in its evaluation + // of feerate. For example, if a transaction fails here due to violation of a + // consensus rule, the result will not change when it is submitted as part of a + // package. To minimize the amount of repeated work, unless the transaction fails + // due to feerate or missing inputs (its parent is a previous transaction in the + // package that failed due to feerate), don't run package validation. Note that this + // decision might not make sense if different types of packages are allowed in the + // future. Continue individually validating the rest of the transactions, because + // some of them may still be valid. + quit_early = true; + } else { + txns_new.push_back(tx); + } } } // Nothing to do if the entire package has already been submitted. - if (txns_new.empty()) return PackageMempoolAcceptResult(package_state, std::move(results)); + if (quit_early || txns_new.empty()) { + // No package feerate when no package validation was done. + return PackageMempoolAcceptResult(package_state, std::move(results)); + } // Validate the (deduplicated) transactions as a package. auto submission_result = AcceptMultipleTransactions(txns_new, args); // Include already-in-mempool transaction results in the final result. for (const auto& [wtxid, mempoolaccept_res] : results) { submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res); } + if (submission_result.m_state.IsValid()) assert(submission_result.m_package_feerate.has_value()); return submission_result; } diff --git a/src/validation.h b/src/validation.h index b13e7f3d8b..53ea2d4aea 100644 --- a/src/validation.h +++ b/src/validation.h @@ -234,11 +234,19 @@ struct PackageMempoolAcceptResult * was a package-wide error (see result in m_state), m_tx_results will be empty. */ std::map<const uint256, const MempoolAcceptResult> m_tx_results; + /** Package feerate, defined as the aggregated modified fees divided by the total virtual size + * of all transactions in the package. May be unavailable if some inputs were not available or + * a transaction failure caused validation to terminate early. */ + std::optional<CFeeRate> m_package_feerate; explicit PackageMempoolAcceptResult(PackageValidationState state, std::map<const uint256, const MempoolAcceptResult>&& results) : m_state{state}, m_tx_results(std::move(results)) {} + explicit PackageMempoolAcceptResult(PackageValidationState state, CFeeRate feerate, + std::map<const uint256, const MempoolAcceptResult>&& results) + : m_state{state}, m_tx_results(std::move(results)), m_package_feerate{feerate} {} + /** Constructor to create a PackageMempoolAcceptResult from a single MempoolAcceptResult */ explicit PackageMempoolAcceptResult(const uint256& wtxid, const MempoolAcceptResult& result) : m_tx_results{ {wtxid, result} } {} diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 784a91e827..c1484c0a57 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -225,7 +225,7 @@ struct OutputGroup [[nodiscard]] CAmount GetSelectionWaste(const std::set<COutput>& inputs, CAmount change_cost, CAmount target, bool use_effective_value = true); -/** Chooose a random change target for each transaction to make it harder to fingerprint the Core +/** Choose a random change target for each transaction to make it harder to fingerprint the Core * wallet based on the change output values of transactions it creates. * The random value is between 50ksat and min(2 * payment_value, 1milsat) * When payment_value <= 25ksat, the value is just 50ksat. diff --git a/test/functional/rpc_misc.py b/test/functional/rpc_misc.py index 2f1796d7cc..f64aae7223 100755 --- a/test/functional/rpc_misc.py +++ b/test/functional/rpc_misc.py @@ -56,9 +56,6 @@ class RpcMiscTest(BitcoinTestFramework): self.log.info("test logging rpc and help") - # Test logging RPC returns the expected number of logging categories. - assert_equal(len(node.logging()), 27) - # Test toggling a logging category on/off/on with the logging RPC. assert_equal(node.logging()['qt'], True) node.logging(exclude=['qt']) diff --git a/test/lint/lint-qt.sh b/test/lint/lint-qt.sh deleted file mode 100755 index 2e77682aa2..0000000000 --- a/test/lint/lint-qt.sh +++ /dev/null @@ -1,20 +0,0 @@ -#!/usr/bin/env bash -# -# Copyright (c) 2018 The Bitcoin Core developers -# Distributed under the MIT software license, see the accompanying -# file COPYING or http://www.opensource.org/licenses/mit-license.php. -# -# Check for SIGNAL/SLOT connect style, removed since Qt4 support drop. - -export LC_ALL=C - -EXIT_CODE=0 - -OUTPUT=$(git grep -E '(SIGNAL|, ?SLOT)\(' -- src/qt) -if [[ ${OUTPUT} != "" ]]; then - echo "Use Qt5 connect style in:" - echo "$OUTPUT" - EXIT_CODE=1 -fi - -exit ${EXIT_CODE} |