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

petertodd · Nov 30, 2015

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

Defaults to sending txs with full-RBF off.

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

Perhaps -walletusefullrbf

@luke-jr What's your suggested name?

-walletrbf maybe?

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

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

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

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

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

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 ?

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

dcousens · Nov 30, 2015

Wasn't this already closed?
#7119

petertodd · Nov 30, 2015

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

rnicoll · Nov 30, 2015

utACK

ghost · Nov 30, 2015

utACK

dcousens · Dec 1, 2015

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.

gmaxwell · Dec 1, 2015

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

dcousens · Dec 1, 2015

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.

luke-jr · Dec 1, 2015

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.

petertodd · Dec 1, 2015

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

dcousens · Dec 1, 2015

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.

petertodd · Dec 2, 2015

Rebased

petertodd · Dec 2, 2015

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.

dcousens · Dec 2, 2015

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

petertodd · Dec 2, 2015

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)

dcousens · Dec 2, 2015

maxint-64

petertodd · Dec 2, 2015

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)

dcousens · Dec 2, 2015

@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?

petertodd · Dec 2, 2015

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)

dcousens · Dec 2, 2015

@petertodd my bad. Lost context.

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

petertodd · Dec 2, 2015

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

dcousens · Dec 3, 2015

Sure.

jtimon · Jan 3, 2016

Concept ACK

jonasschnelli · Jan 20, 2016

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.

laanwj · Apr 25, 2016

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

petertodd · Apr 25, 2016

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)

laanwj · Apr 25, 2016

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.

petertodd · May 20, 2016

Rebased

btcdrak · Jun 14, 2016

needs rebase

laanwj · Jun 14, 2016

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

petertodd · Jun 15, 2016

Rebased

instagibbs · Jun 17, 2016

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.

Maybe constants for 1 and 2?

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

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

Fair enough. The comment should be revised further.

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""

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.

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.

laanwj · Aug 19, 2016

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

MarcoFalke · Aug 20, 2016

Concept ACK. Needs rebase.

laanwj · Aug 26, 2016

Closing in favor of #8601

jonasschnelli added the Wallet label Nov 30, 2015

laanwj added the Feature label Dec 3, 2015

jonasschnelli referenced this pull request Dec 17, 2015
Merged
RPC: Indicate which transactions are signaling opt-in RBF #7222

jonasschnelli referenced this pull request Jan 14, 2016
Closed
Add NoRBF to URI Scheme #7343

laanwj added this to the 0.13.0 milestone Apr 1, 2016

laanwj removed this from the 0.13.0 milestone Apr 25, 2016

Add option to opt into full-RBF when sending funds

This commit was signed with a verified signature.

petertodd Peter Todd

GPG key ID: 6399011044E8AFB2 Learn about signing commits

e272c26

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 28, 2016

Merge #7132 walletrbf_on_knots

978658a

laanwj referenced this pull request Aug 26, 2016
Merged
Add option to opt into full-RBF when sending funds (rebase, original by petertodd) #8601

laanwj closed this Aug 26, 2016

@@ -405,6 +405,7 @@ std::string HelpMessage(HelpMessageMode mode)
          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));
          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)"),
              CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MAXFEE)));
+    +    strUsage += HelpMessageOpt("-optintofullrbf", strprintf(_("Send transactions with full-RBF opt-in enabled (default: %u)"), DEFAULT_OPT_INTO_FULL_RBF));

                      BOOST_FOREACH(const PAIRTYPE(const CWalletTx*,unsigned int)& coin, setCoins)
                          txNew.vin.push_back(CTxIn(coin.first->GetHash(),coin.second,CScript(),
-    -                                              std::numeric_limits<unsigned int>::max()-1));
+    +                                              std::numeric_limits<unsigned int>::max() - (fOptIntoFullRbf ? 2 : 1)));

bitcoin/bitcoin

Join GitHub today

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

Conversation

petertodd commented Nov 30, 2015

Show 11 comments Hide comments luke-jr and 4 others commented on an outdated diff Nov 30, 2015

dcousens commented Nov 30, 2015

petertodd commented Nov 30, 2015

jonasschnelli added the Wallet label Nov 30, 2015

rnicoll commented Nov 30, 2015

ghost commented Nov 30, 2015

dcousens commented Dec 1, 2015

gmaxwell commented Dec 1, 2015

dcousens commented Dec 1, 2015

luke-jr commented Dec 1, 2015

petertodd commented Dec 1, 2015

dcousens commented Dec 1, 2015

petertodd commented Dec 2, 2015

petertodd commented Dec 2, 2015

dcousens commented Dec 2, 2015

petertodd commented Dec 2, 2015

dcousens commented Dec 2, 2015

petertodd commented Dec 2, 2015

dcousens commented Dec 2, 2015

petertodd commented Dec 2, 2015

dcousens commented Dec 2, 2015

petertodd commented Dec 2, 2015

dcousens commented Dec 3, 2015

laanwj added the Feature label Dec 3, 2015

jonasschnelli referenced this pull request Dec 17, 2015

jtimon commented Jan 3, 2016

jonasschnelli referenced this pull request Jan 14, 2016

jonasschnelli commented Jan 20, 2016

laanwj added this to the 0.13.0 milestone Apr 1, 2016

laanwj commented Apr 25, 2016

laanwj removed this from the 0.13.0 milestone Apr 25, 2016

petertodd commented Apr 25, 2016

laanwj commented Apr 25, 2016

petertodd commented May 20, 2016

btcdrak commented Jun 14, 2016

laanwj commented Jun 14, 2016

petertodd commented Jun 15, 2016

instagibbs commented Jun 17, 2016

jtimon commented on the diff Jun 17, 2016

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 28, 2016

laanwj commented Aug 19, 2016

MarcoFalke commented Aug 20, 2016

laanwj referenced this pull request Aug 26, 2016

laanwj commented Aug 26, 2016

laanwj closed this Aug 26, 2016

luke-jr and 4 others commented on an outdated diff Nov 30, 2015