diff options
author | Andrew Chow <achow101-github@achow101.com> | 2022-03-14 10:27:08 +0000 |
---|---|---|
committer | Andrew Chow <achow101-github@achow101.com> | 2022-03-14 10:31:25 +0000 |
commit | 25d045a9ecc24a2752685d176c28f8f75bb100f6 (patch) | |
tree | 6e53052063c924f6b95388a26454eca845f619bc /src/wallet/spend.cpp | |
parent | e0881aa5f0cde01b0590770f3202dfaf0919e2d4 (diff) | |
parent | fa8e76bb9002a126b3645bd9533c282f5dfff111 (diff) | |
download | bitcoin-25d045a9ecc24a2752685d176c28f8f75bb100f6.tar.xz |
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
Diffstat (limited to 'src/wallet/spend.cpp')
-rw-r--r-- | src/wallet/spend.cpp | 37 |
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); |