diff options
author | fanquake <fanquake@gmail.com> | 2022-08-22 10:33:12 +0100 |
---|---|---|
committer | fanquake <fanquake@gmail.com> | 2022-08-22 10:35:26 +0100 |
commit | c5f0cbefa369b0e4d99a4f871e6334955d537c1f (patch) | |
tree | f1cc643164f8d5e2b9ed828a6554663f8f7711c8 | |
parent | 607d5a46aa0f5053d8643a3e2c31a69bfdeb6e9f (diff) | |
parent | 1dc03dda05e9dce128e57f05bb7b1bb02b3cfb9e (diff) | |
download | bitcoin-c5f0cbefa369b0e4d99a4f871e6334955d537c1f.tar.xz |
Merge bitcoin/bitcoin#25775: docs: remove non-signaling mentions of BIP125
1dc03dda05e9dce128e57f05bb7b1bb02b3cfb9e [doc] remove non-signaling mentions of BIP125 (glozow)
32024d40f03fbf47c64d814fa5f2c2a73ec14cb7 scripted-diff: remove mention of BIP125 from non-signaling var names (glozow)
Pull request description:
We have pretty thorough documentation of our RBF policy in doc/policy/mempool-replacements.md. It enumerates each rule with several sentences of rationale. Also, each rule pretty much has its own function (3 and 4 share one), with extensive comments. The doc states explicitly that our rules are similar but differ from BIP125, and contains a record of historical changes to RBF policy.
We should not use "BIP125" as synonymous with our RBF policy because:
- Our RBF policy is different from what is specified in BIP125, for example:
- the BIP does not mention our rule about the replacement feerate being higher (our Rule 6)
- the BIP uses minimum relay feerate for Rule 4, while we have used incremental relay feerate since #9380
- the "inherited signaling" question (CVE-2021-31876). Call it discrepancy, ambiguous wording, doc misinterpretation, or implementation details, I would recommend users refer to doc/policy/mempool-replacements.md
- the signaling policy is configurable, see #25353
- Our RBF policy may change further
- We have already marked BIP125 as only "partially implemented" in docs/bips.md since 1fd49eb498c75a1d14193bb736d195a3dc75ae12
- See comments from people who are not me recently:
- https://github.com/bitcoin/bitcoin/pull/25038#discussion_r909507429
- https://github.com/bitcoin/bitcoin/pull/25575#issuecomment-1179519204
This PR removes all non-signaling mentions of BIP125 (if people feel strongly, we can remove all mentions of BIP125 period). It may be useful to refer to the concept of "tx opts in to RBF if it has at least one nSequence less than (0xffffffff - 1)" as "BIP125 signaling" because:
- It is succint.
- It has already been widely marketed as BIP125 opt-in signaling.
- Our API uses it when referring to signaling (e.g. getmempoolentry["bip125-replaceable"] and wallet error message "not BIP 125 replaceable"). Changing those is more invasive.
- If/when we have other ways to signal in the future, we can disambiguate them this way. See #25038 which proposes another way of signaling, and where I pulled these commits from.
Alternatives:
- Changing our policy to match BIP125. This doesn't make sense as, for example, we would have to remove the requirement that a replacement tx has a higher feerate (Rule 6).
- Changing BIP125 to match what we have. This doesn't make sense as it would be a significant change to a BIP years after it was finalized and already used as a spec to implement RBF in other places.
- Document our policy as a new BIP and give it a number. This might make sense if we don't expect things to change a lot, and can be done as a next step.
ACKs for top commit:
darosior:
ACK 1dc03dda05e9dce128e57f05bb7b1bb02b3cfb9e
ariard:
ACK 1dc03dda
t-bast:
ACK https://github.com/bitcoin/bitcoin/commit/1dc03dda05e9dce128e57f05bb7b1bb02b3cfb9e
Tree-SHA512: a3adc2039ec5785892d230ec442e50f47f7062717392728152bbbe27ce1c564141f85253143f53cb44e1331cf47476d74f5d2f4b3cd873fc3433d7a0aa783e02
-rw-r--r-- | doc/policy/packages.md | 2 | ||||
-rw-r--r-- | src/init.cpp | 2 | ||||
-rw-r--r-- | src/node/mempool_args.cpp | 2 | ||||
-rw-r--r-- | src/policy/policy.h | 2 | ||||
-rw-r--r-- | src/policy/rbf.cpp | 12 | ||||
-rw-r--r-- | src/policy/rbf.h | 30 | ||||
-rw-r--r-- | src/rpc/mempool.cpp | 2 | ||||
-rw-r--r-- | src/rpc/net.cpp | 2 | ||||
-rw-r--r-- | src/txmempool.h | 2 | ||||
-rw-r--r-- | src/validation.cpp | 30 | ||||
-rw-r--r-- | src/validation.h | 2 | ||||
-rw-r--r-- | src/wallet/feebumper.cpp | 4 | ||||
-rw-r--r-- | src/wallet/rpc/spend.cpp | 4 | ||||
-rw-r--r-- | src/wallet/wallet.h | 4 | ||||
-rwxr-xr-x | test/functional/feature_rbf.py | 4 |
15 files changed, 52 insertions, 52 deletions
diff --git a/doc/policy/packages.md b/doc/policy/packages.md index 03c26da86b..274854ddf9 100644 --- a/doc/policy/packages.md +++ b/doc/policy/packages.md @@ -33,7 +33,7 @@ The following rules are enforced for all packages: * Packages cannot have conflicting transactions, i.e. no two transactions in a package can spend the same inputs. Packages cannot have duplicate transactions. (#20833) -* No transaction in a package can conflict with a mempool transaction. BIP125 Replace By Fee is +* No transaction in a package can conflict with a mempool transaction. Replace By Fee is currently disabled for packages. (#20833) - Package RBF may be enabled in the future. diff --git a/src/init.cpp b/src/init.cpp index 480b8c7705..137d98026e 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -560,7 +560,7 @@ void SetupServerArgs(ArgsManager& argsman) SetupChainParamsBaseOptions(argsman); argsman.AddArg("-acceptnonstdtxn", strprintf("Relay and mine \"non-standard\" transactions (%sdefault: %u)", "testnet/regtest only; ", !testnetChainParams->RequireStandard()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::NODE_RELAY); - argsman.AddArg("-incrementalrelayfee=<amt>", strprintf("Fee rate (in %s/kvB) used to define cost of relay, used for mempool limiting and BIP 125 replacement. (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_INCREMENTAL_RELAY_FEE)), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::NODE_RELAY); + argsman.AddArg("-incrementalrelayfee=<amt>", strprintf("Fee rate (in %s/kvB) used to define cost of relay, used for mempool limiting and replacement policy. (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_INCREMENTAL_RELAY_FEE)), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::NODE_RELAY); argsman.AddArg("-dustrelayfee=<amt>", strprintf("Fee rate (in %s/kvB) used to define dust, the value of an output such that it will cost more than its value in fees at this fee rate to spend it. (default: %s)", CURRENCY_UNIT, FormatMoney(DUST_RELAY_TX_FEE)), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::NODE_RELAY); argsman.AddArg("-bytespersigop", strprintf("Equivalent bytes per sigop in transactions for relay and mining (default: %u)", DEFAULT_BYTES_PER_SIGOP), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY); argsman.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY); diff --git a/src/node/mempool_args.cpp b/src/node/mempool_args.cpp index 60993f1d8d..cb2466804a 100644 --- a/src/node/mempool_args.cpp +++ b/src/node/mempool_args.cpp @@ -45,7 +45,7 @@ std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& argsman, con if (auto hours = argsman.GetIntArg("-mempoolexpiry")) mempool_opts.expiry = std::chrono::hours{*hours}; - // incremental relay fee sets the minimum feerate increase necessary for BIP 125 replacement in the mempool + // incremental relay fee sets the minimum feerate increase necessary for replacement in the mempool // and the amount the mempool min fee increases above the feerate of txs evicted due to mempool limiting. if (argsman.IsArgSet("-incrementalrelayfee")) { if (std::optional<CAmount> inc_relay_fee = ParseMoney(argsman.GetArg("-incrementalrelayfee", ""))) { diff --git a/src/policy/policy.h b/src/policy/policy.h index 3d2660b081..29764ea2d9 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -31,7 +31,7 @@ static constexpr unsigned int MIN_STANDARD_TX_NONWITNESS_SIZE{82}; static constexpr unsigned int MAX_P2SH_SIGOPS{15}; /** The maximum number of sigops we're willing to relay/mine in a single tx */ static constexpr unsigned int MAX_STANDARD_TX_SIGOPS_COST{MAX_BLOCK_SIGOPS_COST/5}; -/** Default for -incrementalrelayfee, which sets the minimum feerate increase for mempool limiting or BIP 125 replacement **/ +/** Default for -incrementalrelayfee, which sets the minimum feerate increase for mempool limiting or replacement **/ static constexpr unsigned int DEFAULT_INCREMENTAL_RELAY_FEE{1000}; /** Default for -bytespersigop */ static constexpr unsigned int DEFAULT_BYTES_PER_SIGOP{20}; diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index e25f5c7c5b..6098caced9 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -65,15 +65,15 @@ std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx, uint64_t nConflictingCount = 0; for (const auto& mi : iters_conflicting) { nConflictingCount += mi->GetCountWithDescendants(); - // BIP125 Rule #5: don't consider replacing more than MAX_BIP125_REPLACEMENT_CANDIDATES + // Rule #5: don't consider replacing more than MAX_REPLACEMENT_CANDIDATES // entries from the mempool. This potentially overestimates the number of actual // descendants (i.e. if multiple conflicts share a descendant, it will be counted multiple // times), but we just want to be conservative to avoid doing too much work. - if (nConflictingCount > MAX_BIP125_REPLACEMENT_CANDIDATES) { + if (nConflictingCount > MAX_REPLACEMENT_CANDIDATES) { return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", txid.ToString(), nConflictingCount, - MAX_BIP125_REPLACEMENT_CANDIDATES); + MAX_REPLACEMENT_CANDIDATES); } } // Calculate the set of all transactions that would have to be evicted. @@ -96,7 +96,7 @@ std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx, } for (unsigned int j = 0; j < tx.vin.size(); j++) { - // BIP125 Rule #2: We don't want to accept replacements that require low feerate junk to be + // Rule #2: We don't want to accept replacements that require low feerate junk to be // mined first. Ideally we'd keep track of the ancestor feerates and make the decision // based on that, but for now requiring all new inputs to be confirmed works. // @@ -162,7 +162,7 @@ std::optional<std::string> PaysForRBF(CAmount original_fees, CFeeRate relay_fee, const uint256& txid) { - // BIP125 Rule #3: The replacement fees must be greater than or equal to fees of the + // Rule #3: The replacement fees must be greater than or equal to fees of the // transactions it replaces, otherwise the bandwidth used by those conflicting transactions // would not be paid for. if (replacement_fees < original_fees) { @@ -170,7 +170,7 @@ std::optional<std::string> PaysForRBF(CAmount original_fees, txid.ToString(), FormatMoney(replacement_fees), FormatMoney(original_fees)); } - // BIP125 Rule #4: The new transaction must pay for its own bandwidth. Otherwise, we have a DoS + // Rule #4: The new transaction must pay for its own bandwidth. Otherwise, we have a DoS // vector where attackers can cause a transaction to be replaced (and relayed) repeatedly by // increasing the fee by tiny amounts. CAmount additional_fees = replacement_fees - original_fees; diff --git a/src/policy/rbf.h b/src/policy/rbf.h index 07f68c8fd4..09c495506a 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -19,9 +19,9 @@ class CFeeRate; class uint256; -/** Maximum number of transactions that can be replaced by BIP125 RBF (Rule #5). This includes all +/** Maximum number of transactions that can be replaced by RBF Rule #5. This includes all * mempool conflicts and their descendants. */ -static constexpr uint32_t MAX_BIP125_REPLACEMENT_CANDIDATES{100}; +static constexpr uint32_t MAX_REPLACEMENT_CANDIDATES{100}; /** The rbf state of unconfirmed transactions */ enum class RBFTransactionState { @@ -47,24 +47,25 @@ enum class RBFTransactionState { RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(pool.cs); RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx); -/** Get all descendants of iters_conflicting. Also enforce BIP125 Rule #5, "The number of original - * transactions to be replaced and their descendant transactions which will be evicted from the - * mempool must not exceed a total of 100 transactions." Quit as early as possible. There cannot be - * more than MAX_BIP125_REPLACEMENT_CANDIDATES potential entries. +/** Get all descendants of iters_conflicting. Checks that there are no more than + * MAX_REPLACEMENT_CANDIDATES potential entries. May overestimate if the entries in + * iters_conflicting have overlapping descendants. * @param[in] iters_conflicting The set of iterators to mempool entries. * @param[out] all_conflicts Populated with all the mempool entries that would be replaced, - * which includes descendants of iters_conflicting. Not cleared at - * the start; any existing mempool entries will remain in the set. - * @returns an error message if Rule #5 is broken, otherwise a std::nullopt. + * which includes iters_conflicting and all entries' descendants. + * Not cleared at the start; any existing mempool entries will + * remain in the set. + * @returns an error message if MAX_REPLACEMENT_CANDIDATES may be exceeded, otherwise a std::nullopt. */ std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& pool, const CTxMemPool::setEntries& iters_conflicting, CTxMemPool::setEntries& all_conflicts) EXCLUSIVE_LOCKS_REQUIRED(pool.cs); -/** BIP125 Rule #2: "The replacement transaction may only include an unconfirmed input if that input - * was included in one of the original transactions." - * @returns error message if Rule #2 is broken, otherwise std::nullopt. */ +/** The replacement transaction may only include an unconfirmed input if that input was included in + * one of the original transactions. + * @returns error message if tx spends unconfirmed inputs not also spent by iters_conflicting, + * otherwise std::nullopt. */ std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool& pool, const CTxMemPool::setEntries& iters_conflicting) EXCLUSIVE_LOCKS_REQUIRED(pool.cs); @@ -90,9 +91,8 @@ std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& std::optional<std::string> PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting, CFeeRate replacement_feerate, const uint256& txid); -/** Enforce BIP125 Rule #3 "The replacement transaction pays an absolute fee of at least the sum - * paid by the original transactions." Enforce BIP125 Rule #4 "The replacement transaction must also - * pay for its own bandwidth at or above the rate set by the node's minimum relay fee setting." +/** The replacement transaction must pay more fees than the original transactions. The additional + * fees must pay for the replacement's bandwidth at or above the incremental relay feerate. * @param[in] original_fees Total modified fees of original transaction(s). * @param[in] replacement_fees Total modified fees of replacement transaction(s). * @param[in] replacement_vsize Total virtual size of replacement transaction(s). diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 02b51ce0a0..e0b32176e8 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -690,7 +690,7 @@ static RPCHelpMan getmempoolinfo() {RPCResult::Type::NUM, "maxmempool", "Maximum memory usage for the mempool"}, {RPCResult::Type::STR_AMOUNT, "mempoolminfee", "Minimum fee rate in " + CURRENCY_UNIT + "/kvB for tx to be accepted. Is the maximum of minrelaytxfee and minimum mempool fee"}, {RPCResult::Type::STR_AMOUNT, "minrelaytxfee", "Current minimum relay fee for transactions"}, - {RPCResult::Type::NUM, "incrementalrelayfee", "minimum fee rate increment for mempool limiting or BIP 125 replacement in " + CURRENCY_UNIT + "/kvB"}, + {RPCResult::Type::NUM, "incrementalrelayfee", "minimum fee rate increment for mempool limiting or replacement in " + CURRENCY_UNIT + "/kvB"}, {RPCResult::Type::NUM, "unbroadcastcount", "Current number of transactions that haven't passed initial broadcast yet"}, {RPCResult::Type::BOOL, "fullrbf", "True if the mempool accepts RBF without replaceability signaling inspection"}, }}, diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 3b234bc317..06f46040b8 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -605,7 +605,7 @@ static RPCHelpMan getnetworkinfo() }}, }}, {RPCResult::Type::NUM, "relayfee", "minimum relay fee rate for transactions in " + CURRENCY_UNIT + "/kvB"}, - {RPCResult::Type::NUM, "incrementalfee", "minimum fee rate increment for mempool limiting or BIP 125 replacement in " + CURRENCY_UNIT + "/kvB"}, + {RPCResult::Type::NUM, "incrementalfee", "minimum fee rate increment for mempool limiting or replacement in " + CURRENCY_UNIT + "/kvB"}, {RPCResult::Type::ARR, "localaddresses", "list of local addresses", { {RPCResult::Type::OBJ, "", "", diff --git a/src/txmempool.h b/src/txmempool.h index 73904cc370..d06816ba97 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -365,7 +365,7 @@ enum class MemPoolRemovalReason { * - a transaction which doesn't meet the minimum fee requirements. * - a new transaction that double-spends an input of a transaction already in * the pool where the new transaction does not meet the Replace-By-Fee - * requirements as defined in BIP 125. + * requirements as defined in doc/policy/mempool-replacements.md. * - a non-standard transaction. * * CTxMemPool::mapTx, and CTxMemPoolEntry bookkeeping: diff --git a/src/validation.cpp b/src/validation.cpp index dc90b53746..f4cae07b9e 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -449,7 +449,7 @@ public: /** Whether we allow transactions to replace mempool transactions by BIP125 rules. If false, * any transaction spending the same inputs as a transaction in the mempool is considered * a conflict. */ - const bool m_allow_bip125_replacement; + const bool m_allow_replacement; /** When true, the mempool will not be trimmed when individual transactions are submitted in * Finalize(). Instead, limits should be enforced at the end to ensure the package is not * partially submitted. @@ -469,7 +469,7 @@ public: /* m_bypass_limits */ bypass_limits, /* m_coins_to_uncache */ coins_to_uncache, /* m_test_accept */ test_accept, - /* m_allow_bip125_replacement */ true, + /* m_allow_replacement */ true, /* m_package_submission */ false, /* m_package_feerates */ false, }; @@ -483,7 +483,7 @@ public: /* m_bypass_limits */ false, /* m_coins_to_uncache */ coins_to_uncache, /* m_test_accept */ true, - /* m_allow_bip125_replacement */ false, + /* m_allow_replacement */ false, /* m_package_submission */ false, // not submitting to mempool /* m_package_feerates */ false, }; @@ -497,7 +497,7 @@ public: /* m_bypass_limits */ false, /* m_coins_to_uncache */ coins_to_uncache, /* m_test_accept */ false, - /* m_allow_bip125_replacement */ false, + /* m_allow_replacement */ false, /* m_package_submission */ true, /* m_package_feerates */ true, }; @@ -510,7 +510,7 @@ public: /* 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_allow_replacement */ true, /* m_package_submission */ false, /* m_package_feerates */ false, // only 1 transaction }; @@ -524,7 +524,7 @@ public: bool bypass_limits, std::vector<COutPoint>& coins_to_uncache, bool test_accept, - bool allow_bip125_replacement, + bool allow_replacement, bool package_submission, bool package_feerates) : m_chainparams{chainparams}, @@ -532,7 +532,7 @@ public: 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_allow_replacement{allow_replacement}, m_package_submission{package_submission}, m_package_feerates{package_feerates} { @@ -731,7 +731,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) { const CTransaction* ptxConflicting = m_pool.GetConflictTx(txin.prevout); if (ptxConflicting) { - if (!args.m_allow_bip125_replacement) { + if (!args.m_allow_replacement) { // Transaction conflicts with a mempool tx, but we're not allowing replacements. return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "bip125-replacement-disallowed"); } @@ -861,8 +861,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Specifically, the subset of RBF transactions which we allow despite chain limits are those which // conflict directly with exactly one other transaction (but may evict children of said transaction), // and which are not adding any new mempool dependencies. Note that the "no new mempool dependencies" - // check is accomplished later, so we don't bother doing anything about it here, but if BIP 125 is - // amended, we may need to move that check to here instead of removing it wholesale. + // check is accomplished later, so we don't bother doing anything about it here, but if our + // policy changes, we may need to move that check to here instead of removing it wholesale. // // Such transactions are clearly not merging any existing packages, so we are only concerned with // ensuring that (a) no package is growing past the package size (not count) limits and (b) we are @@ -929,7 +929,7 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) TxValidationState& state = ws.m_state; CFeeRate newFeeRate(ws.m_modified_fees, ws.m_vsize); - // The replacement transaction must have a higher feerate than its direct conflicts. + // Enforce Rule #6. The replacement transaction must have a higher feerate than its direct conflicts. // - The motivation for this check is to ensure that the replacement transaction is preferable for // block-inclusion, compared to what would be removed from the mempool. // - This logic predates ancestor feerate-based transaction selection, which is why it doesn't @@ -942,18 +942,18 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string); } - // Calculate all conflicting entries and enforce BIP125 Rule #5. + // Calculate all conflicting entries and enforce Rule #5. if (const auto err_string{GetEntriesForConflicts(tx, m_pool, ws.m_iters_conflicting, ws.m_all_conflicting)}) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too many potential replacements", *err_string); } - // Enforce BIP125 Rule #2. + // Enforce Rule #2. if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, ws.m_iters_conflicting)}) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "replacement-adds-unconfirmed", *err_string); } // Check if it's economically rational to mine this transaction rather than the ones it - // replaces and pays for its own relay fees. Enforce BIP125 Rules #3 and #4. + // replaces and pays for its own relay fees. Enforce Rules #3 and #4. for (CTxMemPool::txiter it : ws.m_all_conflicting) { ws.m_conflicting_fees += it->GetModifiedFee(); ws.m_conflicting_size += it->GetTxSize(); @@ -1224,7 +1224,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // package to spend. Since we already checked conflicts in the package and we don't allow // replacements, we don't need to track the coins spent. Note that this logic will need to be // updated if package replace-by-fee is allowed in the future. - assert(!args.m_allow_bip125_replacement); + assert(!args.m_allow_replacement); m_viewmempool.PackageAddTransaction(ws.m_ptx); } diff --git a/src/validation.h b/src/validation.h index 5b71318687..b1b6689c46 100644 --- a/src/validation.h +++ b/src/validation.h @@ -147,7 +147,7 @@ struct MempoolAcceptResult { const TxValidationState m_state; // The following fields are only present when m_result_type = ResultType::VALID or MEMPOOL_ENTRY - /** Mempool transactions replaced by the tx per BIP 125 rules. */ + /** Mempool transactions replaced by the tx. */ const std::optional<std::list<CTransactionRef>> m_replaced_transactions; /** Virtual size as used by the mempool, calculated using serialized size and sigops. */ const std::optional<int64_t> m_vsize; diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 1d741cda6e..6d7bb299cc 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -128,8 +128,8 @@ static CFeeRate EstimateFeeRate(const CWallet& wallet, const CWalletTx& wtx, con // WALLET_INCREMENTAL_RELAY_FEE value to future proof against changes to // network wide policy for incremental relay fee that our node may not be // aware of. This ensures we're over the required relay fee rate - // (BIP 125 rule 4). The replacement tx will be at least as large as the - // original tx, so the total fee will be greater (BIP 125 rule 3) + // (Rule 4). The replacement tx will be at least as large as the + // original tx, so the total fee will be greater (Rule 3) CFeeRate node_incremental_relay_fee = wallet.chain().relayIncrementalFee(); CFeeRate wallet_incremental_relay_fee = CFeeRate(WALLET_INCREMENTAL_RELAY_FEE); feerate += std::max(node_incremental_relay_fee, wallet_incremental_relay_fee); diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 66adc1d327..92ab8cd085 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -224,7 +224,7 @@ RPCHelpMan sendtoaddress() "transaction, just kept in your wallet."}, {"subtractfeefromamount", RPCArg::Type::BOOL, RPCArg::Default{false}, "The fee will be deducted from the amount being sent.\n" "The recipient will receive less bitcoins than you enter in the amount field."}, - {"replaceable", RPCArg::Type::BOOL, RPCArg::DefaultHint{"wallet default"}, "Allow this transaction to be replaced by a transaction with higher fees via BIP 125"}, + {"replaceable", RPCArg::Type::BOOL, RPCArg::DefaultHint{"wallet default"}, "Signal that this transaction can replaced by a transaction (BIP 125)"}, {"conf_target", RPCArg::Type::NUM, RPCArg::DefaultHint{"wallet -txconfirmtarget"}, "Confirmation target in blocks"}, {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, "The fee estimate mode, must be one of (case insensitive):\n" "\"" + FeeModes("\"\n\"") + "\""}, @@ -333,7 +333,7 @@ RPCHelpMan sendmany() {"address", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Subtract fee from this address"}, }, }, - {"replaceable", RPCArg::Type::BOOL, RPCArg::DefaultHint{"wallet default"}, "Allow this transaction to be replaced by a transaction with higher fees via BIP 125"}, + {"replaceable", RPCArg::Type::BOOL, RPCArg::DefaultHint{"wallet default"}, "Signal that this transaction can replaced by a transaction (BIP 125)"}, {"conf_target", RPCArg::Type::NUM, RPCArg::DefaultHint{"wallet -txconfirmtarget"}, "Confirmation target in blocks"}, {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, "The fee estimate mode, must be one of (case insensitive):\n" "\"" + FeeModes("\"\n\"") + "\""}, diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 934811dee1..9281045c21 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -93,7 +93,7 @@ static const CAmount DEFAULT_CONSOLIDATE_FEERATE{10000}; // 10 sat/vbyte static const CAmount DEFAULT_MAX_AVOIDPARTIALSPEND_FEE = 0; //! discourage APS fee higher than this amount constexpr CAmount HIGH_APS_FEE{COIN / 10000}; -//! minimum recommended increment for BIP 125 replacement txs +//! minimum recommended increment for replacement txs static const CAmount WALLET_INCREMENTAL_RELAY_FEE = 5000; //! Default for -spendzeroconfchange static const bool DEFAULT_SPEND_ZEROCONF_CHANGE = true; @@ -768,7 +768,7 @@ public: /* Mark a transaction (and it in-wallet descendants) as abandoned so its inputs may be respent. */ bool AbandonTransaction(const uint256& hashTx); - /** Mark a transaction as replaced by another transaction (e.g., BIP 125). */ + /** Mark a transaction as replaced by another transaction. */ bool MarkReplaced(const uint256& originalHash, const uint256& newHash); /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */ diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py index 2237a4171e..7603248ae5 100755 --- a/test/functional/feature_rbf.py +++ b/test/functional/feature_rbf.py @@ -386,7 +386,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): def test_too_many_replacements_with_default_mempool_params(self): """ - Test rule 5 of BIP125 (do not allow replacements that cause more than 100 + Test rule 5 (do not allow replacements that cause more than 100 evictions) without having to rely on non-default mempool parameters. In order to do this, create a number of "root" UTXOs, and then hang @@ -405,7 +405,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): # limit; 10 works. num_tx_graphs = 10 - # (Number of transactions per graph, BIP125 rule 5 failure expected) + # (Number of transactions per graph, rule 5 failure expected) cases = [ # Test the base case of evicting fewer than MAX_REPLACEMENT_LIMIT # transactions. |