Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

wallet: Add sanity checks to DiscourageFeeSniping #24225

Merged
merged 1 commit into from Mar 14, 2022
Merged

wallet: Add sanity checks to DiscourageFeeSniping #24225

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 25 additions & 12 deletions src/wallet/spend.cpp
View file
Open in desktop
Expand Up @@ -583,12 +583,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
Expand All @@ -610,22 +611,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);
Comment on lines +639 to +640
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for now, but it could be problematic if the sequence of preset inputs and the existing locktime were passed into the newly created transaction. Currently, we don't do that, but we might need to do that later for miniscript.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#24128 removes this assert. So I think either miniscript or #24128 should remove it (whichever happens first)

}
assert(locktime < LOCKTIME_THRESHOLD);
return locktime;
}

static bool CreateTransactionInternal(
Expand All @@ -642,7 +655,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;
Expand Down Expand Up @@ -788,8 +800,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)
Expand All @@ -800,6 +812,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);
Expand Down