From 6825ae0c786dc5647395380ffee7977d90df64e6 Mon Sep 17 00:00:00 2001 From: ishaanam Date: Sun, 26 Nov 2023 13:08:56 -0500 Subject: [PATCH 1/2] wallet, rpc: add anti-fee-sniping to `send` and `sendall` --- src/wallet/rpc/spend.cpp | 22 ++++++++++++++++---- src/wallet/spend.cpp | 6 +----- src/wallet/spend.h | 6 ++++++ test/functional/wallet_import_rescan.py | 1 + test/functional/wallet_rescan_unconfirmed.py | 2 +- 5 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index ac2a4826f05..068bab009c8 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -95,8 +95,21 @@ std::set InterpretSubtractFeeFromOutputInstructions(const UniValue& sffo_in return sffo_set; } -static UniValue FinishTransaction(const std::shared_ptr pwallet, const UniValue& options, const CMutableTransaction& rawTx) +static UniValue FinishTransaction(const std::shared_ptr pwallet, const UniValue& options, CMutableTransaction& rawTx) { + bool can_anti_fee_snipe = !options.exists("locktime"); + + for (const CTxIn& tx_in : rawTx.vin) { + // Checks sequence values consistent with DiscourageFeeSniping + can_anti_fee_snipe &= (tx_in.nSequence == CTxIn::MAX_SEQUENCE_NONFINAL || tx_in.nSequence == MAX_BIP125_RBF_SEQUENCE); + } + + if (can_anti_fee_snipe) { + LOCK(pwallet->cs_wallet); + FastRandomContext rng_fast; + DiscourageFeeSniping(rawTx, rng_fast, pwallet->chain(), pwallet->GetLastBlockHash(), pwallet->GetLastBlockHeight()); + } + // Make a blank psbt PartiallySignedTransaction psbtx(rawTx); @@ -1229,7 +1242,7 @@ RPCHelpMan send() "Remember to convert serialized sizes to weight units when necessary."}, }, }, - {"locktime", RPCArg::Type::NUM, RPCArg::Default{0}, "Raw locktime. Non-0 value also locktime-activates inputs"}, + {"locktime", RPCArg::Type::NUM, RPCArg::DefaultHint{"locktime close to block height to prevent fee sniping"}, "Raw locktime. Non-0 value also locktime-activates inputs"}, {"lock_unspents", RPCArg::Type::BOOL, RPCArg::Default{false}, "Lock selected unspent outputs"}, {"psbt", RPCArg::Type::BOOL, RPCArg::DefaultHint{"automatic"}, "Always return a PSBT, implies add_to_wallet=false."}, {"subtract_fee_from_outputs", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "Outputs to subtract the fee from, specified as integer indices.\n" @@ -1293,7 +1306,8 @@ RPCHelpMan send() rawTx.vout.clear(); auto txr = FundTransaction(*pwallet, rawTx, recipients, options, coin_control, /*override_min_fee=*/false); - return FinishTransaction(pwallet, options, CMutableTransaction(*txr.tx)); + CMutableTransaction tx = CMutableTransaction(*txr.tx); + return FinishTransaction(pwallet, options, tx); } }; } @@ -1341,7 +1355,7 @@ RPCHelpMan sendall() }, }, }, - {"locktime", RPCArg::Type::NUM, RPCArg::Default{0}, "Raw locktime. Non-0 value also locktime-activates inputs"}, + {"locktime", RPCArg::Type::NUM, RPCArg::DefaultHint{"locktime close to block height to prevent fee sniping"}, "Raw locktime. Non-0 value also locktime-activates inputs"}, {"lock_unspents", RPCArg::Type::BOOL, RPCArg::Default{false}, "Lock selected unspent outputs"}, {"psbt", RPCArg::Type::BOOL, RPCArg::DefaultHint{"automatic"}, "Always return a PSBT, implies add_to_wallet=false."}, {"send_max", RPCArg::Type::BOOL, RPCArg::Default{false}, "When true, only use UTXOs that can pay for their own fees to maximize the output amount. When 'false' (default), no UTXO is left behind. send_max is incompatible with providing specific inputs."}, diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 4cbcfdb60f0..b41cc1413c7 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -916,11 +916,7 @@ static bool IsCurrentForAntiFeeSniping(interfaces::Chain& chain, const uint256& return true; } -/** - * Set a height-based locktime for new transactions (uses the height of the - * current chain tip unless we are not synced with the current chain - */ -static void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast, +void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast, interfaces::Chain& chain, const uint256& block_hash, int block_height) { // All inputs must be added by now diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 62a7b4e4c89..26a063dceb6 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -213,6 +213,12 @@ struct CreatedTransactionResult : tx(_tx), fee(_fee), fee_calc(_fee_calc), change_pos(_change_pos) {} }; +/** + * Set a height-based locktime for new transactions (uses the height of the + * current chain tip unless we are not synced with the current chain + */ +void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast, interfaces::Chain& chain, const uint256& block_hash, int block_height); + /** * Create a new transaction paying the recipients with a set of coins * selected by SelectCoins(); Also create the change output, when needed diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py index 2a9435b3706..6d5854fa4b9 100755 --- a/test/functional/wallet_import_rescan.py +++ b/test/functional/wallet_import_rescan.py @@ -300,6 +300,7 @@ class ImportRescanTest(BitcoinTestFramework): add_to_wallet=False, inputs=[unspent_txid_map[variant.initial_txid]], outputs=[{ADDRESS_BCRT1_UNSPENDABLE : variant.initial_amount}], + locktime=0, subtract_fee_from_outputs=[0] ) variant.child_txid = child["txid"] diff --git a/test/functional/wallet_rescan_unconfirmed.py b/test/functional/wallet_rescan_unconfirmed.py index ad9fa081c21..e02d8116069 100755 --- a/test/functional/wallet_rescan_unconfirmed.py +++ b/test/functional/wallet_rescan_unconfirmed.py @@ -57,7 +57,7 @@ class WalletRescanUnconfirmed(BitcoinTestFramework): # The only UTXO available to spend is tx_parent_to_reorg. assert_equal(len(w0_utxos), 1) assert_equal(w0_utxos[0]["txid"], tx_parent_to_reorg["txid"]) - tx_child_unconfirmed_sweep = w0.sendall([ADDRESS_BCRT1_UNSPENDABLE]) + tx_child_unconfirmed_sweep = w0.sendall(recipients=[ADDRESS_BCRT1_UNSPENDABLE], options={"locktime":0}) assert tx_child_unconfirmed_sweep["txid"] in node.getrawmempool() node.syncwithvalidationinterfacequeue() From b11d00d54ed3315901de1f53885433d864a93019 Mon Sep 17 00:00:00 2001 From: ishaanam Date: Sun, 26 Nov 2023 13:43:17 -0500 Subject: [PATCH 2/2] test: test sendall and send do anti-fee-sniping --- test/functional/wallet_send.py | 5 +++++ test/functional/wallet_sendall.py | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py index 0a0a8dba0da..7e8e2412b65 100755 --- a/test/functional/wallet_send.py +++ b/test/functional/wallet_send.py @@ -142,7 +142,12 @@ class WalletSendTest(BitcoinTestFramework): return if locktime: + assert_equal(from_wallet.gettransaction(txid=res["txid"], verbose=True)["decoded"]["locktime"], locktime) return res + else: + if add_to_wallet: + decoded_tx = from_wallet.gettransaction(txid=res["txid"], verbose=True)["decoded"] + assert_greater_than(decoded_tx["locktime"], from_wallet.getblockcount() - 100) if from_wallet.getwalletinfo()["private_keys_enabled"] and not include_watching: assert_equal(res["complete"], True) diff --git a/test/functional/wallet_sendall.py b/test/functional/wallet_sendall.py index 1d308c225d9..b8ae6e455ad 100755 --- a/test/functional/wallet_sendall.py +++ b/test/functional/wallet_sendall.py @@ -6,6 +6,7 @@ from decimal import Decimal, getcontext +from test_framework.messages import SEQUENCE_FINAL from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, @@ -437,6 +438,26 @@ class SendallTest(BitcoinTestFramework): assert_greater_than(higher_parent_feerate_amount, lower_parent_feerate_amount) + @cleanup + def sendall_anti_fee_sniping(self): + self.log.info("Testing sendall does anti-fee-sniping when locktime is not specified") + self.add_utxos([10,11]) + tx_from_wallet = self.test_sendall_success(sendall_args = [self.remainder_target]) + + assert_greater_than(tx_from_wallet["decoded"]["locktime"], tx_from_wallet["blockheight"] - 100) + + self.log.info("Testing sendall does not do anti-fee-sniping when locktime is specified") + self.add_utxos([10,11]) + txid = self.wallet.sendall(recipients=[self.remainder_target], options={"locktime":0})["txid"] + assert_equal(self.wallet.gettransaction(txid=txid, verbose=True)["decoded"]["locktime"], 0) + + self.log.info("Testing sendall does not do anti-fee-sniping when even one of the sequences is final") + self.add_utxos([10, 11]) + utxos = self.wallet.listunspent() + utxos[0]["sequence"] = SEQUENCE_FINAL + txid = self.wallet.sendall(recipients=[self.remainder_target], inputs=utxos)["txid"] + assert_equal(self.wallet.gettransaction(txid=txid, verbose=True)["decoded"]["locktime"], 0) + # This tests needs to be the last one otherwise @cleanup will fail with "Transaction too large" error def sendall_fails_with_transaction_too_large(self): self.log.info("Test that sendall fails if resulting transaction is too large") @@ -518,6 +539,9 @@ class SendallTest(BitcoinTestFramework): # Sendall only uses outputs with less than a given number of confirmation when using minconf self.sendall_with_maxconf() + # Sendall discourages fee-sniping when a locktime is not specified + self.sendall_anti_fee_sniping() + # Sendall spends unconfirmed change self.sendall_spends_unconfirmed_change()