-
Notifications
You must be signed in to change notification settings - Fork 34
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.
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
doublespendinputs: allow RBF per default #118
Conversation
8f6acf4
to
3b50a5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Though I don't think any action is really required? Since the flag would only allow you to turn off RBF, which I don't think is required really.
// Add the inputs. | ||
for _, outpoint := range outpoints { | ||
tx.AddTxIn(wire.NewTxIn(outpoint, nil, nil)) | ||
|
||
tx.AddTxIn(&wire.TxIn{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AddTxIn
already uses wire.MaxTxInSequenceNum
under the hood. So I don't think any change is required? I don't think it makes sense to be able to turn off RBF?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the default it usse is indeed
// MaxTxInSequenceNum is the maximum sequence number the sequence field
// of a transaction input can be.
MaxTxInSequenceNum uint32 = 0xffffffff
but this value disallows RBF.
therefore to allow RBF this PR changes the default to
// MaxRBFSequence is the maximum sequence number an input can use to
// signal that the transaction spending it can be replaced using the
// Replace-By-Fee (RBF) policy.
MaxRBFSequence = 0xfffffffd
and introduces a switch to give the users the possibility to make a final transactions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right. I got the two values confused. So I guess we'll just want RBF to be the default, no need to turn it off. Removes the need for the boolean flag which has bad UX if the default is true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so i pushed new commit without the rbf flag.
fae020c
to
d9af0e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM 🎉
doublespendinputs
is designed to replace transactions, those new transactions should also be replacable by default.