throw an exception rather than returning false when building invalid transactions

diff --git a/src/gtest/test_transaction_builder.cpp b/src/gtest/test_transaction_builder.cpp
index 40f53d05673efe8a9141b145659d015e6c47a315..0f658075e5c20a7fb087e4d12e62240b1b02d4c2 100644 (file)
--- a/src/gtest/test_transaction_builder.cpp
+++ b/src/gtest/test_transaction_builder.cpp
@@ -4,6 +4,7 @@
 #include "key_io.h"
 #include "main.h"
 #include "pubkey.h"
+#include "rpc/protocol.h"
 #include "transaction_builder.h"
 #include "zcash/Address.hpp"
 
@@ -67,9 +68,10 @@ TEST(TransactionBuilder, Invoke)
     // Create a Sapling-only transaction
     // 0.0004 z-ZEC in, 0.00025 z-ZEC out, 0.0001 t-ZEC fee, 0.00005 z-ZEC change
     auto builder2 = TransactionBuilder(consensusParams, 2);
-    ASSERT_TRUE(builder2.AddSaplingSpend(expsk, note, anchor, witness));
+    builder2.AddSaplingSpend(expsk, note, anchor, witness);
     // Check that trying to add a different anchor fails
-    ASSERT_FALSE(builder2.AddSaplingSpend(expsk, note, uint256(), witness));
+    // TODO: the following check can be split out in to another test
+    ASSERT_THROW(builder2.AddSaplingSpend(expsk, note, uint256(), witness), UniValue);
 
     builder2.AddSaplingOutput(fvk.ovk, pk, 25000, {});
     auto tx2 = builder2.Build().GetTxOrThrow();
@@ -106,7 +108,7 @@ TEST(TransactionBuilder, RejectsInvalidTransparentOutput)
     // Default CTxDestination type is an invalid address
     CTxDestination taddr;
     auto builder = TransactionBuilder(consensusParams, 1);
-    EXPECT_FALSE(builder.AddTransparentOutput(taddr, 50));
+    ASSERT_THROW(builder.AddTransparentOutput(taddr, 50), UniValue);
 }
 
 TEST(TransactionBuilder, RejectsInvalidTransparentChangeAddress)
@@ -117,7 +119,7 @@ TEST(TransactionBuilder, RejectsInvalidTransparentChangeAddress)
     // Default CTxDestination type is an invalid address
     CTxDestination taddr;
     auto builder = TransactionBuilder(consensusParams, 1);
-    EXPECT_FALSE(builder.SendChangeTo(taddr));
+    ASSERT_THROW(builder.SendChangeTo(taddr), UniValue);
 }
 
 TEST(TransactionBuilder, FailsWithNegativeChange)
@@ -158,12 +160,12 @@ TEST(TransactionBuilder, FailsWithNegativeChange)
     // Fail if there is only a transparent output
     // 0.0005 t-ZEC out, 0.0001 t-ZEC fee
     builder = TransactionBuilder(consensusParams, 1, &keystore);
-    EXPECT_TRUE(builder.AddTransparentOutput(taddr, 50000));
+    builder.AddTransparentOutput(taddr, 50000);
     EXPECT_EQ("Change cannot be negative", builder.Build().GetError());
 
     // Fails if there is insufficient input
     // 0.0005 t-ZEC out, 0.0001 t-ZEC fee, 0.00059999 z-ZEC in
-    EXPECT_TRUE(builder.AddSaplingSpend(expsk, note, anchor, witness));
+    builder.AddSaplingSpend(expsk, note, anchor, witness);
     EXPECT_EQ("Change cannot be negative", builder.Build().GetError());
 
     // Succeeds if there is sufficient input
@@ -219,7 +221,7 @@ TEST(TransactionBuilder, ChangeOutput)
     {
         auto builder = TransactionBuilder(consensusParams, 1, &keystore);
         builder.AddTransparentInput(COutPoint(), scriptPubKey, 25000);
-        ASSERT_TRUE(builder.AddSaplingSpend(expsk, note, anchor, witness));
+        builder.AddSaplingSpend(expsk, note, anchor, witness);
         auto tx = builder.Build().GetTxOrThrow();
 
         EXPECT_EQ(tx.vin.size(), 1);
@@ -249,7 +251,7 @@ TEST(TransactionBuilder, ChangeOutput)
     {
         auto builder = TransactionBuilder(consensusParams, 1, &keystore);
         builder.AddTransparentInput(COutPoint(), scriptPubKey, 25000);
-        ASSERT_TRUE(builder.SendChangeTo(taddr));
+        builder.SendChangeTo(taddr);
         auto tx = builder.Build().GetTxOrThrow();
 
         EXPECT_EQ(tx.vin.size(), 1);
@@ -290,7 +292,7 @@ TEST(TransactionBuilder, SetFee)
     // Default fee
     {
         auto builder = TransactionBuilder(consensusParams, 1);
-        ASSERT_TRUE(builder.AddSaplingSpend(expsk, note, anchor, witness));
+        builder.AddSaplingSpend(expsk, note, anchor, witness);
         builder.AddSaplingOutput(fvk.ovk, pk, 25000, {});
         auto tx = builder.Build().GetTxOrThrow();
 
@@ -305,7 +307,7 @@ TEST(TransactionBuilder, SetFee)
     // Configured fee
     {
         auto builder = TransactionBuilder(consensusParams, 1);
-        ASSERT_TRUE(builder.AddSaplingSpend(expsk, note, anchor, witness));
+        builder.AddSaplingSpend(expsk, note, anchor, witness);
         builder.AddSaplingOutput(fvk.ovk, pk, 25000, {});
         builder.SetFee(20000);
         auto tx = builder.Build().GetTxOrThrow();

diff --git a/src/transaction_builder.cpp b/src/transaction_builder.cpp
index 77989f8e4c9ec77a6df14181328e39703ec7bb26..2f4f84308bd60f1161f6861b422a3f02328f7db3 100644 (file)
--- a/src/transaction_builder.cpp
+++ b/src/transaction_builder.cpp
@@ -54,7 +54,7 @@ TransactionBuilder::TransactionBuilder(
     mtx = CreateNewContextualCMutableTransaction(consensusParams, nHeight);
 }
 
-bool TransactionBuilder::AddSaplingSpend(
+void TransactionBuilder::AddSaplingSpend(
     libzcash::SaplingExpandedSpendingKey expsk,
     libzcash::SaplingNote note,
     uint256 anchor,
@@ -66,15 +66,12 @@ bool TransactionBuilder::AddSaplingSpend(
     }
 
     // Consistency check: all anchors must equal the first one
-    if (!spends.empty()) {
-        if (spends[0].anchor != anchor) {
-            return false;
-        }
+    if (spends.size() > 0 && spends[0].anchor != anchor) {
+        throw JSONRPCError(RPC_WALLET_ERROR, "Anchor does not match previously-added Sapling spends.");
     }
 
     spends.emplace_back(expsk, note, anchor, witness);
     mtx.valueBalance += note.value();
-    return true;
 }
 
 void TransactionBuilder::AddSaplingOutput(
@@ -103,16 +100,15 @@ void TransactionBuilder::AddTransparentInput(COutPoint utxo, CScript scriptPubKe
     tIns.emplace_back(scriptPubKey, value);
 }
 
-bool TransactionBuilder::AddTransparentOutput(CTxDestination& to, CAmount value)
+void TransactionBuilder::AddTransparentOutput(CTxDestination& to, CAmount value)
 {
     if (!IsValidDestination(to)) {
-        return false;
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid output address, not a valid taddr.");
     }
 
     CScript scriptPubKey = GetScriptForDestination(to);
     CTxOut out(value, scriptPubKey);
     mtx.vout.push_back(out);
-    return true;
 }
 
 void TransactionBuilder::SetFee(CAmount fee)
@@ -126,16 +122,14 @@ void TransactionBuilder::SendChangeTo(libzcash::SaplingPaymentAddress changeAddr
     tChangeAddr = boost::none;
 }
 
-bool TransactionBuilder::SendChangeTo(CTxDestination& changeAddr)
+void TransactionBuilder::SendChangeTo(CTxDestination& changeAddr)
 {
     if (!IsValidDestination(changeAddr)) {
-        return false;
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid change address, not a valid taddr.");
     }
 
     tChangeAddr = changeAddr;
     zChangeAddr = boost::none;
-
-    return true;
 }
 
 TransactionBuilderResult TransactionBuilder::Build()
@@ -167,7 +161,7 @@ TransactionBuilderResult TransactionBuilder::Build()
             AddSaplingOutput(zChangeAddr->first, zChangeAddr->second, change);
         } else if (tChangeAddr) {
             // tChangeAddr has already been validated.
-            assert(AddTransparentOutput(tChangeAddr.value(), change));
+            AddTransparentOutput(tChangeAddr.value(), change);
         } else if (!spends.empty()) {
             auto fvk = spends[0].expsk.full_viewing_key();
             auto note = spends[0].note;

diff --git a/src/transaction_builder.h b/src/transaction_builder.h
index 5b65f6f871982b1fbbd12914ea93fd03855e9a66..592f4f64db2650647dbbc5db838521a5bef91f2e 100644 (file)
--- a/src/transaction_builder.h
+++ b/src/transaction_builder.h
@@ -88,9 +88,9 @@ public:
 
     void SetFee(CAmount fee);
 
-    // Returns false if the anchor does not match the anchor used by
+    // Throws if the anchor does not match the anchor used by
     // previously-added Sapling spends.
-    bool AddSaplingSpend(
+    void AddSaplingSpend(
         libzcash::SaplingExpandedSpendingKey expsk,
         libzcash::SaplingNote note,
         uint256 anchor,
@@ -105,11 +105,11 @@ public:
     // Assumes that the value correctly corresponds to the provided UTXO.
     void AddTransparentInput(COutPoint utxo, CScript scriptPubKey, CAmount value);
 
-    bool AddTransparentOutput(CTxDestination& to, CAmount value);
+    void AddTransparentOutput(CTxDestination& to, CAmount value);
 
     void SendChangeTo(libzcash::SaplingPaymentAddress changeAddr, uint256 ovk);
 
-    bool SendChangeTo(CTxDestination& changeAddr);
+    void SendChangeTo(CTxDestination& changeAddr);
 
     TransactionBuilderResult Build();
 };

diff --git a/src/wallet/asyncrpcoperation_mergetoaddress.cpp b/src/wallet/asyncrpcoperation_mergetoaddress.cpp
index 9129dbc1e89e6a90a37c6488fe7ac6d95325a286..ebadbef815535c852aa5a96e936db1c6a5448476 100644 (file)
--- a/src/wallet/asyncrpcoperation_mergetoaddress.cpp
+++ b/src/wallet/asyncrpcoperation_mergetoaddress.cpp
@@ -338,13 +338,11 @@ bool AsyncRPCOperation_mergetoaddress::main_impl()
             if (!witnesses[i]) {
                 throw JSONRPCError(RPC_WALLET_ERROR, "Missing witness for Sapling note");
             }
-            assert(builder_.AddSaplingSpend(expsks[i], saplingNotes[i], anchor, witnesses[i].get()));
+            builder_.AddSaplingSpend(expsks[i], saplingNotes[i], anchor, witnesses[i].get());
         }
 
         if (isToTaddr_) {
-            if (!builder_.AddTransparentOutput(toTaddr_, sendAmount)) {
-                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid output address, not a valid taddr.");
-            }
+            builder_.AddTransparentOutput(toTaddr_, sendAmount);
         } else {
             std::string zaddr = std::get<0>(recipient_);
             std::string memo = std::get<1>(recipient_);

diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp
index d71e5d91eaa6b2ec82fd2dd31de811e81daf3bf2..41e423a378f387fbff22000eb5fb80968b9f9f4d 100644 (file)
--- a/src/wallet/asyncrpcoperation_sendmany.cpp
+++ b/src/wallet/asyncrpcoperation_sendmany.cpp
@@ -416,7 +416,7 @@ bool AsyncRPCOperation_sendmany::main_impl() {
             }
 
             CTxDestination changeAddr = vchPubKey.GetID();
-            assert(builder_.SendChangeTo(changeAddr));
+            builder_.SendChangeTo(changeAddr);
         }
 
         // Select Sapling notes
@@ -445,7 +445,7 @@ bool AsyncRPCOperation_sendmany::main_impl() {
             if (!witnesses[i]) {
                 throw JSONRPCError(RPC_WALLET_ERROR, "Missing witness for Sapling note");
             }
-            assert(builder_.AddSaplingSpend(expsk, notes[i], anchor, witnesses[i].get()));
+            builder_.AddSaplingSpend(expsk, notes[i], anchor, witnesses[i].get());
         }
 
         // Add Sapling outputs
@@ -469,9 +469,7 @@ bool AsyncRPCOperation_sendmany::main_impl() {
             auto amount = std::get<1>(r);
 
             auto address = DecodeDestination(outputAddress);
-            if (!builder_.AddTransparentOutput(address, amount)) {
-                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid output address, not a valid taddr.");
-            }
+            builder_.AddTransparentOutput(address, amount);
         }
 
         // Build the transaction

diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp
index 78eae8a86a94843f0fd3fd6c0c62d1753d123608..4745b086022dea973dc327273b85c43e3275303a 100644 (file)
--- a/src/wallet/gtest/test_wallet.cpp
+++ b/src/wallet/gtest/test_wallet.cpp
@@ -383,7 +383,7 @@ TEST(WalletTests, SetSaplingNoteAddrsInCWalletTx) {
     uint256 nullifier = nf.get();
 
     auto builder = TransactionBuilder(consensusParams, 1);
-    ASSERT_TRUE(builder.AddSaplingSpend(expsk, note, anchor, witness));
+    builder.AddSaplingSpend(expsk, note, anchor, witness);
     builder.AddSaplingOutput(fvk.ovk, pk, 50000, {});
     builder.SetFee(0);
     auto tx = builder.Build().GetTxOrThrow();
@@ -501,7 +501,7 @@ TEST(WalletTests, FindMySaplingNotes) {
 
     // Generate transaction
     auto builder = TransactionBuilder(consensusParams, 1);
-    ASSERT_TRUE(builder.AddSaplingSpend(expsk, note, anchor, witness));
+    builder.AddSaplingSpend(expsk, note, anchor, witness);
     builder.AddSaplingOutput(fvk.ovk, pk, 25000, {});
     auto tx = builder.Build().GetTxOrThrow();
 
@@ -639,7 +639,7 @@ TEST(WalletTests, GetConflictedSaplingNotes) {
 
     // Generate tx to create output note B
     auto builder = TransactionBuilder(consensusParams, 1);
-    ASSERT_TRUE(builder.AddSaplingSpend(expsk, note, anchor, witness));
+    builder.AddSaplingSpend(expsk, note, anchor, witness);
     builder.AddSaplingOutput(fvk.ovk, pk, 35000, {});
     auto tx = builder.Build().GetTxOrThrow();
     CWalletTx wtx {&wallet, tx};
@@ -693,13 +693,13 @@ TEST(WalletTests, GetConflictedSaplingNotes) {
 
     // Create transaction to spend note B
     auto builder2 = TransactionBuilder(consensusParams, 2);
-    ASSERT_TRUE(builder2.AddSaplingSpend(expsk, note2, anchor, spend_note_witness));
+    builder2.AddSaplingSpend(expsk, note2, anchor, spend_note_witness);
     builder2.AddSaplingOutput(fvk.ovk, pk, 20000, {});
     auto tx2 = builder2.Build().GetTxOrThrow();
 
     // Create conflicting transaction which also spends note B
     auto builder3 = TransactionBuilder(consensusParams, 2);
-    ASSERT_TRUE(builder3.AddSaplingSpend(expsk, note2, anchor, spend_note_witness));
+    builder3.AddSaplingSpend(expsk, note2, anchor, spend_note_witness);
     builder3.AddSaplingOutput(fvk.ovk, pk, 19999, {});
     auto tx3 = builder3.Build().GetTxOrThrow();
 
@@ -798,7 +798,7 @@ TEST(WalletTests, SaplingNullifierIsSpent) {
 
     // Generate transaction
     auto builder = TransactionBuilder(consensusParams, 1);
-    ASSERT_TRUE(builder.AddSaplingSpend(expsk, note, anchor, witness));
+    builder.AddSaplingSpend(expsk, note, anchor, witness);
     builder.AddSaplingOutput(fvk.ovk, pk, 25000, {});
     auto tx = builder.Build().GetTxOrThrow();
 
@@ -893,7 +893,7 @@ TEST(WalletTests, NavigateFromSaplingNullifierToNote) {
 
     // Generate transaction
     auto builder = TransactionBuilder(consensusParams, 1);
-    ASSERT_TRUE(builder.AddSaplingSpend(expsk, note, anchor, witness));
+    builder.AddSaplingSpend(expsk, note, anchor, witness);
     builder.AddSaplingOutput(fvk.ovk, pk, 25000, {});
     auto tx = builder.Build().GetTxOrThrow();
 
@@ -1027,7 +1027,7 @@ TEST(WalletTests, SpentSaplingNoteIsFromMe) {
 
     // Generate transaction, which sends funds to note B
     auto builder = TransactionBuilder(consensusParams, 1);
-    ASSERT_TRUE(builder.AddSaplingSpend(expsk, note, anchor, witness));
+    builder.AddSaplingSpend(expsk, note, anchor, witness);
     builder.AddSaplingOutput(fvk.ovk, pk, 25000, {});
     auto tx = builder.Build().GetTxOrThrow();
 
@@ -1097,7 +1097,7 @@ TEST(WalletTests, SpentSaplingNoteIsFromMe) {
 
     // Create transaction to spend note B
     auto builder2 = TransactionBuilder(consensusParams, 2);
-    ASSERT_TRUE(builder2.AddSaplingSpend(expsk, note2, anchor, spend_note_witness));
+    builder2.AddSaplingSpend(expsk, note2, anchor, spend_note_witness);
     builder2.AddSaplingOutput(fvk.ovk, pk, 12500, {});
     auto tx2 = builder2.Build().GetTxOrThrow();
     EXPECT_EQ(tx2.vin.size(), 0);
@@ -1725,7 +1725,7 @@ TEST(WalletTests, UpdatedSaplingNoteData) {
 
     // Generate transaction
     auto builder = TransactionBuilder(consensusParams, 1);
-    ASSERT_TRUE(builder.AddSaplingSpend(expsk, note, anchor, witness));
+    builder.AddSaplingSpend(expsk, note, anchor, witness);
     builder.AddSaplingOutput(fvk.ovk, pk2, 25000, {});
     auto tx = builder.Build().GetTxOrThrow();
 
@@ -1928,7 +1928,7 @@ TEST(WalletTests, MarkAffectedSaplingTransactionsDirty) {
     // Create a Sapling-only transaction
     // 0.0004 z-ZEC in, 0.00025 z-ZEC out, 0.0001 t-ZEC fee, 0.00005 z-ZEC change
     auto builder2 = TransactionBuilder(consensusParams, 2);
-    ASSERT_TRUE(builder2.AddSaplingSpend(expsk, note, anchor, witness));
+    builder2.AddSaplingSpend(expsk, note, anchor, witness);
     builder2.AddSaplingOutput(fvk.ovk, pk, 25000, {});
     auto tx2 = builder2.Build().GetTxOrThrow();