Note that the prioritizetransaction dummy value is deprecated, and has no meaning #10488
pull TheBlueMatt wants to merge 1 commits into bitcoin:master from TheBlueMatt:2017-05-dummy-api changing 2 files +6 −5-
TheBlueMatt commented at 4:08 pm on May 31, 2017: memberThis 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: Compaibilityin 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, 2017in 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 aboutThe 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 namedfee_delta
argument if possible for forward compatibility.jnewbery commented at 9:15 pm on May 31, 2017: membertested ACK 99445847d43da86724aa49ef193f57c63a97f17b . A couple of documentation suggestions inline.fanquake added the label RPC/REST/ZMQ on Jun 1, 2017TheBlueMatt force-pushed on Jun 1, 2017MarcoFalke commented at 2:03 pm on June 6, 2017: memberNeeds rebase if still relevantTheBlueMatt force-pushed on Jun 6, 2017TheBlueMatt commented at 2:14 pm on June 6, 2017: memberRebased.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 requirepriority_delta==0
luke-jr commented at 6:55 pm on June 6, 2017:Hmm, good point.luke-jr changes_requestedRemove 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.
TheBlueMatt force-pushed on Jun 6, 2017ryanofsky commented at 8:26 pm on June 8, 2017: memberutACK 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: memberMentioned 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, 2017sipa commented at 7:44 pm on June 9, 2017: memberutACK 40796e1a9d01758ba61d0d68fd05574a4272e4eblaanwj commented at 11:00 am on June 12, 2017: memberutACK 40796e1laanwj merged this on Jun 12, 2017laanwj closed this on Jun 12, 2017
laanwj referenced this in commit fa1f106218 on Jun 12, 2017PastaPastaPasta referenced this in commit 639341fd14 on Jul 17, 2019PastaPastaPasta referenced this in commit 2ad480f41b on Jul 17, 2019DrahtBot locked this on Sep 8, 2021
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 13:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me