Merge 28944 via sendall_antifeesniping-26

src/wallet/rpc/spend.cpp

@@ -76,8 +76,21 @@ static void InterpretFeeEstimationInstructions(const UniValue& conf_target, cons
     }
 }
 
-static UniValue FinishTransaction(const std::shared_ptr<CWallet> pwallet, const UniValue& options, const CMutableTransaction& rawTx)
+static UniValue FinishTransaction(const std::shared_ptr<CWallet> pwallet, const UniValue& options, CMutableTransaction& rawTx)
 {
+    bool can_anti_fee_snipe = !options.exists("locktime");
+
+    for (const CTxIn& tx_in : rawTx.vin) {
+        // Can not be FINAL for locktime to work
+        can_anti_fee_snipe &= tx_in.nSequence != CTxIn::SEQUENCE_FINAL;
+    }
+
+    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);
 
@@ -1230,7 +1243,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"
@@ -1335,7 +1348,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."},

src/wallet/spend.cpp

@@ -908,7 +908,7 @@ static bool IsCurrentForAntiFeeSniping(interfaces::Chain& chain, const uint256&
  * 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

src/wallet/spend.h

@@ -213,6 +213,8 @@ struct CreatedTransactionResult
         : tx(_tx), fee(_fee), fee_calc(_fee_calc), change_pos(_change_pos) {}
 };
 
+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

test/functional/wallet_import_rescan.py

@@ -293,6 +293,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"]

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()
 

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)

test/functional/wallet_sendall.py

@@ -379,6 +379,18 @@ class SendallTest(BitcoinTestFramework):
         assert_equal(len(self.wallet.listunspent()), 1)
         assert_equal(self.wallet.listunspent()[0]['confirmations'], 6)
 
+    @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.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)
+
     # 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")
@@ -460,6 +472,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 fails when many inputs result to too large transaction
         self.sendall_fails_with_transaction_too_large()