Skip to content

wallet: Add sanity checks to DiscourageFeeSniping#24225

Merged
achow101 merged 1 commit intobitcoin:masterfrom
maflcko:2202-walletStuff
Mar 14, 2022
Merged

wallet: Add sanity checks to DiscourageFeeSniping#24225
achow101 merged 1 commit intobitcoin:masterfrom
maflcko:2202-walletStuff

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Feb 1, 2022 •
edited
Loading

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.

@maflcko maflcko mentioned this pull request Feb 1, 2022
Closed
PastaPastaPasta
PastaPastaPasta approved these changes Feb 1, 2022
View reviewed changes
Copy link
Contributor

@PastaPastaPasta PastaPastaPasta left a comment •
edited
Loading

Choose a reason for hiding this comment

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

light-utACK.. I'm not super familiar with this part of codebase, but generally looks correct

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 2, 2022 •
edited
Loading

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.

@DrahtBot DrahtBot mentioned this pull request Feb 2, 2022
Merged
S3RK
S3RK reviewed Feb 2, 2022
View reviewed changes
Copy link
Contributor

@S3RK S3RK left a comment

Choose a reason for hiding this comment

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

Code review ACK. Changes look good

@chris-belcher
Copy link
Contributor

chris-belcher commented Mar 8, 2022

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 fa8e76b

w0xlt
w0xlt approved these changes Mar 8, 2022
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

Code Review ACK fa8e76b

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.

@maflcko maflcko changed the title wallet: Add sanity checks to AntiFeeSnipe wallet: Add sanity checks to DiscourageFeeSniping Mar 9, 2022
@maflcko
Copy link
Member Author

maflcko commented Mar 10, 2022

@S3RK Mind doing a quick re-ACK?

@S3RK
Copy link
Contributor

S3RK commented Mar 14, 2022

Code review ACK fa8e76b

@achow101
Copy link
Member

achow101 commented Mar 14, 2022

ACK fa8e76b

achow101
achow101 reviewed Mar 14, 2022
View reviewed changes
src/wallet/spend.cpp
Comment on lines +639 to +640
// The wallet does not support any other sequence-use right now.
assert(false);
Copy link
Member

@achow101 achow101 Mar 14, 2022

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

@maflcko maflcko Mar 14, 2022

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)

@achow101 achow101 merged commit 25d045a into bitcoin:master Mar 14, 2022
@maflcko maflcko deleted the 2202-walletStuff branch March 14, 2022 10:32
@bitcoin bitcoin locked and limited conversation to collaborators Mar 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@achow101 achow101 achow101 left review comments

+3 more reviewers

@S3RK S3RK S3RK left review comments

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

@w0xlt w0xlt w0xlt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@achow101 achow101

Labels

Refactoring Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@maflcko @DrahtBot @chris-belcher @S3RK @achow101 @PastaPastaPasta @w0xlt