[24.x] rpc: treat univalue type check error as RPC_TYPE_ERROR, not RPC_MISC_ERROR #26147

1. 
   maflcko commented at 10:22 AM on September 21, 2022: member

   Not sure if we want to backport this, but if we do, this is the identical commits from https://github.com/bitcoin/bitcoin/pull/25737

2. rpc: treat univalue type check error as RPC_TYPE_ERROR, not RPC_MISC_ERROR

   By throwing a custom exception from `Univalue::checkType` (instead of a plain
   std::runtime_error) and catching it on the RPC server request handler.
   
   So we properly return RPC_TYPE_ERROR (-3) on arg type errors and
   not the general RPC_MISC_ERROR (-1).

   55566630c6
3. rpc: remove unneeded RPCTypeCheckArgument checks

   No-behavior change.
   
   Since #25629, we check the univalue type internally.

   e68d380797
4. fanquake added this to the milestone 24.0 on Sep 21, 2022
5. fanquake added the label Backport on Sep 21, 2022
6. fanquake approved
7. 
   fanquake commented at 4:07 PM on September 21, 2022: member

   ACK e68d380797918e655decb76fc11725197d6d5323

8. 
   luke-jr commented at 7:35 PM on September 24, 2022: member

   Concept ACK

9. in src/wallet/rpc/spend.cpp:1646 in e68d380797

   1642 | @@ -1644,7 +1643,6 @@ RPCHelpMan walletcreatefundedpsbt()
   1643 |      bool rbf{wallet.m_signal_rbf};
   1644 |      const UniValue &replaceable_arg = options["replaceable"];
   1645 |      if (!replaceable_arg.isNull()) {
   1646 | -        RPCTypeCheckArgument(replaceable_arg, UniValue::VBOOL);
   1647 |          rbf = replaceable_arg.isTrue();
   

   ---

   ---

   

   maflcko commented at 4:09 PM on September 29, 2022:

   This is a behavior change. Previously it would throw on non-bool, now it will translate it to false

   ---

   

   luke-jr commented at 5:31 PM on September 29, 2022:

   Sounds like a bug in our UniValue fork that will affect master as well?

10. 
    maflcko commented at 4:09 PM on September 29, 2022: member

    idk, seems too risky

11. maflcko closed this on Sep 29, 2022

    ---

12. 
    luke-jr commented at 5:32 PM on September 29, 2022: member

    What if we just take the first commit, and leave the redundant checks in place for 24.x?

13. 
    maflcko commented at 5:34 PM on September 29, 2022: member

    Yes, first commit looks ok

14. bitcoin deleted a comment on Oct 1, 2022
15. furszy deleted the branch on May 27, 2023
16. bitcoin locked this on May 26, 2024

Contributors

maflckofanquakeluke-jr

Milestone
24.0