Add option to opt into full-RBF when sending funds #7132

pull petertodd wants to merge 1 commits into bitcoin:master from petertodd:2015-11-opt-into-full-rbf-option changing 2 files +9 −3
  1. petertodd commented at 5:17 AM on November 30, 2015: contributor

    Useful for anyone using third-party scripts to make use of RBF functionality.

    Defaults to sending txs with full-RBF off.

  2. in src/init.cpp:None in 412bf575f3 outdated 
     404 | @@ -405,6 +405,7 @@ std::string HelpMessage(HelpMessageMode mode)
 405 |      strUsage += HelpMessageOpt("-txconfirmtarget=<n>", strprintf(_("If paytxfee is not set, include enough fee so transactions begin confirmation on average within n blocks (default: %u)"), DEFAULT_TX_CONFIRM_TARGET));
 406 |      strUsage += HelpMessageOpt("-maxtxfee=<amt>", strprintf(_("Maximum total fees (in %s) to use in a single wallet transaction; setting this too low may abort large transactions (default: %s)"),
 407 |          CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MAXFEE)));
 408 | +    strUsage += HelpMessageOpt("-optintofullrbf", strprintf(_("Send transactions with full-RBF opt-in enabled (default: %u)"), DEFAULT_OPT_INTO_FULL_RBF));
    luke-jr commented at 5:21 AM on November 30, 2015:

    Parameter names shouldn't imply a default value (as "opt-in" does).

    dcousens commented at 5:34 AM on November 30, 2015:

    Perhaps -walletusefullrbf

    petertodd commented at 5:35 AM on November 30, 2015:

    @luke-jr What's your suggested name?

    luke-jr commented at 7:20 AM on November 30, 2015:

    -walletrbf maybe?

    luke-jr commented at 7:21 AM on November 30, 2015:

    Also, don't forget the #ifdef ENABLE_WALLET guard...

    petertodd commented at 1:05 AM on December 1, 2015:

    I think everything is under an ENABLE_WALLET guard; did I miss something?

    petertodd commented at 1:07 AM on December 1, 2015:

    Re: name, other names like -spendzeroconfchange and -sendfreetransactions have similar grammar as -optintofullrbf, so I'm inclined to continue that pattern.

    luke-jr commented at 1:07 AM on December 1, 2015:

    No, I just failed to expand more of the code visible here.

    petertodd commented at 1:12 AM on December 1, 2015:

    Cool, thanks for reminding me though - that it was under ENABLE_WALLET was just luck.

    jtimon commented at 7:49 PM on January 3, 2016:

    This is confusing, is it full RBF or opt-in RBF ? We've been using fullRBF to refer to the relay policy that ignores the opt-in flag and always does RBF regardless of the sender's intentions. Can you just call it optinRBF instead of optinfullRBF ?

    MarcoFalke commented at 8:40 PM on January 8, 2016:

    Agree with @luke-jr 's suggestion: -walletrbf.

  3. dcousens commented at 5:33 AM on November 30, 2015: contributor

    Wasn't this already closed? #7119

  4. petertodd commented at 5:35 AM on November 30, 2015: contributor

    @dcousens The closed version defaulted to opt-in=true.

  5. jonasschnelli added the label Wallet on Nov 30, 2015
  6. rnicoll commented at 12:59 PM on November 30, 2015: contributor

    utACK

  7. ghost commented at 6:50 PM on November 30, 2015: none

    utACK

  8. dcousens commented at 12:27 AM on December 1, 2015: contributor

    utACK, but does the wallet actually support the re-creation of a transaction in any sane way? Or is this a political thing?

    concept ACK only if the wallet actually allows you to re-broadcast a replacement.

  9. gmaxwell commented at 1:03 AM on December 1, 2015: contributor

    @dcousens I suppose it's useful for testing software-- e.g. if you want something that identifies these transactions you need to generate some, if nothing else. Actual replacement will be a non-trivial amount of work. Concept ACK at least...

  10. petertodd force-pushed on Dec 1, 2015
  11. dcousens commented at 1:07 AM on December 1, 2015: contributor

    if you want something that identifies these transactions you need to generate some, if nothing else

    Then use sendrawtransaction?

    Actual replacement will be a non-trivial amount of work.

    IMHO, then that is what should prefix this flag. Otherwise its just misleading.

  12. luke-jr commented at 1:09 AM on December 1, 2015: member

    Real world use case: Core's wallet is used in day-to-day operation, but occasionally stuck transactions need replacing to get confirmed, which the user then uses an external script for. It's not pretty, but it's a real use case.

    If it was only needed for testing stuff, I'd say +1 to just telling people to use sendrawtransaction also - or modify the code.

  13. petertodd commented at 1:10 AM on December 1, 2015: contributor

    @dcousens I have scripts that do the rebroadcast, and using those scripts is a pain if Bitcoin Core isn't already sending txs with opt-in enabled, allowing those scripts to be used with existing txs. I'm sure we'll have even better support in the future, but this is a trivial first-step.

    Particularly with the opt-in defaulting to false, I can't see how this is political - we're just making it a little easier to use a feature that we've already merged.

  14. dcousens commented at 1:17 AM on December 1, 2015: contributor

    Real world use case: Core's wallet is used in day-to-day operation, but occasionally stuck transactions need replacing to get confirmed, which the user then uses an external script for. It's not pretty, but it's a real use case.

    Sure, concept ACK then.

    I can't see how this is political - we're just making it a little easier to use a feature that we've already merged.

    If you don't accept the above use case as a possibility, then, IMHO, it didn't really serve any other purpose.

    I didn't personally think people would be mixing scripts and the UI.

  15. petertodd force-pushed on Dec 2, 2015
  16. petertodd commented at 12:57 AM on December 2, 2015: contributor

    Rebased

  17. petertodd force-pushed on Dec 2, 2015
  18. petertodd commented at 4:25 AM on December 2, 2015: contributor

    Just changed this to set nSequence to maxint-2 instead, so that more of the nSequence bits are identical to non-opt-in behavior.

    This might come in handy if we, for instance, ever implement proof-of-stake blocksize voting and need a default option that matches what most wallets already do.

  19. dcousens commented at 4:31 AM on December 2, 2015: contributor

    re-ACK , but, if the idea is future proofing it. Why not just allow say 64 nSequence bits?

  20. petertodd commented at 4:35 AM on December 2, 2015: contributor

    What do you mean by "100 nSequence bits"?

    On 2 December 2015 12:32:11 GMT+08:00, Daniel Cousens notifications@github.com wrote:

    re-ACK, but, if the idea is future proofing it. Why not just allow say 100 nSequence bits?

    Reply to this email directly or view it on GitHub: #7132 (comment)

  21. dcousens commented at 4:37 AM on December 2, 2015: contributor

    maxint-64

  22. petertodd commented at 4:40 AM on December 2, 2015: contributor

    Sorry, I don't see what's the advantage of that; maxint-2 seems simpler.

    On 2 December 2015 12:38:06 GMT+08:00, Daniel Cousens notifications@github.com wrote:

    maxint-64

    Reply to this email directly or view it on GitHub: #7132 (comment)

  23. dcousens commented at 4:45 AM on December 2, 2015: contributor

    @petertodd my understanding is you're expanding the non-opt-in space by 1 to allow for possible future usage. Why only increase that space by 1 bit, and not say 5 bits?

  24. petertodd commented at 4:46 AM on December 2, 2015: contributor

    Oh! That's not at all what I'm doing! This is just the wallet code; I'm changing what txs the wallet produces, not changing the rules for what is considered RBF opt-in.

    On 2 December 2015 12:43:47 GMT+08:00, Daniel Cousens notifications@github.com wrote:

    @petertodd my understanding is you're expanding the non-opt-in space by 1 to allow for possible future usage. Why only increase that space by 8 bits, and not say 64 bits?

    Reply to this email directly or view it on GitHub: #7132 (comment)

  25. dcousens commented at 4:49 AM on December 2, 2015: contributor

    @petertodd my bad. Lost context.

    On that note then, why not 0 as discussed in #6871 (comment)?

  26. petertodd commented at 8:58 AM on December 2, 2015: contributor

    @dcousens Because of #7132 (comment)

    Remember that all we need is for all users' to be using the same nSequence number for a given type of tx; for privacy the common standard is what matters, not exactly what number we pick.

  27. dcousens commented at 2:58 AM on December 3, 2015: contributor

    Sure.

  28. laanwj added the label Feature on Dec 3, 2015
  29. jtimon commented at 7:50 PM on January 3, 2016: contributor

    Concept ACK

  30. jonasschnelli commented at 1:00 PM on January 20, 2016: contributor

    Tend to NACK. I think there should be an option to selective opt-in a wtx. I guess some users like to use RBF by default, but still want to capability to create a non-RBF transaction if they spend to a "0-conf-merchant" (BitPay, Coinbase, etc.).

    I think base work for this (starting with rawtx command) could be #7159.

  31. laanwj added this to the milestone 0.13.0 on Apr 1, 2016
  32. laanwj commented at 9:42 AM on April 25, 2016: member

    Agree with @jonasschnelli here, a global option is too coarse, need a way to have control over this per transaction.

  33. laanwj removed this from the milestone 0.13.0 on Apr 25, 2016
  34. petertodd commented at 11:28 AM on April 25, 2016: contributor

    Why not both?

    On 25 April 2016 05:43:48 GMT-04:00, "Wladimir J. van der Laan" notifications@github.com wrote:

    Agree with @jonasschnelli here, a global option is too coarse, need a way to have control over this per transaction.

    You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #7132 (comment)

  35. laanwj commented at 11:56 AM on April 25, 2016: member

    Especially as it's aimed at third-party scripts it is better if those scripts can specify the option, without requiring the user to change yet another option before they can be used.

  36. petertodd force-pushed on May 20, 2016
  37. petertodd commented at 2:49 PM on May 20, 2016: contributor

    Rebased

  38. btcdrak commented at 10:36 AM on June 14, 2016: contributor

    needs rebase

  39. laanwj commented at 11:24 AM on June 14, 2016: member

    @arowser can you stop posting "Can one of the admins verify this patch?" in every pull, this is annoying and completely redundant. You can help by reviewing or testing the code.

  40. Add option to opt into full-RBF when sending funds e272c26374
  41. petertodd force-pushed on Jun 15, 2016
  42. petertodd commented at 4:53 PM on June 15, 2016: contributor

    Rebased

  43. instagibbs commented at 1:44 PM on June 17, 2016: member

    Would this work with -mempoolreplacement=0 ? Seems like the node will reject these. Perhaps check for that flag too.

    further nit: anything "optin" will become "optout" if the default changes, so perhaps: "sendfullrbf"? Makes it clearer it's about sending, not mempool replacement.

    concept ACK, I'd really like something for 0.13.

  44. in src/wallet/wallet.cpp:None in e272c26374 
    2327 | +                // Note how the sequence number is set to non-maxint so that
2328 | +                // the nLockTime set above actually works.
2329 |                  BOOST_FOREACH(const PAIRTYPE(const CWalletTx*,unsigned int)& coin, setCoins)
2330 |                      txNew.vin.push_back(CTxIn(coin.first->GetHash(),coin.second,CScript(),
2331 | -                                              std::numeric_limits<unsigned int>::max()-1));
2332 | +                                              std::numeric_limits<unsigned int>::max() - (fOptIntoFullRbf ? 2 : 1)));
    jtimon commented at 2:15 PM on June 17, 2016:

    Maybe constants for 1 and 2?

    instagibbs commented at 2:28 PM on June 17, 2016:

    those numbers aren't special, the 1st number just has to be bigger than 1. Not sure if that's a candidate for constant.

    jtimon commented at 2:33 PM on June 17, 2016:

    Mqybe more documentation solves it. I just cannot look at that line without wondering what the heck 1 and 2 mean...

    instagibbs commented at 2:33 PM on June 17, 2016:

    Fair enough. The comment should be revised further.

    petertodd commented at 9:13 PM on June 18, 2016:

    How about this comment:

    "BIP125 defines opt-in RBF as any nSequence < maxint-1, so we use the highest possible value in that range (maxint-2) to avoid conflicting with other possible uses of nSequence, and in the spirit of "smallest posible change from prior behavior""

    MarcoFalke commented at 8:24 AM on August 20, 2016:

    Nit: Maybe we could introduce std::numeric_limits<unsigned int>::max()-1 as some constant somewhere. SEQUENCE_OPT_OUT = std::numeric_limits<unsigned int>::max()-1 is used in other places (mempool) as well.

    And then comment here "Use any value less than SEQUENCE_OPT_OUT according to BIP125." or use the comment you wrote above.

    jtimon commented at 9:37 AM on August 20, 2016: 
    fOptIntoFullRbf ? SEQUENCE_OPT_OUT -1 : SEQUENCE_OPT_OUT

    It's clrearer to me than:

     std::numeric_limits<unsigned int>::max() - (fOptIntoFullRbf ? 2 : 1)

    Specially with the ""Use any value less than SEQUENCE_OPT_OUT..." comment.

  45. luke-jr referenced this in commit 586d983a67 on Jun 28, 2016
  46. luke-jr referenced this in commit 978658a335 on Jun 28, 2016
  47. laanwj commented at 10:10 AM on August 19, 2016: member

    Is there anything blocking this? (besides needing rebase again, sorry)

  48. MarcoFalke commented at 8:25 AM on August 20, 2016: member

    Concept ACK. Needs rebase.

  49. laanwj commented at 10:17 AM on August 26, 2016: member

    Closing in favor of #8601

  50. laanwj closed this on Aug 26, 2016
  51. MarcoFalke locked this on Sep 8, 2021
Contributors
petertoddluke-jrdcousensjtimonMarcoFalkernicollghostgmaxwelljonasschnellilaanwjbtcdrakinstagibbs
Labels
FeatureWallet
Linked (view graph)
#8601 Add option to opt into full-RBF when sending funds (rebase, original by petertodd)
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 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me