Add option to opt into full-RBF when sending funds by petertodd · Pull Request #7132 · bitcoin/bitcoin

petertodd · 2015-11-30T05:17:48Z

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

Defaults to sending txs with full-RBF off.

luke-jr · 2015-11-30T05:21:32Z

src/init.cpp

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

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 · 2015-11-30T05:33:23Z

Wasn't this already closed?
#7119

petertodd · 2015-11-30T05:35:44Z

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

rnicoll · 2015-11-30T12:59:32Z

utACK

ghost · 2015-11-30T18:50:13Z

utACK

dcousens · 2015-12-01T00:27:14Z

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 · 2015-12-01T01:03:48Z

@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 · 2015-12-01T01:07:00Z

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 · 2015-12-01T01:09:52Z

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 · 2015-12-01T01:10:58Z

@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 · 2015-12-01T01:17:43Z

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 · 2015-12-02T00:57:03Z

Rebased

petertodd · 2015-12-02T04:25:09Z

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 · 2015-12-02T04:31:33Z

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

petertodd · 2015-12-02T04:35:14Z

What do you mean by "100 nSequence bits"?

On 2 December 2015 12:32:11 GMT+08:00, Daniel Cousens [email protected] 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 · 2015-12-02T04:37:24Z

maxint-64

petertodd · 2015-12-02T04:40:48Z

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 [email protected] wrote:

maxint-64

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

dcousens · 2015-12-02T04:45:36Z

@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 · 2015-12-02T04:46:38Z

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 [email protected] 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 · 2015-12-02T04:49:55Z

@petertodd my bad. Lost context.

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

petertodd · 2015-12-02T08:58:34Z

@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 · 2015-12-03T02:58:21Z

Sure.

jtimon · 2016-01-03T19:50:53Z

Concept ACK

jonasschnelli · 2016-01-20T13:00:38Z

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 · 2016-04-25T09:42:44Z

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

petertodd · 2016-04-25T11:28:39Z

Why not both?

On 25 April 2016 05:43:48 GMT-04:00, "Wladimir J. van der Laan" [email protected] 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 · 2016-04-25T11:56:33Z

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 · 2016-05-20T14:49:25Z

Rebased

btcdrak · 2016-06-14T10:36:21Z

needs rebase

laanwj · 2016-06-14T11:24:49Z

@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 · 2016-06-15T16:53:35Z

Rebased

instagibbs · 2016-06-17T13:44:46Z

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.

jtimon · 2016-06-17T14:15:39Z

src/wallet/wallet.cpp

                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)));


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 · 2016-08-19T10:10:32Z

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

maflcko · 2016-08-20T08:25:12Z

Concept ACK. Needs rebase.

laanwj · 2016-08-26T10:17:34Z

Closing in favor of #8601

luke-jr reviewed Nov 30, 2015
View reviewed changes

jonasschnelli added the Wallet label Nov 30, 2015

petertodd force-pushed the 2015-11-opt-into-full-rbf-option branch from 412bf57 to 623f29d Compare December 1, 2015 01:05

petertodd force-pushed the 2015-11-opt-into-full-rbf-option branch from 623f29d to 5fc02e2 Compare December 2, 2015 00:56

petertodd force-pushed the 2015-11-opt-into-full-rbf-option branch from 5fc02e2 to 0292b63 Compare December 2, 2015 04:23

laanwj added the Feature label Dec 3, 2015

jonasschnelli mentioned this pull request Dec 17, 2015

RPC: Indicate which transactions are signaling opt-in RBF #7222

Merged

jonasschnelli mentioned this pull request Jan 14, 2016

Add NoRBF to URI Scheme #7343

Closed

laanwj added this to the 0.13.0 milestone Apr 1, 2016

laanwj removed this from the 0.13.0 milestone Apr 25, 2016

petertodd force-pushed the 2015-11-opt-into-full-rbf-option branch from 0292b63 to 630857a Compare May 20, 2016 14:48

Add option to opt into full-RBF when sending funds

e272c26

petertodd force-pushed the 2015-11-opt-into-full-rbf-option branch from 630857a to e272c26 Compare June 15, 2016 16:53

jtimon reviewed Jun 17, 2016
View reviewed changes

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

Merge bitcoin#7132 walletrbf_on_knots

978658a

laanwj mentioned this pull request Aug 26, 2016

Add option to opt into full-RBF when sending funds (rebase, original by petertodd) #8601

Merged

laanwj closed this Aug 26, 2016

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

petertodd commented Nov 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens commented Nov 30, 2015

Uh oh!

petertodd commented Nov 30, 2015

Uh oh!

rnicoll commented Nov 30, 2015

Uh oh!

ghost commented Nov 30, 2015

Uh oh!

dcousens commented Dec 1, 2015

Uh oh!

gmaxwell commented Dec 1, 2015

Uh oh!

dcousens commented Dec 1, 2015

Uh oh!

luke-jr commented Dec 1, 2015

Uh oh!

petertodd commented Dec 1, 2015

Uh oh!

dcousens commented Dec 1, 2015

Uh oh!

petertodd commented Dec 2, 2015

Uh oh!

petertodd commented Dec 2, 2015

Uh oh!

dcousens commented Dec 2, 2015

Uh oh!

petertodd commented Dec 2, 2015

Uh oh!

dcousens commented Dec 2, 2015

Uh oh!

petertodd commented Dec 2, 2015

Uh oh!

dcousens commented Dec 2, 2015

Uh oh!

petertodd commented Dec 2, 2015

Uh oh!

dcousens commented Dec 2, 2015

Uh oh!

petertodd commented Dec 2, 2015

Uh oh!

dcousens commented Dec 3, 2015

Uh oh!

jtimon commented Jan 3, 2016

Uh oh!

jonasschnelli commented Jan 20, 2016

Uh oh!

laanwj commented Apr 25, 2016

Uh oh!

petertodd commented Apr 25, 2016

Uh oh!

laanwj commented Apr 25, 2016

Uh oh!

petertodd commented May 20, 2016

Uh oh!

btcdrak commented Jun 14, 2016

Uh oh!

laanwj commented Jun 14, 2016