wallet: Add sanity checks to DiscourageFeeSniping #24225

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2202-walletStuff changing 1 files +25 −12

MarcoFalke commented at 1:29 PM on February 1, 2022: member

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.
MarcoFalke added the label Refactoring on Feb 1, 2022
MarcoFalke added the label Wallet on Feb 1, 2022
PastaPastaPasta approved
PastaPastaPasta commented at 3:22 PM on February 1, 2022: contributor

light-utACK.. I'm not super familiar with this part of codebase, but generally looks correct
DrahtBot commented at 4:16 AM on February 2, 2022: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #24213 (refactor: use Span in random.*; make GetRand a template, remove GetRandInt by PastaPastaPasta)
- #24128 (wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs by MarcoFalke)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/wallet/spend.cpp:589 in fa32fb032d outdated

 582 | @@ -583,12 +583,13 @@ static bool IsCurrentForAntiFeeSniping(interfaces::Chain& chain, const uint256&
 583 |  }
 584 |  
 585 |  /**
 586 | - * Return a height-based locktime for new transactions (uses the height of the
 587 | + * Set a height-based locktime for new transactions (uses the height of the
 588 |   * current chain tip unless we are not synced with the current chain
 589 |   */
 590 | -static uint32_t GetLocktimeForNewTransaction(interfaces::Chain& chain, const uint256& block_hash, int block_height)
 591 | +static void AntiFeeSnipe(CMutableTransaction& tx, interfaces::Chain& chain, const uint256& block_hash, int block_height)

S3RK commented at 7:27 AM on February 2, 2022:

nit: I'd probably add a verb to the function's name

MarcoFalke commented at 7:48 AM on February 2, 2022:

Isn't to snipe a verb? Though, I am open to suggestions.

S3RK commented at 9:04 AM on February 2, 2022:

I thought fee sniping is that something we want to counter, not something we're doing ourselves. I was thinking along the lines of "SetAntiFeeSnipe" or "SetAntiFeeSnipeLocktime" 🤷‍♂️

MarcoFalke commented at 9:11 AM on February 2, 2022:

Thx, done something

S3RK commented at 7:28 AM on February 2, 2022: member

Code review ACK. Changes look good
wallet: Add sanity checks to AntiFeeSnipe fa8e76bb90
MarcoFalke force-pushed on Feb 2, 2022
chris-belcher commented at 6:47 PM on March 8, 2022: contributor

Code review is fine. I compiled the PR and created a transaction on regtest, which seemed to go well. I added my own print statement to check that the DiscourageFeeSniping function really was called, and therefore that the asserts passed.

ACK https://github.com/bitcoin/bitcoin/pull/24225/commits/fa8e76bb9002a126b3645bd9533c282f5dfff111
w0xlt approved
w0xlt commented at 7:23 PM on March 8, 2022: contributor

Code Review ACK https://github.com/bitcoin/bitcoin/pull/24225/commits/fa8e76bb9002a126b3645bd9533c282f5dfff111

DiscourageFeeSniping better describes the purpose of the function than GetLocktimeForNewTransaction.

Passing the transaction as an argument allows sanity checking on the inputs in DiscourageFeeSniping function.
MarcoFalke renamed this:
~~wallet: Add sanity checks to AntiFeeSnipe~~
wallet: Add sanity checks to DiscourageFeeSniping
on Mar 9, 2022
MarcoFalke commented at 12:30 PM on March 10, 2022: member

@S3RK Mind doing a quick re-ACK?
S3RK commented at 7:10 AM on March 14, 2022: member

Code review ACK fa8e76bb9002a126b3645bd9533c282f5dfff111
MarcoFalke assigned achow101 on Mar 14, 2022
achow101 commented at 10:27 AM on March 14, 2022: member

ACK fa8e76bb9002a126b3645bd9533c282f5dfff111
in src/wallet/spend.cpp:640 in fa8e76bb90
```
 639 | +        // May be MAX NONFINAL to disable both BIP68 and BIP125
 640 | +        if (in.nSequence == CTxIn::MAX_SEQUENCE_NONFINAL) continue;
 641 | +        // May be MAX BIP125 to disable BIP68 and enable BIP125
 642 | +        if (in.nSequence == MAX_BIP125_RBF_SEQUENCE) continue;
 643 | +        // The wallet does not support any other sequence-use right now.
 644 | +        assert(false);
```
achow101 commented at 10:29 AM on March 14, 2022:

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.

MarcoFalke commented at 10:32 AM on March 14, 2022:

#24128 removes this assert. So I think either miniscript or #24128 should remove it (whichever happens first)
achow101 merged this on Mar 14, 2022
achow101 closed this on Mar 14, 2022
MarcoFalke deleted the branch on Mar 14, 2022
DrahtBot locked this on Mar 14, 2023

Contributors

Labels

wallet: Add sanity checks to DiscourageFeeSniping #24225

Conflicts