Note that the prioritizetransaction dummy value is deprecated, and has no meaning

TheBlueMatt commented at 4:08 pm on May 31, 2017: member

This is an alternative version of #10252, explaining that the first value is “dummy” instead of adding more references to priority now that we’ve removed it.

in src/rpc/mining.cpp:266 in 0ebbd8aef6 outdated

264+            "prioritisetransaction <txid> <dummy value> <fee delta>\n"
265             "Accepts the transaction into mined blocks at a higher (or lower) priority\n"
266             "\nArguments:\n"
267             "1. \"txid\"       (string, required) The transaction id.\n"
268-            "2. fee_delta      (numeric, required) The fee value (in satoshis) to add (or subtract, if negative).\n"
269+            "2. dummy          (numeric, optional) API-Compaibility for previous API. Must be zero or null.\n"

sipa commented at 4:11 pm on May 31, 2017:

Typo: Compaibility

in src/rpc/client.cpp:115 in 0ebbd8aef6 outdated

111@@ -112,7 +112,8 @@ static const CRPCConvertParam vRPCConvertParams[] =
112     { "estimaterawfee", 0, "nblocks" },
113     { "estimaterawfee", 1, "threshold" },
114     { "estimaterawfee", 2, "horizon" },
115-    { "prioritisetransaction", 1, "fee_delta" },
116+    { "prioritisetransaction", 1, "dummy" },

luke-jr commented at 5:15 pm on May 31, 2017:

This breaks the API compatibility…

TheBlueMatt commented at 7:17 pm on May 31, 2017:

I dont think we care for position-independant arguments? They’re still super new and I would be highly surprised if anyone is prioritizing by priority in 0.14, where its highly unsupported and disabled by default?

TheBlueMatt force-pushed on May 31, 2017

in src/rpc/mining.cpp:283 in 99445847d4 outdated

284     uint256 hash = ParseHashStr(request.params[0].get_str(), "txid");
285-    CAmount nAmount = request.params[1].get_int64();
286+    CAmount nAmount = request.params[2].get_int64();
287+
288+    if (!(request.params[1].isNull() || request.params[1].get_real() == 0)) {
289+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Priority is no longer supported, adjustment thereof must be zero.");

jnewbery commented at 9:13 pm on May 31, 2017:

adjustment thereof sounds a little tortured to me. How about The second argument must be set to 0.0

(simpler English and specifies precisely what the argument should be set to)

in src/rpc/mining.cpp:266 in 99445847d4 outdated

264+            "prioritisetransaction <txid> <dummy value> <fee delta>\n"
265             "Accepts the transaction into mined blocks at a higher (or lower) priority\n"
266             "\nArguments:\n"
267             "1. \"txid\"       (string, required) The transaction id.\n"
268-            "2. fee_delta      (numeric, required) The fee value (in satoshis) to add (or subtract, if negative).\n"
269+            "2. dummy          (numeric, optional) API-Compatibility for previous API. Must be zero or null.\n"

jnewbery commented at 9:15 pm on May 31, 2017:

Suggest you add a note that the second (dummy) argument is deprecated and may be removed in a future version. Clients should change to calling this RPC with named fee_delta argument if possible for forward compatibility.

jnewbery commented at 9:15 pm on May 31, 2017: member

tested ACK 99445847d43da86724aa49ef193f57c63a97f17b . A couple of documentation suggestions inline.

fanquake added the label RPC/REST/ZMQ on Jun 1, 2017

TheBlueMatt force-pushed on Jun 1, 2017

MarcoFalke commented at 2:03 pm on June 6, 2017: member

Needs rebase if still relevant

TheBlueMatt force-pushed on Jun 6, 2017

TheBlueMatt commented at 2:14 pm on June 6, 2017: member

Rebased.

in src/rpc/mining.cpp:284 in f81d5fd5f0 outdated

280@@ -280,7 +281,7 @@ UniValue prioritisetransaction(const JSONRPCRequest& request)
281     CAmount nAmount = request.params[2].get_int64();
282 
283     if (!(request.params[1].isNull() || request.params[1].get_real() == 0)) {
284-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Priority is not supported, and adjustment thereof must be zero.");
285+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Priority is no longer supported, first argument to prioritisetransaction must be 0.");

luke-jr commented at 5:42 pm on June 6, 2017:

Using named arguments, “first argument” may be confusing. Also suggest the recommendation should be null, not 0.

TheBlueMatt commented at 7:13 pm on June 6, 2017:

Fixed.

in src/rpc/mining.cpp:968 in f81d5fd5f0 outdated

964@@ -964,7 +965,7 @@ static const CRPCCommand commands[] =
965   //  --------------------- ------------------------  -----------------------  ----------
966     { "mining",             "getnetworkhashps",       &getnetworkhashps,       true,  {"nblocks","height"} },
967     { "mining",             "getmininginfo",          &getmininginfo,          true,  {} },
968-    { "mining",             "prioritisetransaction",  &prioritisetransaction,  true,  {"txid","priority_delta","fee_delta"} },
969+    { "mining",             "prioritisetransaction",  &prioritisetransaction,  true,  {"txid","dummy","fee_delta"} },

luke-jr commented at 5:42 pm on June 6, 2017:

This breaks the API compatibility. IMO leave it alone here, and just update the help.

MarcoFalke commented at 5:57 pm on June 6, 2017:

Compatibility is already broken, as we require priority_delta==0

luke-jr commented at 6:55 pm on June 6, 2017:

Hmm, good point.

luke-jr changes_requested

Remove references to priority that snuck back in in 870824e9.

The "priority" field should be appropriately marked as a "dummy"
value and noted that it is deprecated and will likely be removed.

40796e1a9d

TheBlueMatt force-pushed on Jun 6, 2017

ryanofsky commented at 8:26 pm on June 8, 2017: member

utACK 40796e1a9d01758ba61d0d68fd05574a4272e4eb.

Maybe rename PR to something like “Improve documentation for prioritisetransaction” since this isn’t really about restoring API compatibility anymore.

Also, if you wanted to update the release notes here to mention the priority_delta->dummy rename, and requirement to pass 0 or null, that would be good.

TheBlueMatt commented at 4:18 pm on June 9, 2017: member

Mentioned the release notes at #9889, updated PR title.

TheBlueMatt renamed this:
~~RPC/Mining: Restore API compatibility for prioritisetransaction~~
Note that the prioritizetransaction dummy value is deprecated, and has no meaning
on Jun 9, 2017

sipa commented at 7:44 pm on June 9, 2017: member

utACK 40796e1a9d01758ba61d0d68fd05574a4272e4eb

laanwj commented at 11:00 am on June 12, 2017: member

utACK 40796e1

laanwj merged this on Jun 12, 2017

laanwj closed this on Jun 12, 2017

laanwj referenced this in commit fa1f106218 on Jun 12, 2017

PastaPastaPasta referenced this in commit 639341fd14 on Jul 17, 2019

PastaPastaPasta referenced this in commit 2ad480f41b on Jul 17, 2019

DrahtBot locked this on Sep 8, 2021

Note that the prioritizetransaction dummy value is deprecated, and has no meaning #10488