Thanks to @morcos and @luke-jr for pointing out that we were massively inconsistent between RPCs on IRC.
Note that none of these have been in a release, except "replaceable", which has been used in wallet tx descriptions in RPCs in 0.14.2, at least. Hence why I stuck with "replaceable" for all of them.
Also adds the missing named-argument option for createrawtransaction.
NACK, this breaks API compatibility with Knots for no reason. replacable also gives a false sense of other transactions not being replacable (which they are).
opt_into_rbf seems to be the sensible name, so use that and just provide backward compatibility aliases.
We do not have a framework for backwards compatibility aliases (but if you want to add one, feel free, dont think its worth doing for Knots, but we may need it at some point).
It is important to be consistent, and we use replaceable elsewhere in 0.14, so we should stick with that IMO, if folks want to paint the shed "opt_into_rbf", I'd be ok with that too.
This should likely be tagged 0.15.
Yes we do. See getblock's "verbosity" aka "verbose" param.
Ahh, didnt realize that, alright, well then its a super simple patch for Knots to carry to keep backwards compat with itself, no need to upstream it :).
in terms of the "other things arn't" -- this is why the list transaction output calls it BIP125 replaceable.
@TheBlueMatt The default should be compatibility. There is no reason to be incompatible.
@gmaxwell Good point, updated the docs to point to BIP 125. @luke Indeed, compatibility is the default, but that does not override the legitimate reason that we should be consistent in our naming, and I'm really not at all convinced that compatibility with previous unreleased code is really much of a preference.
@TheBlueMatt Yes, we should be consistent, but compatibility doesn't interfere in this at all. It's not unreleased code; it's been part of Knots for several releases now.
I generally think this change makes sense... though I'm worried about the API break.
Users/apps calling RPC with the then invalid optintorbf will not even get a notice that this has been deprecated/nuked, leading to the fact, that this could harm businesses.
Same could be true for the startup argument.
This is why I'm not sure if this is a "hard" improvement. Ideally, we deprecate the old parameter first while we introduce the new one... but this is maybe an overkill.
Keeping it as it is right now seems for me the safest path (it's just wording).
If we go with this PR, then it would certainly require a release-notes part.
317 | @@ -318,7 +318,8 @@ UniValue createrawtransaction(const JSONRPCRequest& request) 318 | " ,...\n" 319 | " }\n" 320 | "3. locktime (numeric, optional, default=0) Raw locktime. Non-0 value also locktime-activates inputs\n" 321 | - "4. optintorbf (boolean, optional, default=false) Allow this transaction to be replaced by a transaction with higher fees. If provided, it is an error if explicit sequence numbers are incompatible.\n" 322 | + "4. replaceable (boolean, optional, default=false) Marks this transaction as BIP125 replaceable.\n" 323 | + " Allowing this transaction to be replaced by a transaction with higher fees. If provided, it is an error if explicit sequence numbers are incompatible.\n"
I think Allow or Allows is better than Allowing.
2654 | @@ -2655,7 +2655,8 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) 2655 | " Those recipients will receive less bitcoins than you enter in their corresponding amount field.\n" 2656 | " If no outputs are specified here, the sender pays the fee.\n" 2657 | " [vout_index,...]\n" 2658 | - " \"optIntoRbf\" (boolean, optional) Allow this transaction to be replaced by a transaction with higher fees\n" 2659 | + " \"replaceable\" (boolean, optional) Marks this transaction as BIP125 replaceable.\n" 2660 | + " Allowing this transaction to be replaced by a transaction with higher fees\n"
as above - prefer Allow over Allowing.
ACK.
though I'm worried about the API break.
This isn't an API break. These arguments are not yet in a release. We should change them to be consistent before v0.15 is released.
If we go with this PR, then it would certainly require a release-notes part.
Yes - since these are new arguments.
Concept ACK (we've discussed this on IRC)
Note that none of these have been in a release, except "replaceable", which has been used in wallet tx descriptions in RPCs in 0.14.2, at least. Hence why I stuck with "replaceable" for all of them.
This means that we're in time to fix this. Breaking compatiblity is a non-issue for things that haven't been in a release. Everything in master should be considered unstable from an API point of view.
it's been part of Knots for several releases now.
I'm sorry, but we really can't take into account the release schedule of derived projects. By taking on patches that aren't in a release yet, you risk later incompatibility.
This means that we're in time to fix this. Breaking compatiblity is a non-issue for things that haven't been in a release. Everything in master should be considered unstable from an API point of view.
Ah. Yes. I though we have released the RBF opt-in argument in createrawtransaction and fundrawtransaction.
But right, we haven't so there is no API break. Sorry for the noise.
I'm sorry, but we really can't take into account the release schedule of derived projects. By taking on patches that aren't in a release yet, you risk later incompatibility.
If there was an argument for being incompatible, maybe, but when it is reasonable to be compatible, compatibility should be preferred. In this case, there is zero harm or overhead from being compatible, and opt_into_rbf is the clearer param name anyway.
NACK without that.
We do not have a framework for backwards compatibility aliases
Yes we do. See getblock's "verbosity" aka "verbose" param.
I wasn't aware of this, so I tried it out:
→ bitcoin-cli -named getblock blockhash=0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206 verbose=true
error code: -1
error message:
JSON value is not a boolean as expected
→ bitcoin-cli -named getblock blockhash=0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206 verbosity=true
{
"hash": "0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206",
...
Seems like the implementation for getblock with verbose argument has been broken in bitcoin-cli since #8704. Should be an easy fix, but it's a subtle bug and a good example of why being tricksy with aliases and arguments is probably a bad idea.
utACK
To be consistent with other places (and add the missing named
args entry for it).
To be consistent with other RPCs
To be consistent with RPC naming
Changed "Allowing"s to "Allows" as requested by @jnewbery.