rpc: Remove deprecated migration code #18495

pull vasild wants to merge 1 commits into bitcoin:master from vasild:remove_deprecated_migration_code changing 2 files +10 −20
  1. vasild commented at 2:24 PM on April 1, 2020: member

    Don't accept a second argument to sendrawtransaction and testmempoolaccept of type bool. Actually even the code before this change would not accept bool, but it would print a long explanatory message when rejecting it: "Second argument must be numeric (maxfeerate) and no longer supports a boolean. To allow a transaction with high fees, set maxfeerate to 0."

    This was scheduled for removal in 6c0a6f73e.

  2. vasild commented at 2:33 PM on April 1, 2020: member

    @kallewoof, you scheduled this for removal in 6c0a6f73e. Maybe you want to take a look?

  3. laanwj added the label RPC/REST/ZMQ on Apr 1, 2020
  4. MarcoFalke added this to the milestone 0.21.0 on Apr 1, 2020
  5. kallewoof approved
  6. kallewoof commented at 2:35 AM on April 2, 2020: member

    Concept ACK and light code review ACK

  7. MarcoFalke commented at 1:04 PM on April 2, 2020: member

    Tests are failing

  8. vasild force-pushed on Apr 2, 2020
  9. in src/rpc/rawtransaction.cpp:828 in 568cbfa13e outdated 
     824 | @@ -825,7 +825,7 @@ static UniValue sendrawtransaction(const JSONRPCRequest& request)
 825 |  
 826 |      RPCTypeCheck(request.params, {
 827 |          UniValue::VSTR,
 828 | -        UniValueType(), // NUM or BOOL, checked later
 829 | +        UniValueType(), // VNUM or VSTR, checked inside AmountFromValue()
    vasild commented at 7:08 PM on April 2, 2020:

    The previous comment said "NUM or BOOL" but it would actually accept "NUM or BOOL or STR" - we allowed BOOL explicitly and if not BOOL, then passed it to AmountFromValue() which would accept NUM or STR and throw otherwise.

    MarcoFalke commented at 5:32 PM on April 10, 2020:

    side-note: NUM or STR is often just referred to as AMOUNT

  10. vasild commented at 9:46 AM on April 3, 2020: member

    Tests are failing

    Fixed the failing tests - the reason was that previously we allowed either number or string for maxfeerate and I mistakenly made it stricter with this PR to accept only number. Relaxed it back to accept number or string. This PR is a non-functional change and should not have externally visible effects.

    AppVeyor is failing with HTTP 504 - seemingly due to github or some proxy in between being down.

  11. DrahtBot commented at 7:50 PM on April 5, 2020: member

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  12. in src/rpc/rawtransaction.cpp:838 in 568cbfa13e outdated 
     840 | -        throw JSONRPCError(RPC_INVALID_PARAMETER, "Second argument must be numeric (maxfeerate) and no longer supports a boolean. To allow a transaction with high fees, set maxfeerate to 0.");
 841 | -    } else if (!request.params[1].isNull()) {
 842 | -        max_raw_tx_fee_rate = CFeeRate(AmountFromValue(request.params[1]));
 843 | -    }
 844 | +    const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull()
 845 | +        ? DEFAULT_MAX_RAW_TX_FEE_RATE
    MarcoFalke commented at 5:33 PM on April 10, 2020:

    our clang-format tells me that the question mark goes on the previous line:

    diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 2f44fd607a..0085954689 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -834,9 +834,9 @@ static UniValue sendrawtransaction(const JSONRPCRequest& request)
         throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
     CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
 
-    const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull()
-        ? DEFAULT_MAX_RAW_TX_FEE_RATE
-        : CFeeRate(AmountFromValue(request.params[1]));
+    const CFeeRate max_raw_tx_fee_rate{request.params[1].isNull() ?
+                                           DEFAULT_MAX_RAW_TX_FEE_RATE :
+                                           CFeeRate(AmountFromValue(request.params[1]))};
 
     int64_t virtual_size = GetVirtualTransactionSize(*tx);
     CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size);
@@ -906,9 +906,9 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
     CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
     const uint256& tx_hash = tx->GetHash();
 
-    const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull()
-        ? DEFAULT_MAX_RAW_TX_FEE_RATE
-        : CFeeRate(AmountFromValue(request.params[1]));
+    const CFeeRate max_raw_tx_fee_rate{request.params[1].isNull() ?
+                                           DEFAULT_MAX_RAW_TX_FEE_RATE :
+                                           CFeeRate(AmountFromValue(request.params[1]))};
 
     CTxMemPool& mempool = EnsureMemPool();
     int64_t virtual_size = GetVirtualTransactionSize(*tx);
    vasild commented at 7:34 PM on April 10, 2020:

    I reformatted it as above.

    However, when I run clang-format it would put everything on one line (145 chars long). This is because ./src/.clang-format contains ColumnLimit: 0 (no max line length).

    How did you run clang-format so that it would break the line? Have you changed ColumnLimit locally from 0 to 90? I will probably do this myself because I don't like the unlimited line length, but it would be cumbersome to drag this local mod all over the place and I wonder if there is something smarter...

    MarcoFalke commented at 8:11 PM on April 10, 2020:

    See https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy

    I run

    $ clang-format --version
clang-format version 10.0.0
    vasild commented at 6:54 PM on April 11, 2020:

    Strange.

    $ git checkout 568cbfa # the commit before I applied the above suggestions
$ git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v
Formatting src/rpc/rawtransaction.cpp
$ git diff

    produces the following (the max_raw_tx_fee_rate assignment is all in one, long line):

    diff --git i/src/rpc/rawtransaction.cpp w/src/rpc/rawtransaction.cpp
index 2f44fd607..0fd6597a8 100644
--- i/src/rpc/rawtransaction.cpp
+++ w/src/rpc/rawtransaction.cpp
@@ -821,25 +821,23 @@ static UniValue sendrawtransaction(const JSONRPCRequest& request)
             "\nAs a JSON-RPC call\n"
             + HelpExampleRpc("sendrawtransaction", "\"signedhex\"")
                 },
     }.Check(request);
 
     RPCTypeCheck(request.params, {
-        UniValue::VSTR,
-        UniValueType(), // VNUM or VSTR, checked inside AmountFromValue()
-    });
+                                     UniValue::VSTR,
+                                     UniValueType(), // VNUM or VSTR, checked inside AmountFromValue()
+                                 });
 
     // parse hex string from parameter
     CMutableTransaction mtx;
     if (!DecodeHexTx(mtx, request.params[0].get_str()))
         throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
     CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
 
-    const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull()
-        ? DEFAULT_MAX_RAW_TX_FEE_RATE
-        : CFeeRate(AmountFromValue(request.params[1]));
+    const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull() ? DEFAULT_MAX_RAW_TX_FEE_RATE : CFeeRate(AmountFromValue(request.params[1]));
 
     int64_t virtual_size = GetVirtualTransactionSize(*tx);
     CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size);
 
     std::string err_string;
     AssertLockNotHeld(cs_main);
@@ -888,30 +886,28 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
             "\nAs a JSON-RPC call\n"
             + HelpExampleRpc("testmempoolaccept", "[\"signedhex\"]")
                 },
     }.Check(request);
 
     RPCTypeCheck(request.params, {
-        UniValue::VARR,
-        UniValueType(), // VNUM or VSTR, checked inside AmountFromValue()
-    });
+                                     UniValue::VARR,
+                                     UniValueType(), // VNUM or VSTR, checked inside AmountFromValue()
+                                 });
 
     if (request.params[0].get_array().size() != 1) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Array must contain exactly one raw transaction for now");
     }
 
     CMutableTransaction mtx;
     if (!DecodeHexTx(mtx, request.params[0].get_array()[0].get_str())) {
         throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
     }
     CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
     const uint256& tx_hash = tx->GetHash();
 
-    const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull()
-        ? DEFAULT_MAX_RAW_TX_FEE_RATE
-        : CFeeRate(AmountFromValue(request.params[1]));
+    const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull() ? DEFAULT_MAX_RAW_TX_FEE_RATE : CFeeRate(AmountFromValue(request.params[1]));
 
     CTxMemPool& mempool = EnsureMemPool();
     int64_t virtual_size = GetVirtualTransactionSize(*tx);
     CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size);
 
     UniValue result(UniValue::VARR);

    $ clang-format --version
clang-format version 10.0.0 (git@github.com:freebsd/freebsd-ports.git aa0d93977b94fab30bf46ab1f38fe4f2576600eb)
    MarcoFalke commented at 8:35 PM on April 11, 2020:

    Jup, if the ? is on a new line to start with, all lines will get compressed. Though, if the ? is right before a newline character, they won't get compressed. Idk why, just clang-format being clang-format.

  13. in src/rpc/rawtransaction.cpp:914 in 568cbfa13e outdated 
     905 | @@ -910,13 +906,9 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
 906 |      CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
 907 |      const uint256& tx_hash = tx->GetHash();
 908 |  
 909 | -    CFeeRate max_raw_tx_fee_rate = DEFAULT_MAX_RAW_TX_FEE_RATE;
 910 | -    // TODO: temporary migration code for old clients. Remove in v0.20
    MarcoFalke commented at 5:34 PM on April 10, 2020:

    Does this breaking change to the RPC API need to go through an -deprecatedrpc cycle? See https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md#versioning

    vasild commented at 7:39 PM on April 10, 2020:

    Maybe not because it is already broken :) That is - even in master now a bool argument is not accepted.

    MarcoFalke commented at 8:09 PM on April 10, 2020:

    lolwut

    Could add a test to this pull to check that it still fails?

  14. MarcoFalke commented at 5:34 PM on April 10, 2020: member

    Concept ACK

  15. rpc: Remove deprecated migration code
    Don't accept a second argument to `sendrawtransaction` and
`testmempoolaccept` of type `bool`. Actually even the code before this
change would not accept `bool`, but it would print a long explanatory
message when rejecting it: "Second argument must be numeric (maxfeerate)
and no longer supports a boolean. To allow a transaction with high fees,
set maxfeerate to 0."

This was scheduled for removal in 6c0a6f73e.
    2599d13c94
  16. vasild force-pushed on Apr 10, 2020
  17. MarcoFalke commented at 12:46 PM on April 12, 2020: member

    ACK 2599d13c9417dc8c5107535521173687ec5e6c2f 📅

    Tested that with current master this throws an error: "Second argument must be numeric (maxfeerate) and no longer supports a boolean. To allow a transaction with high fees, set maxfeerate to 0. (code -8)"

    Tested that the current code throws a new error: "Amount is not a number or string (code -3)"

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 2599d13c9417dc8c5107535521173687ec5e6c2f 📅


Tested that with current master this throws an error: "Second argument must be numeric (maxfeerate) and no longer supports a boolean. To allow a transaction with high fees, set maxfeerate to 0. (code -8)"

Tested that the current code throws a new error: "Amount is not a number or string (code -3)"
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgypQwApp9/9v5Q0salUez7sYPkhlPjSXup/SjLvXQuSl6Lp9yMbNZ2CcT33b9I
0Y3ouSGXUyRQZfvCJNWY7g7E6J2/4BygHB9G+Eyu+gjgSo3n60Afo7ChdpOHsoGz
w25S7kle8Bb15RoCsk4UgDyIAriOQRx9U2Tqham90nxgz7ybjEocBRoSUQBCkgyz
2orshMmqV4J0yGT+1pnI/hKP9CQGQlG354tVuFBsSveep1Cd0nfWfyWh8FoCWnls
6KuU1p52NOPhjAdgbFluly9tbdUfTVoOGyXx7+c2VHCYlNn9lVQ2Da1f94BcxHms
NPMGAc0CDHyo9J5KhVbs0bSa0syaNAiwkBlDhm8G6DqIcgxLCx7eyTw9yucTOBdS
lCcEBs1KZuFRlclzn36HdxiKFdiUFZDjRmi07lyiPWKJAVt3JkwpPI6cpID4uBPf
XgwnuKE4Curx2CR3N9IbY1RHQtAVNjUomuq2aJL00xBWjenTfzyZ0wzOAvbPmRyD
ZYmHGvdy
=6gMz
-----END PGP SIGNATURE-----

    Timestamp of file with hash 06e77ffb97ea2db249501f57e32aee53a8c41f3a6aeb99beec0870b2214a3776 -

    </details>

  18. MarcoFalke merged this on Apr 12, 2020
  19. MarcoFalke closed this on Apr 12, 2020
  20. vasild deleted the branch on Apr 13, 2020
  21. ComputerCraftr referenced this in commit 70b87c210b on Jun 10, 2020
  22. Fabcien referenced this in commit fe5862ed71 on Jan 15, 2021
  23. PhotoshiNakamoto referenced this in commit b8c66d83d4 on Dec 13, 2021
  24. DrahtBot locked this on Feb 15, 2022
Contributors
vasildkallewoofMarcoFalkeDrahtBot
Labels
RPC/REST/ZMQ

Milestone
0.21.0

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: 2026-04-13 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me