diff --git a/src/test/fuzz/util/check_globals.cpp b/src/test/fuzz/util/check_globals.cpp index 6e7901dd98f3..1c29906932c5 100644 --- a/src/test/fuzz/util/check_globals.cpp +++ b/src/test/fuzz/util/check_globals.cpp @@ -19,6 +19,7 @@ struct CheckGlobalsImpl { g_seeded_g_prng_zero = false; g_used_system_time = false; SetMockTime(0s); + MockableSteadyClock::ClearMockTime(); } ~CheckGlobalsImpl() { diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h index f329de688a71..08d7351c0b83 100644 --- a/src/wallet/coincontrol.h +++ b/src/wallet/coincontrol.h @@ -116,6 +116,8 @@ class CCoinControl uint32_t m_version = DEFAULT_WALLET_TX_VERSION; //! Locktime std::optional m_locktime; + //! Save the previous locktime for replacements + std::optional m_previous_locktime; //! Caps weight of resulting tx std::optional m_max_tx_weight{std::nullopt}; diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 1a402c851faf..9c7180dcb5b2 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -312,6 +312,10 @@ Result CreateRateBumpTransaction(CWallet& wallet, const Txid& txid, const CCoinC // We cannot source new unconfirmed inputs(bip125 rule 2) new_coin_control.m_min_depth = 1; + // If no locktime is set, we save the previous one for anti fee sniping + if (!new_coin_control.m_locktime){ + new_coin_control.m_previous_locktime = wtx.tx->nLockTime; + } auto res = CreateTransaction(wallet, recipients, /*change_pos=*/std::nullopt, new_coin_control, false); if (!res) { diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index dba7b8829595..c17d09e814c9 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -1016,6 +1016,7 @@ void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast, // now we ensure code won't be written that makes assumptions about // nLockTime that preclude a fix later. if (IsCurrentForAntiFeeSniping(chain, block_hash)) { + uint32_t previous_locktime = tx.nLockTime; tx.nLockTime = block_height; // Secondly occasionally randomly pick a nLockTime even further back, so @@ -1023,7 +1024,15 @@ void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast, // e.g. high-latency mix networks and some CoinJoin implementations, have // better privacy. if (rng_fast.randrange(10) == 0) { - tx.nLockTime = std::max(0, int(tx.nLockTime) - int(rng_fast.randrange(100))); + // If a previous lockitme is passed (like in the bump fee case), the + // backdating is limited betwen the current height and the previous lockitme + if (previous_locktime > 0){ + // To avoid issues with the randrange, locktime_range cannot be 0 + int locktime_range = std::max(1, std::min(100, int(block_height - previous_locktime))); + tx.nLockTime = std::max(int(previous_locktime), int(tx.nLockTime) - int(rng_fast.randrange(locktime_range))); + }else{ + tx.nLockTime = std::max(0, int(tx.nLockTime) - int(rng_fast.randrange(100))); + } } } else { // If our chain is lagging behind, we can't discourage fee sniping nor help @@ -1317,10 +1326,18 @@ static util::Result CreateTransactionInternal( txNew.vin.back().scriptWitness = *scripts.second; } } + // txNew.nLockTime = 0; if (coin_control.m_locktime) { txNew.nLockTime = coin_control.m_locktime.value(); // If we have a locktime set, we can't use anti-fee-sniping use_anti_fee_sniping = false; + } else if (coin_control.m_previous_locktime.has_value()) { + // If the previous locktime is time-based, we keep the same locktime, + //and we don't apply fee sniping + if (coin_control.m_previous_locktime >= LOCKTIME_THRESHOLD){ + use_anti_fee_sniping = false; + } + txNew.nLockTime = coin_control.m_previous_locktime.value(); } if (use_anti_fee_sniping) { DiscourageFeeSniping(txNew, rng_fast, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight()); diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index b07425c1a26f..251ff276d9d3 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -30,6 +30,7 @@ assert_raises_rpc_error, get_fee, find_vout_for_address, + assert_greater_than_or_equal ) from test_framework.wallet import MiniWallet @@ -114,6 +115,8 @@ def run_test(self): # Context independent tests test_feerate_checks_replaced_outputs(self, rbf_node, peer_node) test_bumpfee_with_feerate_ignores_walletincrementalrelayfee(self, rbf_node, peer_node) + test_bumpfee_dont_backdate(self, rbf_node) + test_bumpfee_timebased_locktime(self, rbf_node) def test_invalid_parameters(self, rbf_node, peer_node, dest_address): self.log.info('Test invalid parameters') @@ -828,6 +831,54 @@ def test_bumpfee_with_feerate_ignores_walletincrementalrelayfee(self, rbf_node, rbf_node.bumpfee(tx["txid"], {"fee_rate": 2.1}) self.clear_mempool() +def test_bumpfee_dont_backdate(self, rbf_node): + self.log.info('Test to check bumpfee do not backdate the locktime') + + rbf_node.createwallet("backdating_rbf") + wallet = rbf_node.get_wallet_rpc("backdating_rbf") + self.generatetoaddress(rbf_node, 101, wallet.getnewaddress()) + + current_height = rbf_node.getblockchaininfo()["blocks"] + # The original tx has an older locktime so we can differentiate when the replacement backdates + original_locktime = current_height - 50 + replaced_locktime = current_height + + # Exti the loop when the locktime backdates + while current_height == replaced_locktime: + # Original transaction with locktime at the current height + tx = wallet.send(outputs={wallet.getnewaddress(): 9}, fee_rate=2, locktime=original_locktime) + + # Replacement with higher fee_rate + change_addr = get_change_address(tx["txid"], wallet)[0] + bumped = wallet.bumpfee(txid=tx["txid"], options={"fee_rate":5, "outputs": [{change_addr: 10}]}) + + replaced_locktime = rbf_node.getrawtransaction(bumped["txid"],True)["locktime"] + + # Clear mempool to create another transaction to replace + self.clear_mempool() + + # Even when backdating, the replaced tx must always have locktime >= than the original tx + assert_greater_than_or_equal(replaced_locktime, original_locktime) + + wallet.unloadwallet() + + def test_bumpfee_timebased_locktime(self, rbf_node): + self.log.info(f"Test to check bumpfee don't anti fee snipe if original tx had time-based locktime") + + rbf_node.createwallet("rbf") + wallet = rbf_node.get_wallet_rpc("rbf") + self.generatetoaddress(rbf_node, 101, wallet.getnewaddress()) + + original_locktime = 500000000 + tx = wallet.send(outputs={wallet.getnewaddress(): 9}, fee_rate=2, locktime=original_locktime) + hex_tx = rbf_node.getrawtransaction(tx["txid"]) + + # Replacement with higher fee_rate + change_addr = get_change_address(tx["txid"], wallet)[0] + bumped = wallet.bumpfee(txid=tx["txid"], options={"fee_rate":5, "outputs": [{change_addr: 10}]}) + + locktime = rbf_node.getrawtransaction(bumped["txid"],True)["locktime"] + assert_equal(original_locktime, locktime) if __name__ == "__main__": BumpFeeTest(__file__).main()