wallet: Add sanity checks to DiscourageFeeSniping

[maflcko]

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.

[PastaPastaPasta]

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

[DrahtBot]

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.

[S3RK]

Code review ACK. Changes look good

[chris-belcher]

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]

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]

@S3RK Mind doing a quick re-ACK?

[S3RK]

Code review ACK fa8e76b

[achow101]

ACK fa8e76b

[achow101]

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.

[maflcko]

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