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

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2201-126 changing 2 files +80 −15
  1. maflcko commented at 2:12 PM on January 22, 2022: member

    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.

  2. maflcko force-pushed on Jan 22, 2022
  3. maflcko renamed this:
    wallet: Implement BIP 326 sequence based anti-fee-snipe for taproot inputs
    wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs
    on Jan 22, 2022
  4. maflcko force-pushed on Jan 22, 2022
  5. darosior commented at 2:35 PM on January 22, 2022: member

    Concept ACK, that's awesome

  6. maflcko force-pushed on Jan 22, 2022
  7. maflcko force-pushed on Jan 22, 2022
  8. DrahtBot added the label RPC/REST/ZMQ on Jan 22, 2022
  9. DrahtBot added the label Wallet on Jan 22, 2022
  10. DrahtBot commented at 8:34 PM on January 22, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  11. w0xlt commented at 12:57 PM on January 23, 2022: contributor

    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.

  12. maflcko force-pushed on Jan 24, 2022
  13. maflcko force-pushed on Jan 31, 2022
  14. in src/wallet/spend.cpp:664 in e0a3bf98b5 outdated 
     657 | +            // Secondly occasionally randomly pick a nLockTime even further back, so
 658 | +            // that transactions that are delayed after signing for whatever reason,
 659 | +            // e.g. high-latency mix networks and some CoinJoin implementations, have
 660 | +            // better privacy.
 661 | +            if (GetRandInt(10) == 0) {
 662 | +                tx.nLockTime = std::max(0, (int)tx.nLockTime - GetRandInt(100));
    PastaPastaPasta commented at 12:52 PM on February 1, 2022:

    please convert this to a c++11 style functional cast

    maflcko commented at 1:30 PM on February 1, 2022:

    Seems unrelated to this pull request, but done in https://github.com/bitcoin/bitcoin/pull/24225

  15. maflcko force-pushed on Feb 1, 2022
  16. maflcko force-pushed on Feb 1, 2022
  17. maflcko force-pushed on Mar 14, 2022
  18. maflcko force-pushed on Mar 14, 2022
  19. DrahtBot added the label Needs rebase on Mar 23, 2022
  20. maflcko force-pushed on Mar 24, 2022
  21. maflcko force-pushed on Mar 24, 2022
  22. DrahtBot removed the label Needs rebase on Mar 24, 2022
  23. DrahtBot added the label Needs rebase on Mar 24, 2022
  24. maflcko force-pushed on Mar 25, 2022
  25. DrahtBot removed the label Needs rebase on Mar 25, 2022
  26. maflcko marked this as ready for review on Mar 25, 2022
  27. in src/wallet/spend.cpp:677 in fa2066fd12 outdated 
     680 | +        } else { // sequence based
 681 | +            tx.nLockTime = 0;
 682 | +            const auto i_input{GetRandInt(tx.vin.size())};
 683 | +            CTxIn& in{tx.vin.at(i_input)};
 684 | +            in.nSequence = inputs.at(i_input).depth;
 685 | +            if (GetRandInt(10) == 0) {
    darosior commented at 4:47 PM on March 30, 2022:

    nit: since you already have rng_fast you could use it instead of re-creating a context with GetRandInt. (Here and elsewhere in this function.)

  28. in src/wallet/spend.cpp:994 in fa2066fd12 outdated 
     661 | +                std::vector<std::vector<uint8_t>> dummy;
 662 | +                const TxoutType in_type{Solver(in.txout.scriptPubKey, dummy)};
 663 | +                // Use locktime if any input isn't taproot
 664 | +                if (in_type != TxoutType::WITNESS_V1_TAPROOT) {
 665 | +                    return true;
 666 | +                }
    darosior commented at 5:17 PM on March 30, 2022:

    It has a specific usecase for Taproot, but what is the rationale for disabling it for non-(pure-)Taproot transactions? The "created by Bitcoin Core wallet" fingerprint?

    maflcko commented at 4:52 PM on March 31, 2022:

    I can't say for sure, since I am not the author of BIP 326, but I presume:

    • Any protocol that relies on sequence values in "legacy" key-only spends may be encouraged to upgrade to taproot.
    • Any new "population" of txs that have the same fingerprint will reduce the privacy for themselves and everyone else. Though, if such a population already exists, I can see the reason for Bitcoin Core to mix itself into that population.

    So I guess the question is whether such a protocol exists and is unlikely to use/upgrade to taproot?

    chris-belcher commented at 4:24 PM on April 6, 2022:

    As the author of bip326, yes, I was thinking along the lines of @MarcoFalke's second point. Changing the behaviour of legacy transactions of which there is already a large population would reduce privacy. It's best to leave legacy transactions unchanged. Also non-taproot outputs cannot (easily) be used to create point-time-locked-contracts so the "privacy cloak" they create is useless, it would never be used.

    maflcko commented at 6:40 AM on April 7, 2022:

    I think a protocol that uses plain non-taproot keys with just plain pre-signed transactions and sequence based locktimes could still benefit from Bitcoin Core cloaking non-taproot inputs, no? Though, as they only use plain keys they might as well upgrade to taproot to get the taproot-specific cloak.

    chris-belcher commented at 9:25 AM on April 7, 2022:

    I don't think that kind of protocol is possible. A point lock requires taproot. Why would someone use nsequence-based locktimes when they can't have some kind of point-lock?

    darosior commented at 8:24 AM on May 9, 2022:

    Forgot to reply to this comment earlier. Chris, nSequence-based locktimes are useful for anti-fee sniping for presigned transactions (as you can't use nLockTime-based protection for those, use an input with an nSequence set when bumping the fees). I don't think this is a justification for cloaking non-Taproot inputs here. By the time the cloak is realistically available, they would probably use Taproot and otherwise they should. They have an incentive to upgrade anyways, as fee-bumping with Taproot inputs is more efficient.

  29. darosior approved
  30. darosior commented at 5:47 PM on March 30, 2022: member

    I've a question and a nit, but the code looks good to me and i tested it locally. ACK fa2066fd1272b857501f996267052b4f64a04b29

  31. maflcko force-pushed on Mar 31, 2022
  32. darosior commented at 1:08 PM on April 4, 2022: member

    re-ACK fa8cfae8b82a36ae598e1956d16d2d65ac8df44c

  33. chris-belcher commented at 9:33 AM on April 7, 2022: contributor

    I've reviewed the code and it looks fine.

    I compiled this branch and in regtest mode I created many taproot UTXOs which I spent. For each one I looked at the decoded transaction and confirmed that it sometimes sets the sequence number and sometimes sets the locktime, which is the correct behaviour.

    tACK https://github.com/bitcoin/bitcoin/pull/24128/commits/fa8cfae8b82a36ae598e1956d16d2d65ac8df44c

  34. 0xB10C commented at 2:52 PM on April 7, 2022: contributor

    Concept ACK. I'm putting this on my to-review list.

  35. in src/wallet/spend.cpp:667 in fa8cfae8b8 outdated 
     670 | +                    return true;
 671 | +                }
 672 | +            }
 673 | +            return false;
 674 | +        }};
 675 | +        if (locktime_based() || rng_fast.randrange(2) == 0) {
    achow101 commented at 9:17 PM on May 19, 2022:

    nit:

    Use randbool

            if (locktime_based() || rng_fast.randbool()) {

    Also, why not have the random bool be part of locktime_based?

    maflcko commented at 12:18 PM on June 1, 2022:

    Yeah, it can be moved inside, as all early returns are true and the final return is the only one with false.

  36. in src/wallet/spend.cpp:851 in fa8cfae8b8 outdated 
     684 | +            }
 685 | +        } else { // sequence based
 686 | +            tx.nLockTime = 0;
 687 | +            const auto i_input{rng_fast.randrange(tx.vin.size())};
 688 | +            CTxIn& in{tx.vin.at(i_input)};
 689 | +            in.nSequence = inputs.at(i_input).depth;
    achow101 commented at 9:29 PM on May 19, 2022:

    There could be some unexpected behavior here when users set their own sequence numbers.

    If a user set their own sequence, we would not want to clobber it as that could have negative effects on their payment. So we should avoid changing the sequence on any inputs that use a non-default sequence number.

    However, DiscourageFeeSniping is called by CreateTransactionInternal which does not actually set (or know about) user defined sequences. User defined sequences are handled externally in FundTransaction where the it actually retains the CTxIns of the transaction to fund and then adds the inputs that CreateTransaction chooses.

    So user defined sequences actually end up in the transaction and we lose the anti-fee-sniping that we wanted.

    I think the correct way to do this is to make CreateTransactionInternal aware of user defined sequences and set them in the transaction it is creating. Then here in DiscrouageFeeSniping, we can check just choose randomly among the inputs that have default sequences.

    However, what should we do if the user sets a default sequence explicitly? Should we respect that and avoid randomly selecting those inputs? Additionally, even if a user did set a default sequence explicitly, fundrawtransaction would not know because it takes a raw transaction which has sequences that createrawtransaction may have set using defaults.

    maflcko commented at 12:17 PM on June 1, 2022:

    I think we should never fiddle with user chosen sequence numbers, even if they happen to be "default". I also think this is the way that createrawtransaction/createpsbt works (it wouldn't set locktime nonzero unless the user asked for it and it wouldn't set sequence in violation of the replaceable boolean option unless the user asked for it).

    If you are asking to extend fee sniping to createrawtransaction/createpsbt, I think this should be done in a separate pull request.

    The sequence numbers in CreateTransactionInternal solely depend on const bool use_rbf, so I think the changes here are both safe and sane.

    achow101 commented at 3:45 PM on June 1, 2022:

    At the very least I think this ends up applying anti-fee-sniping inconsistently.

    The current behavior is that we never apply anti-fee-sniping to transactions funded with fundrawtransaction and walletcreatefundedpsbt as the locktime provided by the user (or the default of 0) is always used regardless of DiscourageFeeSniping.

    With this PR, if DiscourageFeeSniping randomly chose a preset input, then we would discard the sequence it applies to that input and use the user provided sequence (or the default).

    However if DiscourageFeeSniping chooses an input that coin selection chose, then the sequence it applies would be used and the transaction would end up using sequence based anti-fee-sniping.

    So the only results if a user uses fundrawtransaction or walletcreatefundedpsbt are sequence based anti-fee-sniping or no anti-fee-sniping. This could be a potential fingerprint. Thus we may want to disable anti-fee-sniping whenever fundrawtransaction or walletcreatefundedpsbt are used, at least until we are able to provide sequences and locktimes to CreateTransactionInternal (which I am currently working on).

    maflcko commented at 5:59 PM on June 1, 2022:

    Oh, good catch. Though, it appears this bug already exists in current master.

    You can test this by calling createrawtransaction with replacable=True and providing some inputs, then have fundrawtransaction pick the remaining inputs. Now anyone can tell apart the createrawtransaction-inputs from the fundrawtransaction-inputs by looking at the sequence number.

  37. maflcko force-pushed on Jun 1, 2022
  38. maflcko force-pushed on Jun 1, 2022
  39. darosior commented at 11:36 AM on March 23, 2023: member

    People interested in seeing this move forward may be interested in giving #25273 a look. It was recently rebased and reviewed twice.

  40. DrahtBot added the label Needs rebase on Aug 17, 2023
  41. maflcko force-pushed on Aug 17, 2023
  42. maflcko force-pushed on Aug 17, 2023
  43. DrahtBot added the label CI failed on Aug 17, 2023
  44. DrahtBot removed the label Needs rebase on Aug 17, 2023
  45. DrahtBot removed the label CI failed on Aug 18, 2023
  46. DrahtBot added the label CI failed on Oct 25, 2023
  47. DrahtBot removed the label CI failed on Oct 26, 2023
  48. DrahtBot added the label Needs rebase on Dec 11, 2023
  49. maflcko force-pushed on Dec 11, 2023
  50. DrahtBot removed the label Needs rebase on Dec 11, 2023
  51. DrahtBot added the label CI failed on Jan 1, 2024
  52. DrahtBot removed the label CI failed on Jan 9, 2024
  53. DrahtBot added the label CI failed on Jan 14, 2024
  54. maflcko force-pushed on Jan 15, 2024
  55. DrahtBot removed the label CI failed on Jan 15, 2024
  56. achow101 commented at 9:32 PM on January 24, 2024: member

    ACK fa08291375ea00cfacd52956bcac7468824a0bf4

    With #25273 now merged, I believe this now applies the anti-fee-sniping consistently. It won't apply it to any transactions that are created by calling FundTransaction (that should only affect fundrawtransaction and walletcreatefundedpsbt) since those transactions always have preset sequences. But that can be improved in the future and this will at least behave consistently.

  57. DrahtBot requested review from chris-belcher on Jan 24, 2024
  58. DrahtBot requested review from 0xB10C on Jan 24, 2024
  59. DrahtBot requested review from darosior on Jan 24, 2024
  60. DrahtBot requested review from w0xlt on Jan 24, 2024
  61. DrahtBot removed review request from chris-belcher on Jan 25, 2024
  62. DrahtBot removed review request from w0xlt on Jan 25, 2024
  63. DrahtBot requested review from w0xlt on Jan 25, 2024
  64. DrahtBot requested review from chris-belcher on Jan 25, 2024
  65. achow101 requested review from achow101 on Apr 9, 2024
  66. achow101 removed review request from w0xlt on Apr 9, 2024
  67. achow101 removed review request from chris-belcher on Apr 9, 2024
  68. achow101 requested review from S3RK on Apr 9, 2024
  69. achow101 removed review request from achow101 on Apr 15, 2024
  70. DrahtBot commented at 8:56 PM on June 14, 2024: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 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.

    <sub>Debug: https://github.com/bitcoin/bitcoin/runs/20487933500</sub>

  71. DrahtBot added the label CI failed on Jun 14, 2024
  72. KristijanSajenko approved
  73. DrahtBot requested review from w0xlt on Jun 15, 2024
  74. DrahtBot requested review from chris-belcher on Jun 15, 2024
  75. S3RK commented at 7:48 AM on June 17, 2024: contributor

    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

  76. Mazzika1 approved
  77. maflcko force-pushed on Jul 5, 2024
  78. DrahtBot removed the label CI failed on Jul 5, 2024
  79. maflcko commented at 6:01 AM on July 8, 2024: member

    rebased

  80. Implement sequence-based anti-fee-snipe faf4c7107b
  81. maflcko force-pushed on Jul 11, 2024
  82. maflcko commented at 6:59 PM on July 11, 2024: member

    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.

  83. achow101 removed review request from w0xlt on Oct 15, 2024
  84. achow101 removed review request from chris-belcher on Oct 15, 2024
  85. achow101 requested review from instagibbs on Oct 15, 2024
  86. achow101 requested review from achow101 on Oct 15, 2024
  87. instagibbs commented at 10:47 AM on October 16, 2024: member

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

    will do deeper review if tests are applied

  88. DrahtBot added the label Needs rebase on Oct 16, 2024
  89. DrahtBot commented at 2:28 PM on October 16, 2024: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

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

  90. pythcoiner commented at 4:32 AM on November 18, 2024: none

    tACK faf4c7107bafc8b0db48909f37e4a242107b804a 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
  91. DrahtBot requested review from w0xlt on Nov 18, 2024
  92. DrahtBot requested review from chris-belcher on Nov 18, 2024
  93. DrahtBot requested review from instagibbs on Nov 18, 2024
  94. DrahtBot commented at 1:48 AM on February 15, 2025: contributor

    <!--13523179cfe9479db18ec6c5d236f789-->

    ⌛ 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.
  95. maflcko commented at 2:27 PM on February 25, 2025: member

    Closing for now as up-for-grabs

  96. maflcko closed this on Feb 25, 2025
  97. maflcko deleted the branch on Feb 25, 2025
Contributors
maflckodarosiorDrahtBotw0xltPastaPastaPastachris-belcher0xB10Cachow101KristijanSajenkoS3RKMazzika1instagibbspythcoiner


S3RKachow101instagibbschris-belcher0xB10Cdarosiorw0xlt

Labels
WalletRPC/REST/ZMQNeeds rebase
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me