refactor: Throw exception on invalid Univalue pushes over silent ignore #25551

1. 
   MarcoFalke commented at 10:01 am on July 6, 2022: member

   The return value of the push* helpers is never used, but important to determine if the operation was successful. One way to fix this would be to add the “nodiscard” attribute. However, this would make the code (and this diff) overly verbose for no reason.

   So fix it by removing the never used return value. Also, fail verbosely in case of a programming mistake.

2. MarcoFalke renamed this:
   refactor: Throw exception on invalid pushes over silent ignore
   refactor: Throw exception on invalid Univalue pushes over silent ignore
   on Jul 6, 2022
3. fanquake added the label Refactoring on Jul 6, 2022
4. 
   DrahtBot commented at 11:33 am on July 6, 2022: contributor

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #25617 (refactor: univalue test cleanups by fanquake)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

5. 
   furszy commented at 3:41 pm on July 6, 2022: member

   Concept ACK

   Would be good to add coverage for it with few BOOST_CHECK_THROW (one per changed method).

6. MarcoFalke force-pushed on Jul 6, 2022
7. 
   MarcoFalke commented at 4:27 pm on July 6, 2022: member

   Thanks, done
8. 
   fanquake commented at 3:02 pm on July 13, 2022: member

   cc @martinus @furszy want to take another look?
9. DrahtBot added the label Needs rebase on Jul 13, 2022
10. refactor: Default options in walletcreatefundedpsbt to VOBJ instead of VNULL

    This should not change behavior and makes the code consistent with other
    places.

    ccccc17b91
11. univalue: Throw exception on invalid pushes over silent ignore fa277cd55d
12. MarcoFalke force-pushed on Jul 13, 2022
13. DrahtBot removed the label Needs rebase on Jul 13, 2022
14. 
    MarcoFalke commented at 10:40 am on July 14, 2022: member

    Rebased
15. 
    furszy commented at 2:40 pm on July 14, 2022: member

    code ACK fa277cd5

    Non-blocking cosmetic nit:

    Could probably unify all the throw calls with something like this:

    0static void ThrowIfNotEqual(UniValue::VType type, UniValue::VType expected_type)
    1{
    2    if (type != expected_type) throw std::runtime_error{"JSON value is not an " + std::string(uvTypeName(type)) + " as expected"};
    3}
    

    Then just call ThrowIfNotEqual(typ, VARR) or ThrowIfNotEqual(typ, VOBJ) at the beginning of each function.

16. 
    MarcoFalke commented at 4:11 pm on July 14, 2022: member

    Could probably unify all the throw calls with something like this:

    Thanks, will do in a non-refactoring follow-up

17. fanquake merged this on Jul 15, 2022
18. fanquake closed this on Jul 15, 2022

    ---

19. in src/univalue/test/object.cpp:88 in fa277cd55d

    84@@ -85,6 +85,16 @@ BOOST_AUTO_TEST_CASE(univalue_constructor)
    85     BOOST_CHECK_EQUAL(v9.getValStr(), "zappa");
    86 }
    87 
    88+BOOST_AUTO_TEST_CASE(univalue_push_throw)
    

    ---

    ---

    

    fanquake commented at 3:44 pm on July 15, 2022:

    Only noticed on rebase these tests aren’t called. Addressed in 25617
20. in src/wallet/rpc/spend.cpp:1636 in fa277cd55d

    1632@@ -1633,7 +1633,7 @@ RPCHelpMan walletcreatefundedpsbt()
    1633         }, true
    1634     );
    1635 
    1636-    UniValue options = request.params[3];
    1637+    UniValue options{request.params[3].isNull() ? UniValue::VOBJ : request.params[3]};
    

    ---

    ---

    

    luke-jr commented at 3:38 pm on July 16, 2022:

    Is this well-defined, valid C++? At least in normal C, the ?: operator requires both results to be the same type…

    ---

    

    MarcoFalke commented at 4:06 pm on July 16, 2022:

    Yes, it should be, though it likely involves the copy-constructor. Pretty sure I copied it from the other places where this is used consistently.
21. MarcoFalke deleted the branch on Jul 16, 2022
22. sidhujag referenced this in commit 4cd8b1a151 on Jul 18, 2022
23. bitcoin locked this on Jul 16, 2023

Contributors
MarcoFalke DrahtBot furszy fanquake luke-jr

Labels
Refactoring