diff options
| -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 | 8 | ||||
| -rw-r--r-- | src/policy/rbf.h | 28 | ||||
| -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 | 12 | ||||
| -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, 40 insertions, 40 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 e3d13ad972..83c1893c50 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -556,7 +556,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 d15946de21..6098caced9 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -65,7 +65,7 @@ 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_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. @@ -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 f3efcebbe0..09c495506a 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -19,7 +19,7 @@ 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_REPLACEMENT_CANDIDATES{100}; @@ -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_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 0ee905a77a..76e1bc9e31 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -604,7 +604,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 da1b35f255..800023e03c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -862,8 +862,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 @@ -930,7 +930,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 @@ -943,18 +943,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(); diff --git a/src/validation.h b/src/validation.h index df9146852f..9de3842565 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 43fdcee48d..43cbbf6590 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 e27cb1139e..63a2f7bcf9 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"}, std::string() + "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"}, std::string() + "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 20f0c3579c..2ad7806e18 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -90,7 +90,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; @@ -764,7 +764,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. |