Merge bitcoin/bitcoin#24225: wallet: Add sanity checks to DiscourageFeeSniping

fa8e76bb9002a126b3645bd9533c282f5dfff111 wallet: Add sanity checks to AntiFeeSnipe (MarcoFalke)

Pull request description:

  I added those sanity checks as part of implementing BIP 326, but I think they make sense on their own. The checks require the transaction to be passed in to `DiscourageFeeSniping`. Also, replace `(int)locktime` cast with the equivalent `int(locktime)` cast.

ACKs for top commit:
  chris-belcher:
    ACK https://github.com/bitcoin/bitcoin/pull/24225/commits/fa8e76bb9002a126b3645bd9533c282f5dfff111
  S3RK:
    Code review ACK fa8e76bb9002a126b3645bd9533c282f5dfff111
  achow101:
    ACK fa8e76bb9002a126b3645bd9533c282f5dfff111
  w0xlt:
    Code Review ACK https://github.com/bitcoin/bitcoin/pull/24225/commits/fa8e76bb9002a126b3645bd9533c282f5dfff111

Tree-SHA512: 6fe37c85f074851ef09cae8addcb836257089290fecec515c129c3180e9ccb64740aaac76043af10ce7c213e5001568ca6b4b62ae9631f0994ed676b07118074

1 files changed, 25 insertions, 12 deletions

diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 582464c5b8..a707ef89d2 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -582,12 +582,13 @@ static bool IsCurrentForAntiFeeSniping(interfaces::Chain& chain, const uint256&
 }
 
 /**
- * Return a height-based locktime for new transactions (uses the height of the
+ * 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 uint32_t GetLocktimeForNewTransaction(interfaces::Chain& chain, const uint256& block_hash, int block_height)
+static void DiscourageFeeSniping(CMutableTransaction& tx, interfaces::Chain& chain, const uint256& block_hash, int block_height)
 {
-    uint32_t locktime;
+    // All inputs must be added by now
+    assert(!tx.vin.empty());
     // Discourage fee sniping.
     //
     // For a large miner the value of the transactions in the best block and
@@ -609,22 +610,34 @@ static uint32_t GetLocktimeForNewTransaction(interfaces::Chain& chain, const uin
     // now we ensure code won't be written that makes assumptions about
     // nLockTime that preclude a fix later.
     if (IsCurrentForAntiFeeSniping(chain, block_hash)) {
-        locktime = block_height;
+        tx.nLockTime = block_height;
 
         // Secondly occasionally randomly pick a nLockTime even further back, so
         // that transactions that are delayed after signing for whatever reason,
         // e.g. high-latency mix networks and some CoinJoin implementations, have
         // better privacy.
-        if (GetRandInt(10) == 0)
-            locktime = std::max(0, (int)locktime - GetRandInt(100));
+        if (GetRandInt(10) == 0) {
+            tx.nLockTime = std::max(0, int(tx.nLockTime) - GetRandInt(100));
+        }
     } else {
         // If our chain is lagging behind, we can't discourage fee sniping nor help
         // the privacy of high-latency transactions. To avoid leaking a potentially
         // unique "nLockTime fingerprint", set nLockTime to a constant.
-        locktime = 0;
+        tx.nLockTime = 0;
+    }
+    // Sanity check all values
+    assert(tx.nLockTime < LOCKTIME_THRESHOLD); // Type must be block height
+    assert(tx.nLockTime <= uint64_t(block_height));
+    for (const auto& in : tx.vin) {
+        // Can not be FINAL for locktime to work
+        assert(in.nSequence != CTxIn::SEQUENCE_FINAL);
+        // May be MAX NONFINAL to disable both BIP68 and BIP125
+        if (in.nSequence == CTxIn::MAX_SEQUENCE_NONFINAL) continue;
+        // May be MAX BIP125 to disable BIP68 and enable BIP125
+        if (in.nSequence == MAX_BIP125_RBF_SEQUENCE) continue;
+        // The wallet does not support any other sequence-use right now.
+        assert(false);
     }
-    assert(locktime < LOCKTIME_THRESHOLD);
-    return locktime;
 }
 
 static bool CreateTransactionInternal(
@@ -641,7 +654,6 @@ static bool CreateTransactionInternal(
     AssertLockHeld(wallet.cs_wallet);
 
     CMutableTransaction txNew; // The resulting transaction that we make
-    txNew.nLockTime = GetLocktimeForNewTransaction(wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight());
 
     CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy
     coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends;
@@ -787,8 +799,8 @@ static bool CreateTransactionInternal(
     // Shuffle selected coins and fill in final vin
     std::vector<CInputCoin> selected_coins = result->GetShuffledInputVector();
 
-    // Note how the sequence number is set to non-maxint so that
-    // the nLockTime set above actually works.
+    // The sequence number is set to non-maxint so that DiscourageFeeSniping
+    // works.
     //
     // BIP125 defines opt-in RBF as any nSequence < maxint-1, so
     // we use the highest possible value in that range (maxint-2)
@@ -799,6 +811,7 @@ static bool CreateTransactionInternal(
     for (const auto& coin : selected_coins) {
         txNew.vin.push_back(CTxIn(coin.outpoint, CScript(), nSequence));
     }
+    DiscourageFeeSniping(txNew, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight());
 
     // Calculate the transaction fee
     TxSize tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, &coin_control);