Skip to content
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

Be consistent in using "opt_into_rbf" parameter for Opt-In RBF #10745

Closed
wants to merge 1 commit into from

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Jul 5, 2017

Essentially just #10698 done right.

Also fixes missing param name for createrawtransaction.

@practicalswift
Copy link
Contributor

Concept ACK 3187d4ed98c62a4a094d14c6df8d52dad03c7972

@jnewbery
Copy link
Contributor

jnewbery commented Jul 5, 2017

I prefer #10698. This implementation causes an unnecessary API break for bumpfee between 0.14.2 and 0.15.0 (replaceable option becomes opt-into-rbf).

Your feedback in #10698 was that we shouldn't change the argument names because it'd break compatibility with a derived project. This implementation is changing those names anyway, so it seems like that's no longer an argument against #10698.

@luke-jr
Copy link
Member Author

luke-jr commented Jul 5, 2017

@jnewbery This implementation is completely compatible (eg, for bumpfee).

@morcos
Copy link
Member

morcos commented Jul 5, 2017

Also prefer #10698. Please just introduce your compatible argument names in Knots and let's stop bike shedding this and get back to doing real work.

@luke-jr
Copy link
Member Author

luke-jr commented Jul 5, 2017

Yes, please stop bike shedding by trying to make this incompatible for no reason whatsoever. The other day you even said you preferred opt_into_rbf...

@morcos
Copy link
Member

morcos commented Jul 5, 2017

I actually said I liked opt_in_rbf but that's besides the point. What I most like is not changing the name now from something that has already been released (replaceable) and is perfectly good.

Tying up this much time arguing about the names is ridiculous. I don't really care what name we use, but seems like everyone other than you wants to stick with replaceable and I have actual meaningful PR's open that could benefit from just having a decision and moving forward because they add the option to other RPC's.

@laanwj
Copy link
Member

laanwj commented Jul 6, 2017

Sigh, agree with @morcos here, do we really need a competing PR for this?

We have been over this: replaceable is already in 0.14, so renaming them all to opt_in_rbf (which is only used in newly introduced interfaces for 0.15) is a more serious interface break than renaming them all to replaceable. Both result in consistency of the interface.

@luke-jr
Copy link
Member Author

luke-jr commented Jul 6, 2017

This is not an interface break at all. It is completely backward compatible.

@laanwj
Copy link
Member

laanwj commented Jul 10, 2017

Closing as #10698 was merged

@laanwj laanwj closed this Jul 10, 2017
@luke-jr
Copy link
Member Author

luke-jr commented Jul 10, 2017

This is still needed, even if that means rebasing it. Please reopen.

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants