8000 wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs by maflcko · Pull Request #24128 · bitcoin/bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs #24128

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
Closed
wants to merge 1 commit into from
Closed

wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs #24128

wants to merge 1 commit into from

Conversation

maflcko
Copy link
Member
@maflcko maflcko commented Jan 22, 2022 •
edited
Loading

The goal of BIP 326 is to provide a "privacy cloak" for txs that use sequence-based locking. This is only possible for taproot inputs, as taproot spends may look identical for "dumb wallets" and "smart contracts" iff they use the key-path spend.

Sequence-based locking has minimally lower anti-fee sniping guarantees if all the txs that created the inputs aren't itself locked to the block they were included in. However, the minimal degradation should be acceptable, given that anti-fee-snipe will set the locktime to the past of 10% of the txs anyway.

@maflcko maflcko changed the title wallet: Implement BIP 326 sequence based anti-fee-snipe for taproot inputs wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs Jan 22, 2022
darosior
darosior reviewed Jan 22, 2022
View reviewed changes
Copy link
Member
@darosior darosior left a comment

Choose a reason for hiding this comment

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

Concept ACK, that's awesome

@maflcko maflcko mentioned this pull request Jan 22, 2022
Merged
@DrahtBot DrahtBot added RPC/REST/ZMQ Wallet labels Jan 22, 2022
@DrahtBot
Copy link
Contributor
DrahtBot commented Jan 22, 2022 •
edited
Loading

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/24128.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK pythcoiner
Concept ACK w0xlt, 0xB10C, instagibbs
Stale ACK darosior, chris-belcher, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #30221 (wallet: Ensure best block matches wallet scan state by achow101)
  • #30093 (optimization: reserve memory allocation for transaction inputs/outputs by l0rinc)
  • #28944 (wallet, rpc: add anti-fee-sniping to send and sendall by ishaanam)

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.

This was referenced Jan 22, 2022
Merged
Merged
Closed
w0xlt
w0xlt reviewed Jan 23, 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.

Concept ACK.
AntiFeeSnipe seems to be in line with the approach proposed in BIP 326 and with the suggested pseudocode.
The changes in comments made the purpose of some fields / variables clearer.

@DrahtBot DrahtBot mentioned this pull request Jan 27, 2022
Merged
@DrahtBot DrahtBot mentioned this pull request Jan 31, 2022
Merged
PastaPastaPasta
PastaPastaPasta reviewed Feb 1, 2022
View reviewed changes
@maflcko maflcko force-pushed the 2201-126 branch 2 times, most recently from c00fd82 to 3c6252a Compare February 1, 2022 16:37
@DrahtBot DrahtBot mentioned this pull request Feb 2, 2022
Merged
@maflcko maflcko force-pushed the 2201-126 branch 2 times, most recently from 3dd9f1e to 5d1e606 Compare March 14, 2022 10:48
@DrahtBot DrahtBot mentioned this pull request Mar 14, 2022
Merged
@maflcko maflcko force-pushed the 2201-126 branch 2 times, most recently from a672493 to 37a5206 Compare March 24, 2022 08:21
@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/20487933500

@DrahtBot DrahtBot requested review from w0xlt and chris-belcher June 15, 2024 00:13
@S3RK
Copy link
Contributor
S3RK commented Jun 17, 2024

I think there is a silent merge conflict with #29325

Otherwise, to the best of my understanding, the code is implemented according to BIP-326 and LGTM

@DrahtBot DrahtBot removed the CI failed label Jul 5, 2024
@maflcko
Copy link
Member Author
maflcko commented Jul 8, 2024

rebased

@maflcko
Copy link
Member Author
maflcko commented Jul 11, 2024

One-line force-push to relax the v2 assumption to allow v3/TRUC transactions, to avoid having to touch the code again in the future.

@darosior darosior mentioned this pull request Aug 5, 2024
Open
@achow101 achow101 requested review from instagibbs and achow101 and removed request for w0xlt and chris-belcher October 15, 2024 15:06
instagibbs
instagibbs reviewed Oct 16, 2024
View reviewed changes
Copy link
Member
@instagibbs instagibbs left a comment

Choose a reason for hiding this comment

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

concept ACK, appears to adhere to the BIP on first glance

will do deeper review if tests are applied

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

@pythcoiner pythcoiner mentioned this pull request Nov 17, 2024
Closed
@pythcoiner
Copy link

tACK faf4c71 modulo rebase
reviewed & write this tests that verify:

  • when all inputs are taproot anti fee sniping is 50/50 nSequence/nLockTime
    • when nSequence, the input index is randomly chosen
    • when nLocktime, 10% are 1-100 blocks behind the current height
  • when there is some non-taproot inputs, anti fee sniping is always nLocktime
  • when there is some non-confirmed inputs (from self), anti fee sniping is always nLocktime
  • when there rbf is disabled, anti fee sniping is always nLocktime

@DrahtBot
Copy link
Contributor

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@maflcko
Copy link
Member Author
maflcko commented Feb 25, 2025

Closing for now as up-for-grabs

@maflcko maflcko closed this Feb 25, 2025
@maflcko maflcko deleted the 2201-126 branch February 25, 2025 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PastaPastaPasta PastaPastaPasta PastaPastaPasta left review comments

@KristijanSajenko KristijanSajenko KristijanSajenko approved these changes

@Mazzika1 Mazzika1 Mazzika1 approved these changes

@0xB10C 0xB10C Awaiting requested review from 0xB10C

@darosior darosior Awaiting requested review from darosior

@S3RK S3RK Awaiting requested review from S3RK

@achow101 achow101 Awaiting requested review from achow101

@w0xlt w0xlt Awaiting requested review from w0xlt

@chris-belcher chris-belcher Awaiting requested review from chris-belcher

@instagibbs instagibbs Awaiting requested review from instagibbs

Assignees
No one assigned
Labels
Needs rebase RPC/REST/ZMQ Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
0