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

fa277cd55dd105018e7d1220b4c3d96779e6b0f4 univalue: Throw exception on invalid pushes over silent ignore (MacroFake)
ccccc17b91698aa09ac85f7efea298f3938594ad refactor: Default options in walletcreatefundedpsbt to VOBJ instead of VNULL (MacroFake)

Pull request description:

  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.

ACKs for top commit:
  furszy:
    code ACK fa277cd5

Tree-SHA512: ef212a5bf5ae6bbad20acc4dafa3715521e81544185988d1eab724f440e4864a27e686aff51d5bc51b3017892c2eb8e577bcb8f37e8ddbaa0d8833bb622f2f9c

4 files changed, 50 insertions, 49 deletions

diff --git a/src/univalue/include/univalue.h b/src/univalue/include/univalue.h
index 7f9a6aaffd..5cb8268472 100644
--- a/src/univalue/include/univalue.h
+++ b/src/univalue/include/univalue.h
@@ -82,14 +82,14 @@ public:
     bool isArray() const { return (typ == VARR); }
     bool isObject() const { return (typ == VOBJ); }
 
-    bool push_back(const UniValue& val);
-    bool push_backV(const std::vector<UniValue>& vec);
+    void push_back(const UniValue& val);
+    void push_backV(const std::vector<UniValue>& vec);
     template <class It>
-    bool push_backV(It first, It last);
+    void push_backV(It first, It last);
 
     void __pushKV(const std::string& key, const UniValue& val);
-    bool pushKV(const std::string& key, const UniValue& val);
-    bool pushKVs(const UniValue& obj);
+    void pushKV(const std::string& key, const UniValue& val);
+    void pushKVs(const UniValue& obj);
 
     std::string write(unsigned int prettyIndent = 0,
                       unsigned int indentLevel = 0) const;
@@ -140,11 +140,10 @@ public:
 };
 
 template <class It>
-bool UniValue::push_backV(It first, It last)
+void UniValue::push_backV(It first, It last)
 {
-    if (typ != VARR) return false;
+    if (typ != VARR) throw std::runtime_error{"JSON value is not an array as expected"};
     values.insert(values.end(), first, last);
-    return true;
 }
 
 enum jtokentype {
diff --git a/src/univalue/lib/univalue.cpp b/src/univalue/lib/univalue.cpp
index 3553995c28..ac6f31a5a9 100644
--- a/src/univalue/lib/univalue.cpp
+++ b/src/univalue/lib/univalue.cpp
@@ -108,53 +108,45 @@ bool UniValue::setObject()
     return true;
 }
 
-bool UniValue::push_back(const UniValue& val_)
+void UniValue::push_back(const UniValue& val_)
 {
-    if (typ != VARR)
-        return false;
+    if (typ != VARR) throw std::runtime_error{"JSON value is not an array as expected"};
 
     values.push_back(val_);
-    return true;
 }
 
-bool UniValue::push_backV(const std::vector<UniValue>& vec)
+void UniValue::push_backV(const std::vector<UniValue>& vec)
 {
-    if (typ != VARR)
-        return false;
+    if (typ != VARR) throw std::runtime_error{"JSON value is not an array as expected"};
 
     values.insert(values.end(), vec.begin(), vec.end());
-
-    return true;
 }
 
 void UniValue::__pushKV(const std::string& key, const UniValue& val_)
 {
+    if (typ != VOBJ) throw std::runtime_error{"JSON value is not an object as expected"};
+
     keys.push_back(key);
     values.push_back(val_);
 }
 
-bool UniValue::pushKV(const std::string& key, const UniValue& val_)
+void UniValue::pushKV(const std::string& key, const UniValue& val_)
 {
-    if (typ != VOBJ)
-        return false;
+    if (typ != VOBJ) throw std::runtime_error{"JSON value is not an object as expected"};
 
     size_t idx;
     if (findKey(key, idx))
         values[idx] = val_;
     else
         __pushKV(key, val_);
-    return true;
 }
 
-bool UniValue::pushKVs(const UniValue& obj)
+void UniValue::pushKVs(const UniValue& obj)
 {
-    if (typ != VOBJ || obj.typ != VOBJ)
-        return false;
+    if (typ != VOBJ || obj.typ != VOBJ) throw std::runtime_error{"JSON value is not an object as expected"};
 
     for (size_t i = 0; i < obj.keys.size(); i++)
         __pushKV(obj.keys[i], obj.values.at(i));
-
-    return true;
 }
 
 void UniValue::getObjMap(std::map<std::string,UniValue>& kv) const
diff --git a/src/univalue/test/object.cpp b/src/univalue/test/object.cpp
index 8a35bf914d..196b536164 100644
--- a/src/univalue/test/object.cpp
+++ b/src/univalue/test/object.cpp
@@ -85,6 +85,16 @@ BOOST_AUTO_TEST_CASE(univalue_constructor)
     BOOST_CHECK_EQUAL(v9.getValStr(), "zappa");
 }
 
+BOOST_AUTO_TEST_CASE(univalue_push_throw)
+{
+    UniValue j;
+    BOOST_CHECK_THROW(j.push_back(1), std::runtime_error);
+    BOOST_CHECK_THROW(j.push_backV({1}), std::runtime_error);
+    BOOST_CHECK_THROW(j.__pushKV("k", 1), std::runtime_error);
+    BOOST_CHECK_THROW(j.pushKV("k", 1), std::runtime_error);
+    BOOST_CHECK_THROW(j.pushKVs({}), std::runtime_error);
+}
+
 BOOST_AUTO_TEST_CASE(univalue_typecheck)
 {
     UniValue v1;
@@ -198,13 +208,13 @@ BOOST_AUTO_TEST_CASE(univalue_array)
     UniValue arr(UniValue::VARR);
 
     UniValue v((int64_t)1023LL);
-    BOOST_CHECK(arr.push_back(v));
+    arr.push_back(v);
 
     std::string vStr("zippy");
-    BOOST_CHECK(arr.push_back(vStr));
+    arr.push_back(vStr);
 
     const char *s = "pippy";
-    BOOST_CHECK(arr.push_back(s));
+    arr.push_back(s);
 
     std::vector<UniValue> vec;
     v.setStr("boing");
@@ -213,13 +223,13 @@ BOOST_AUTO_TEST_CASE(univalue_array)
     v.setStr("going");
     vec.push_back(v);
 
-    BOOST_CHECK(arr.push_backV(vec));
+    arr.push_backV(vec);
 
-    BOOST_CHECK(arr.push_back((uint64_t) 400ULL));
-    BOOST_CHECK(arr.push_back((int64_t) -400LL));
-    BOOST_CHECK(arr.push_back((int) -401));
-    BOOST_CHECK(arr.push_back(-40.1));
-    BOOST_CHECK(arr.push_back(true));
+    arr.push_back(uint64_t{400ULL});
+    arr.push_back(int64_t{-400LL});
+    arr.push_back(int{-401});
+    arr.push_back(-40.1);
+    arr.push_back(true);
 
     BOOST_CHECK_EQUAL(arr.empty(), false);
     BOOST_CHECK_EQUAL(arr.size(), 10);
@@ -260,39 +270,39 @@ BOOST_AUTO_TEST_CASE(univalue_object)
 
     strKey = "age";
     v.setInt(100);
-    BOOST_CHECK(obj.pushKV(strKey, v));
+    obj.pushKV(strKey, v);
 
     strKey = "first";
     strVal = "John";
-    BOOST_CHECK(obj.pushKV(strKey, strVal));
+    obj.pushKV(strKey, strVal);
 
     strKey = "last";
-    const char *cVal = "Smith";
-    BOOST_CHECK(obj.pushKV(strKey, cVal));
+    const char* cVal = "Smith";
+    obj.pushKV(strKey, cVal);
 
     strKey = "distance";
-    BOOST_CHECK(obj.pushKV(strKey, (int64_t) 25));
+    obj.pushKV(strKey, int64_t{25});
 
     strKey = "time";
-    BOOST_CHECK(obj.pushKV(strKey, (uint64_t) 3600));
+    obj.pushKV(strKey, uint64_t{3600});
 
     strKey = "calories";
-    BOOST_CHECK(obj.pushKV(strKey, (int) 12));
+    obj.pushKV(strKey, int{12});
 
     strKey = "temperature";
-    BOOST_CHECK(obj.pushKV(strKey, (double) 90.012));
+    obj.pushKV(strKey, double{90.012});
 
     strKey = "moon";
-    BOOST_CHECK(obj.pushKV(strKey, true));
+    obj.pushKV(strKey, true);
 
     strKey = "spoon";
-    BOOST_CHECK(obj.pushKV(strKey, false));
+    obj.pushKV(strKey, false);
 
     UniValue obj2(UniValue::VOBJ);
-    BOOST_CHECK(obj2.pushKV("cat1", 9000));
-    BOOST_CHECK(obj2.pushKV("cat2", 12345));
+    obj2.pushKV("cat1", 9000);
+    obj2.pushKV("cat2", 12345);
 
-    BOOST_CHECK(obj.pushKVs(obj2));
+    obj.pushKVs(obj2);
 
     BOOST_CHECK_EQUAL(obj.empty(), false);
     BOOST_CHECK_EQUAL(obj.size(), 11);
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index 22c1f2bdc2..83e23497cb 100644
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -1633,7 +1633,7 @@ RPCHelpMan walletcreatefundedpsbt()
         }, true
     );
 
-    UniValue options = request.params[3];
+    UniValue options{request.params[3].isNull() ? UniValue::VOBJ : request.params[3]};
 
     CAmount fee;
     int change_position;