RPC/Mining: Restore API compatibility for prioritisetransaction

luke-jr commented at 4:45 pm on April 21, 2017: member

Breaking API serves no purpose other than to be incompatible with older versions and other implementations that do support priority

fanquake added the label RPC/REST/ZMQ on Apr 22, 2017

jonasschnelli added the label Mining on Apr 24, 2017

kallewoof commented at 7:43 am on May 1, 2017: member

utACK 9852bf5f6660cb02fae89918cbf5b11bf6c4138e

jnewbery commented at 9:23 pm on May 1, 2017: member

This was discussed in #9602 . I was on the fence about whether changing the prioritisetransaction RPC was an acceptable change, but group consensus seemed to be that it should be done. I’m still on the fence, but now that #9602 has been merged I don’t think it makes sense to re-open this issue.

As for rationale, I don’t think making RPC ‘compatible’ other implementations should be a consideration for bitcoin core. The RPC interface is an implementation decision. If other bitcoin implementations wish to implement the bitcoin core RPC interface that’s fine, but it shouldn’t drive core’s decisions.

MarcoFalke commented at 9:54 pm on May 1, 2017: member

utACK 9852bf5. Can you try to use named args in the test such that you don’t need to provide the dummy argument?

TheBlueMatt commented at 3:13 pm on May 2, 2017: member

If we’re gonna re-add the dummy value, it should be called “dummy” and explained that its a useless API-compat value (and should make require it to be 0 to ensure people aren’t confused).

luke-jr commented at 5:37 pm on May 2, 2017: member

@TheBlueMatt Besides the name (which would break named-arg compat), I think this PR already satisfies that description: the description explains it isn’t supported, and the code enforces it to be either null or 0.

sdaftuar commented at 3:59 pm on May 10, 2017: member

I don’t object if others think it’s better to avoid breaking the RPC API immediately by keeping this dummy argument around, but I’d say we should update the help text to also say that support for this option will be removed in the future, so that users can expect a future API change.

But I tend to think that we might as well do the full RPC API break now; removal of priority is inherently breaking the API for software that would try to set it…

MarcoFalke commented at 4:21 pm on May 10, 2017: member

I think it is preferable to break the API in a “smooth” way. I.e. allow that Bitcoin Core version 0.xx and 0.(xx+1) can be switched back and forth with minimal changes to the wrapper. This would allow faster upgrades to new releases and comes with a grace period with the duration of a full release cycle to address any breaking API changes.

On Wed, May 10, 2017 at 6:00 PM, Suhas Daftuar notifications@github.com wrote:

I don’t object if others think it’s better to avoid breaking the RPC API immediately by keeping this dummy argument around, but I’d say we should update the help text to also say that support for this option will be removed in the future, so that users can expect a future API change.

But I tend to think that we might as well do the full RPC API break now; removal of priority is inherently breaking the API for software that would try to set it…

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10252#issuecomment-300529548, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv83e-U895stnQPgq8h0Tx9YNibEJks5r4d8EgaJpZM4NEiQw .

luke-jr commented at 5:38 pm on May 10, 2017: member

@sdaftuar Priority still exists, even if Core doesn’t support it. The option isn’t obsolete, just not supported by Core. There’s no reason to remove it in the future, as that would not only break backward compatibility, but also break cross-node compatibility, and for no gain.

ryanofsky commented at 6:28 pm on May 22, 2017: member

utACK e50650dcf9e856245419dc76d0496a3ea408703b though needs rebase. This seems like an ideal change, since it:

Maintains compatibility for users setting 0 priority.
Provides a useful error message for users setting other priorities.
Updates tests to use named arguments.

Having a dummy priority parameter that doesn’t do anything doesn’t seem like a big drawback, but if it is a sticking point for merging this PR, maybe this PR could add new RPC that only sets the fee delta, and mark the prioritize RPC deprecated in favor of that new one.

luke-jr force-pushed on May 22, 2017

luke-jr commented at 8:55 pm on May 22, 2017: member

Rebased

in test/functional/prioritise_transaction.py:112 in 06a2d767ce outdated

108@@ -109,7 +109,7 @@ def run_test(self):
109         # This is a less than 1000-byte transaction, so just set the fee
110         # to be the minimum for a 1000 byte transaction and check that it is
111         # accepted.
112-        self.nodes[0].prioritisetransaction(tx_id, int(self.relayfee*COIN))
113+        self.nodes[0].prioritisetransaction(txid=tx_id, fee_delta=int(self.relayfee*COIN))

ryanofsky commented at 1:45 pm on May 23, 2017:

There’s another prioritisetransaction call below that needs to be updated (it’s causing travis tests to fail).

ryanofsky commented at 1:47 pm on May 23, 2017: member

utACK 06a2d767ce6e06d66ed4e4cfbd751d2ded179a86 with the additional fix for prioritise_transaction.py. Confirmed no changes since last review other than rebase & squash.

sipa commented at 4:40 pm on May 23, 2017: member

Concept ACK, but tests need fixing.

RPC/Mining: Restore API compatibility for prioritisetransaction

Breaking API serves no purpose other than to be incompatible with older versions and other implementations that do support priority

870824e919

luke-jr force-pushed on May 23, 2017

luke-jr commented at 5:07 am on May 24, 2017: member

Fixed

MarcoFalke commented at 10:26 am on June 2, 2017: member

re-utACK 870824e919474c5b39da73fe73199f8453fd540f

ryanofsky commented at 1:50 pm on June 2, 2017: member

utACK 870824e919474c5b39da73fe73199f8453fd540f. Only change since last review was the suggested test fix.

#10488 is an alternative to this PR which renames the priority argument to dummy and changes the wording of some documentation strings. I slightly prefer this PR because it’s more focused and more backwards-compatible, and I think if the any of the additional changes from #10488 are desirable, that PR can be just be rebased and merged after this one.

in test/functional/replace-by-fee.py:512 in 870824e919

508@@ -509,7 +509,7 @@ def test_prioritised_transactions(self):
509         assert_raises_jsonrpc(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx2b_hex, True)
510 
511         # Now prioritise tx2b to have a higher modified fee
512-        self.nodes[0].prioritisetransaction(tx2b.hash, int(0.1*COIN))
513+        self.nodes[0].prioritisetransaction(txid=tx2b.hash, fee_delta=int(0.1*COIN))

laanwj commented at 1:45 pm on June 6, 2017:

Thanks for converting these to name-based arguments.

laanwj merged this on Jun 6, 2017

laanwj closed this on Jun 6, 2017

laanwj referenced this in commit 980deaf0b1 on Jun 6, 2017

PastaPastaPasta referenced this in commit 491200c4e3 on Jul 17, 2019

PastaPastaPasta referenced this in commit 46e1e2f6f2 on Jul 17, 2019

MarcoFalke locked this on Sep 8, 2021

RPC/Mining: Restore API compatibility for prioritisetransaction #10252