TX Inputs to be RBF-enabled by default

[knocte]

When creating an input, there was a need to explicitly
use the Sequence property setter in order to enable RBF
on it.

Bitcoin Core has recently enabled RBF by default even
from the GUI [1], so it should be safe to assume that
this feature is stable enough to be enabled by default
in NBitcoin too.

The implementation to make this possible needs to use
a Nullable type for the backing field because the C#
compiler doesn't allow structs with parameterless ctors
or default values to be assigned to their fields.

[1] https://bitcoin.org/en/release/v0.16.0

[nopara73]

Concept ACK. There may be some short term privacy concerns, but in the long term as RBF adoption grows, the concerns would shift to the other side.

[NicolasDorier]

I don't like this, the build is breaking and I think tons of tests will break.

Anyway, choosing what is the default sequence to use is a work for the TransactionBuilder, not for the Sequence type.

I am really not sure about this change, I think lot's of code out there have the underlying assumption hat the default sequence is 0xFFFFFFFF. It might not even be for RBF, but some stuff would definitively break.

[knocte]

Last time I checked, it was only one test failing (my bad for not running them before proposing this). Gonna fix soon.

What's in your mind that would break, besides the RBF-by default test?

[knocte]

It's green now, only 1 test changed.

[NicolasDorier]

_ValueInv is used nowhere else?

[knocte]

Right, it's only used in assignments, and those keep working with Nullable.

[NicolasDorier]

Ok so my worry is the following:

Bob and Alice wants to sign a transaction in multi sig:

For doing that what happen is that Alice and Bob need to be able to build exactly the same transaction given the same Coins.

Now if Alice is using a new version of NBitcoin but not Bob, then they will be unable to generate the same transaction.

I think this has better its place as a feature in the TransactionBuilder disabled for now.

[knocte]

Mmm, I see where you're coming from.

The solution to this would be to break API, so that if one wants to upgrade NBitcoin, that person should make an explicit decision about if supporting RBF by default or not at that moment.

The way we could do it is mark Sequence type as private, create an ISequence interface, change all APIs to expose ISequence, and then create a SequenceFactory class that returns ISequence objects with overloads that explicitly define RbfEnabled or RbfDisabled as suffixes.

Is that a good plan?

If we never tackle this, people will keep creating non-RBF transactions with their wallets/solutions, and if we have high fees again soon, they will live in a world of pain.

[NicolasDorier]

This seems pretty complicated, I would just like a boolean TransactionBuilder.RBF

[knocte]

This seems pretty complicated, I would just like a boolean TransactionBuilder.RBF

Sorry for the delay replying. The follow-up question about this proposal would be: would that boolean property be true by default, or false? If your answer to this is true, then we have the same concerns that you were having about mixing versions of NBitcoin. And if your answer to this is false, then we would still have RBF-disabled by default, something which this PR wanted to address in the first place.

Another less complicated solution than my last proposal would be to make TransactionBuilder receive a bool parameter in its constructor called rbfEnabled. This way it's explicit, and everybody needs to make the decision at the moment of creating an instance of it (otherwise the compiler barks at you).

[nopara73]

The follow-up question about this proposal would be: would that boolean property be true by default, or false?

As discussed here, it'd be false.

[knocte]

As discussed here, it'd be false.

(As said in my previous comment,) then we would not have fixed anything.

[nopara73]

(As said in my previous comment,) then we would not have fixed anything.

There's nothing to fix. This is a feature.

[knocte]

There's nothing to fix.

Please read again the title of this PR :) And the first comment, to understand the motivations for this "fix".

[knocte]

And the first comment

Actually, the last sentence of this comment is better: If we never tackle this, people will keep creating non-RBF transactions with their wallets/solutions, and if we have high fees again soon, they will live in a world of pain.

[knocte]

Or another reason: when closing Lightning channels, having no RBF would be very dangerous (especially if there's a fraud/punishment transaction involved).

[nopara73]

Keep in mind RBF is not without issues.

• It only takes one wallet to implement an advanced RBF feature and the whole darknet will start using it opportunistically defrauding places those accept zero conf. This would lead to places to handle rbf transactions differently than non-rbf transactions. Which would lead to the need to stop using RBF.
• From a privacy point of view, using RBF is a wallet fingerprint. All system can create non-rbf transactions, but not all system can create rbf transactions, thus using RBF would help blockchain analysis to narrow the possible originator of the transaction, and this data can be useful for clustering.
• It is even more problematic if you actually use RBF, since the various different ways wallets use RBF leads to more accurate fingerprinting.
• RBF is not QC safe. You expose the pubkey when sending a transaction, QC will eventually get to the point of getting your privkey from the pubkey and override your original transaction. It doesn't happen if RBF is not used, since nodes would refuse the double spend.

Bitcoin Core has recently enabled RBF by default even
from the GUI [1], so it should be safe to assume that
this feature is stable enough to be enabled by default
in NBitcoin too.

Bitcoin Core is known for breaking userspace, quite the opposite, see 0.17.0 release mess.

So my vote is on non-enabling it by default, but I don't feel that strongly about it, so I'm fine with any decision, just please provide me a way to turn it off.

[knocte]

defrauding places those accept zero conf

Places should not accept zero conf ever anyway. If they do, they have a bug. And it's not that I like bitPay, but at least this company provides an insurance that lets you accept 0-conf and they pay cover the bill in case the tx gets double-spent in a different place.

From a privacy point of view, using RBF is a wallet fingerprint.

Well, there are 3 types of wallets: the ones that enable RBF in all transactions (a), the ones that don't enable it for any (b) and the ones that let the end-user choose (c). The only fingerprint this would leave is that you would know the user has not a wallet from the (b) category. Is that much of an issue?

It is even more problematic if you actually use RBF

Again there are so many wallets that fall into the (a) and (c) categories, compared to (b), that I don't see an issue here.

RBF is not QC safe.

If we ever have QC vulnerabilities in bitcoin, we will know immediately: the first funds that would be stolen would be Satoshi's accounts where he was receiving funds via public key. Worrying about much smaller amounts than Satoshi's amounts, is a bit paranoid. If you really want to help on this matter, work on Lamport signatures IMO. Double-spends can still happen without RBF enabled AFAIU.

[knocte]

Bitcoin Core is known for breaking userspace

Well, for them it's much harder to make their users be aware of the risks of upgrading, should they change the defaults. But NBitcoin is a library, if we make API changes, the developers need to explicitly change their code in order to upgrade, so our way of changing the defaults is easier than BitcoinCore. And I used bitcoin-core here as a reference-standard: if they did it, we shouldn't be ashamed of doing it either, especially when we can inform our users via the API.

[knocte]

So my vote is on non-enabling it by default

Take in account we have already discarded changing it to enabling it by default, because of Nicolas' concerns about mixing versions of NBitcoin when signing. What we're discussing here is to change the API so that the dev needs to explicitly set RBF true or false. This would be the only way to make it stop being disabled by default, while addressing Nicolas concern.

[knocte]

Superseded by #686