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
  1. 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.
  2. 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
  3. 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?
  4. TheBlueMatt force-pushed on May 31, 2017
  5. 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)

  6. 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.
  7. jnewbery commented at 9:15 pm on May 31, 2017: member
    tested ACK 99445847d43da86724aa49ef193f57c63a97f17b . A couple of documentation suggestions inline.
  8. fanquake added the label RPC/REST/ZMQ on Jun 1, 2017
  9. TheBlueMatt force-pushed on Jun 1, 2017
  10. MarcoFalke commented at 2:03 pm on June 6, 2017: member
    Needs rebase if still relevant
  11. TheBlueMatt force-pushed on Jun 6, 2017
  12. TheBlueMatt commented at 2:14 pm on June 6, 2017: member
    Rebased.
  13. 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.
  14. 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.
  15. luke-jr changes_requested
  16. 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
  17. TheBlueMatt force-pushed on Jun 6, 2017
  18. 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.

  19. TheBlueMatt commented at 4:18 pm on June 9, 2017: member
    Mentioned the release notes at #9889, updated PR title.
  20. 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
  21. sipa commented at 7:44 pm on June 9, 2017: member
    utACK 40796e1a9d01758ba61d0d68fd05574a4272e4eb
  22. laanwj commented at 11:00 am on June 12, 2017: member
    utACK 40796e1
  23. laanwj merged this on Jun 12, 2017
  24. laanwj closed this on Jun 12, 2017

  25. laanwj referenced this in commit fa1f106218 on Jun 12, 2017
  26. PastaPastaPasta referenced this in commit 639341fd14 on Jul 17, 2019
  27. PastaPastaPasta referenced this in commit 2ad480f41b on Jul 17, 2019
  28. DrahtBot 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-30 00:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me