Skip to content

RBF is not disabled by default anymore #686

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
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

RBF is not disabled by default anymore #686

wants to merge 2 commits into from

Conversation

knocte
Copy link
Contributor

@knocte knocte commented Apr 24, 2019

But it's not enabled by default either: the developer is forced to
make a decision if he wants to avoid the compile-time warnings.

This way we avoid the potential problems that could have been caused
by simply switching the default to be RFB-enabled, as discussed
originally here: #355

@knocte knocte mentioned this pull request Apr 24, 2019
Closed
@NicolasDorier
Copy link
Collaborator

Wait I don't understand why there is both OptInRBF and RBF. What is the rational @lontivero ?

If RBF is set to true we have RBF, else we don't....

#683

@knocte
Copy link
Contributor Author

knocte commented Apr 24, 2019 •
edited
Loading

Nicolas: Lucas didn't introduce two flags. After Lucas' PR, there was only OptInRBF. However, after this PR, calling it with the prefix "OptIn" wouldn't make much sense, so I renamed it to "RBF". However, I left the old property with an [Obsolete] attribute in order not to break API.

@NicolasDorier
Copy link
Collaborator

Ah ok.
Well I think we can rename fine in this case the change is 5 days ago we will not break too much...

@NicolasDorier
Copy link
Collaborator

I don't really like bothering users with a question they don't care or are unlikely to understand. (what the hell is RBF??)

@knocte knocte force-pushed the rbfIsNotDisabledByDefaultAnymore branch from 5e7b223 to c22cfa5 Compare April 24, 2019 06:46
@knocte
Copy link
Contributor Author

knocte commented Apr 24, 2019

Well I think we can rename fine in this case the change is 5 days ago we will not break too much...

Ok, fixed.

(what the hell is RBF??)

I can rename the parameter to replaceByFeeEnabled?

@NicolasDorier
Copy link
Collaborator

No it should be named RBF, but most users don't care about it and asking them a question if they don't care about it is not a good solution I think.

I am mixed... ping @lontivero what do you think?

@knocte
Copy link
Contributor Author

knocte commented Apr 24, 2019

asking them a question if they don't care about it is not a good solution I think.

Well, I can use XML documentation for that API, where we recommend to enable it unless they need to care about backwards compatibility.

@NicolasDorier
Copy link
Collaborator

That make sense.

@knocte knocte force-pushed the rbfIsNotDisabledByDefaultAnymore branch from c22cfa5 to acdde96 Compare April 24, 2019 06:58
@knocte
Copy link
Contributor Author

knocte commented Apr 24, 2019

That make sense.

Done, and rebased (and fixed build).

@knocte knocte force-pushed the rbfIsNotDisabledByDefaultAnymore branch from acdde96 to 6b8cab9 Compare April 24, 2019 07:02
NicolasDorier
NicolasDorier reviewed Apr 24, 2019
View reviewed changes
@knocte knocte force-pushed the rbfIsNotDisabledByDefaultAnymore branch from 6b8cab9 to ad44c79 Compare April 24, 2019 07:06
NicolasDorier
NicolasDorier reviewed Apr 24, 2019
View reviewed changes
@knocte knocte force-pushed the rbfIsNotDisabledByDefaultAnymore branch from ad44c79 to f1438b5 Compare April 24, 2019 07:37
@nopara73
Copy link
Contributor

Concept ACK.

Well I think we can rename fine in this case the change is 5 days ago we will not break too much...

Agree, renaming is fine, probably nobody using it yet.

@knocte
Copy link
Contributor Author

knocte commented Apr 24, 2019

nobody using it yet.

Then let's merge it asap and bump?

@lontivero
Copy link
Contributor

lontivero commented Apr 24, 2019 •
edited
Loading

No it should be named RBF, but most users don't care about it and asking them a question if they don't care about it is not a good solution I think.

I am mixed... ping @lontivero what do you think?

It is okay for me but let me be honest here: the name RBF sucks bad because the semantic is unclear, for example: what does it means IsRBF? Does it means that the tx is replaceable? It is confusing. Bitcoin core calls it OptInRBF (what also sucks) but its semantic is at least a bit more clear. I would call it ExplicitlySignalReplaceability (or MakeItReplaceable) or something like that in a no library component.
I think the high level wording are:

  • replaceable transactions to those signaling (iimplicitly/explicitly) replaceability,
  • replacement transactions to those that spend the same inputs that and unconfirmed tx signaling replaceability.

So, in the library it is okay to call it RBF but in higher level software should be called with a more meaningful name.

@NicolasDorier
Copy link
Collaborator

@knocte have you fixed all the warnings?

@knocte
Copy link
Contributor Author

knocte commented Apr 27, 2019

@knocte have you fixed all the warnings?

Oh well... I started doing this, and found out that Transaction.Sign() calls CreateTransactionBuilder()... Which means that we should add overloads on Sign() (or Transaction ctor) with RBF flag too.

I went for the latter, and oh my, the rabbit hole is very deep. I need help.

@knocte
Copy link
Contributor Author

knocte commented Apr 27, 2019

(I pushed my warnings-fixage WIP work as a 2nd commit)

@NicolasDorier
Copy link
Collaborator

@knocte I think that you just have to call with RBF=false on existing stuff.

@knocte
Copy link
Contributor Author

knocte commented Oct 20, 2020

@knocte I think that you just have to call with RBF=false on existing stuff.

Sorry for late reply, but I never understood the statement above. If I mark RBF=false on existing stuff, it would defeat the point of this PR.

Anyway I've been thinking about this lately, and as you Nicolas seem to always recommend TransactionBuilder API over Transaction API, would you accept marking Transaction type as Obsolete? This way I can live with it being RBF=false by default while TransactionBuilder has RBF enabled by default.

knocte added 2 commits October 20, 2020 15:16
@knocte
Nit: replace 0xFFFFFFFF with a constant, for readability and SSoT
77ee227
@knocte
TransactionBuilder: stop having RBF disabled by default
f8ed69b 
Old CreateTransactionBuilder() methods are now marked as Obsolete; the
new ones, introduced in this commit, will force the NBitcoin consumer
to make a decision about what kind of transactions will be created by
her/his TransactionBuilder instance:

* if using EnhanceOnChainPrivacy mode, RBF will be disabled and the
inputs & outputs will be shuffled;
* if using Lightning mode, RBF will be enabled (ReplaceByFee is very
important in this environment because fee rates might change a lot
since the moment when a channel was opened until when it needs to
be closed), and shuffling inputs or outputs will be disabled.

Why disabling shuffling for Lightning? Because according to the spec,
they need to be ordered lexicographically (e.g. the previous defaults
were causing a bug in DotNetLightning: [1]).

[1] joemphilips/DotNetLightning#133
@knocte knocte force-pushed the rbfIsNotDisabledByDefaultAnymore branch from eb592e0 to f8ed69b Compare October 21, 2020 10:58
@NicolasDorier NicolasDorier force-pushed the master branch 2 times, most recently from 725a14b to bbcaf18 Compare January 16, 2025 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NicolasDorier NicolasDorier NicolasDorier left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@knocte @NicolasDorier @nopara73 @lontivero