Auto merge of #4218 - Eirik0:4158-fix-recursive-memusage, r=str4d

Include shielded transaction data in RecursiveDynamicUsage calculation

Fixes #4158

src/Makefile.gtest.include

@@ -20,6 +20,7 @@ endif
 zcash_gtest_SOURCES += \
 	gtest/test_tautology.cpp \
 	gtest/test_deprecation.cpp \
+	gtest/test_dynamicusage.cpp \
 	gtest/test_equihash.cpp \
 	gtest/test_httprpc.cpp \
 	gtest/test_joinsplit.cpp \

src/core_memusage.h

@@ -33,6 +33,9 @@ static inline size_t RecursiveDynamicUsage(const CTransaction& tx) {
     for (std::vector<CTxOut>::const_iterator it = tx.vout.begin(); it != tx.vout.end(); it++) {
         mem += RecursiveDynamicUsage(*it);
     }
+    mem += memusage::DynamicUsage(tx.vJoinSplit);
+    mem += memusage::DynamicUsage(tx.vShieldedSpend);
+    mem += memusage::DynamicUsage(tx.vShieldedOutput);
     return mem;
 }
 
@@ -44,6 +47,9 @@ static inline size_t RecursiveDynamicUsage(const CMutableTransaction& tx) {
     for (std::vector<CTxOut>::const_iterator it = tx.vout.begin(); it != tx.vout.end(); it++) {
         mem += RecursiveDynamicUsage(*it);
     }
+    mem += memusage::DynamicUsage(tx.vJoinSplit);
+    mem += memusage::DynamicUsage(tx.vShieldedSpend);
+    mem += memusage::DynamicUsage(tx.vShieldedOutput);
     return mem;
 }
 

src/gtest/test_dynamicusage.cpp

@@ -0,0 +1,111 @@
+// Copyright (c) 2019 The Zcash developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://www.opensource.org/licenses/mit-license.php 
+
+#include <gtest/gtest.h>
+#include <iostream>
+
+#include "coins.h"
+#include "init.h"
+#include "key.h"
+#include "transaction_builder.h"
+#include "utiltest.h"
+
+extern ZCJoinSplit* params;
+
+TEST(RecursiveDynamicUsageTests, TestTransactionTransparent)
+{
+    auto consensusParams = RegtestActivateSapling();
+
+    CBasicKeyStore keystore;
+    CKey tsk = AddTestCKeyToKeyStore(keystore);
+    auto scriptPubKey = GetScriptForDestination(tsk.GetPubKey().GetID());
+    CTxDestination taddr = tsk.GetPubKey().GetID();
+    
+    auto builder = TransactionBuilder(consensusParams, 1, &keystore);
+    builder.AddTransparentInput(COutPoint(), scriptPubKey, 50000);
+    builder.AddTransparentOutput(taddr, 40000);
+
+    auto tx = builder.Build().GetTxOrThrow();
+    // 1 vin + 1 vout
+    // (96 + 128) + 64
+    EXPECT_EQ(288, RecursiveDynamicUsage(tx));
+
+    RegtestDeactivateSapling();
+}
+
+TEST(RecursiveDynamicUsageTests, TestTransactionJoinSplit)
+{
+    auto consensusParams = RegtestActivateSapling();
+
+    auto sproutSk = libzcash::SproutSpendingKey::random();
+
+    auto wtx = GetValidSproutReceive(*params, sproutSk, 25000, true);
+    // 2 vin + 1 vJoinSplit + 1 vShieldedOutput
+    // 160 + 1856 + 976
+    EXPECT_EQ(2992, RecursiveDynamicUsage(wtx));
+
+    RegtestDeactivateSapling();
+}
+
+TEST(RecursiveDynamicUsageTests, TestTransactionSaplingToSapling)
+{
+    auto consensusParams = RegtestActivateSapling();
+    
+    auto sk = libzcash::SaplingSpendingKey::random();
+    auto testNote = GetTestSaplingNote(sk.default_address(), 50000);
+
+    auto builder = TransactionBuilder(consensusParams, 1);
+    builder.AddSaplingSpend(sk.expanded_spending_key(), testNote.note, testNote.tree.root(), testNote.tree.witness());
+    builder.AddSaplingOutput(sk.full_viewing_key().ovk, sk.default_address(), 5000, {});
+    
+    auto tx = builder.Build().GetTxOrThrow();
+    // 1 vShieldedSpend + 2 vShieldedOutput
+    // 400 + 1920
+    EXPECT_EQ(2320, RecursiveDynamicUsage(tx));
+
+    RegtestDeactivateSapling();
+}
+
+TEST(RecursiveDynamicUsageTests, TestTransactionTransparentToSapling)
+{
+    auto consensusParams = RegtestActivateSapling();
+    
+    CBasicKeyStore keystore;
+    CKey tsk = AddTestCKeyToKeyStore(keystore);
+    auto scriptPubKey = GetScriptForDestination(tsk.GetPubKey().GetID());
+    auto sk = libzcash::SaplingSpendingKey::random();
+
+    auto builder = TransactionBuilder(consensusParams, 1, &keystore);
+    builder.AddTransparentInput(COutPoint(), scriptPubKey, 50000);
+    builder.AddSaplingOutput(sk.full_viewing_key().ovk, sk.default_address(), 40000, {});
+    
+    auto tx = builder.Build().GetTxOrThrow();
+    // 1 vin + 1 vShieldedOutput
+    // (96 + 128) + 976
+    EXPECT_EQ(1200, RecursiveDynamicUsage(tx));
+
+    RegtestDeactivateSapling();
+}
+
+TEST(RecursiveDynamicUsageTests, TestTransactionSaplingToTransparent)
+{
+    auto consensusParams = RegtestActivateSapling();
+    
+    CBasicKeyStore keystore;
+    CKey tsk = AddTestCKeyToKeyStore(keystore);
+    CTxDestination taddr = tsk.GetPubKey().GetID();
+    auto sk = libzcash::SaplingSpendingKey::random();
+    auto testNote = GetTestSaplingNote(sk.default_address(), 50000);
+
+    auto builder = TransactionBuilder(consensusParams, 1, &keystore);
+    builder.AddSaplingSpend(sk.expanded_spending_key(), testNote.note, testNote.tree.root(), testNote.tree.witness());
+    builder.AddTransparentOutput(taddr, 40000);
+
+    auto tx = builder.Build().GetTxOrThrow();
+    // 1 vShieldedSpend + 1 vout
+    // 400 + 64
+    EXPECT_EQ(464, RecursiveDynamicUsage(tx));
+
+    RegtestDeactivateSapling();
+}

src/gtest/test_mempoollimit.cpp

@@ -156,8 +156,8 @@ TEST(MempoolLimitTests, WeightedTxInfoFromTx)
             std::cerr << result.GetError() << std::endl;
         }
         WeightedTxInfo info = WeightedTxInfo::from(result.GetTxOrThrow(), 10000);
-        EXPECT_EQ(5124, info.txWeight.cost);
+        EXPECT_EQ(5168, info.txWeight.cost);
-        EXPECT_EQ(5124, info.txWeight.evictionWeight);
+        EXPECT_EQ(5168, info.txWeight.evictionWeight);
     }
     
     RegtestDeactivateSapling();

src/mempool_limit.cpp

@@ -148,9 +148,6 @@ TxWeight TxWeight::negate() const
 WeightedTxInfo WeightedTxInfo::from(const CTransaction& tx, const CAmount& fee)
 {
     size_t memUsage = RecursiveDynamicUsage(tx);
-    memUsage += tx.vJoinSplit.size() * JOINSPLIT_SIZE;
-    memUsage += tx.vShieldedOutput.size() * OUTPUTDESCRIPTION_SIZE;
-    memUsage += tx.vShieldedSpend.size() * SPENDDESCRIPTION_SIZE;
     int64_t cost = std::max((int64_t) memUsage, (int64_t) MIN_TX_COST);
     int64_t evictionWeight = cost;
     if (fee < DEFAULT_FEE) {

src/primitives/transaction.h

@@ -23,10 +23,6 @@
 #include "zcash/JoinSplit.hpp"
 #include "zcash/Proof.hpp"
 
-#define JOINSPLIT_SIZE GetSerializeSize(JSDescription(), SER_NETWORK, PROTOCOL_VERSION)
-#define OUTPUTDESCRIPTION_SIZE GetSerializeSize(OutputDescription(), SER_NETWORK, PROTOCOL_VERSION)
-#define SPENDDESCRIPTION_SIZE GetSerializeSize(SpendDescription(), SER_NETWORK, PROTOCOL_VERSION)
-
 // Overwinter transaction version
 static const int32_t OVERWINTER_TX_VERSION = 3;
 static_assert(OVERWINTER_TX_VERSION >= OVERWINTER_MIN_TX_VERSION,
@@ -41,6 +37,14 @@ static_assert(SAPLING_TX_VERSION >= SAPLING_MIN_TX_VERSION,
 static_assert(SAPLING_TX_VERSION <= SAPLING_MAX_TX_VERSION,
     "Sapling tx version must not be higher than maximum");
 
+// These constants are defined in the protocol § 7.1:
+// https://zips.z.cash/protocol/protocol.pdf#txnencoding
+#define OUTPUTDESCRIPTION_SIZE 948
+#define SPENDDESCRIPTION_SIZE 384
+static inline size_t JOINSPLIT_SIZE(int transactionVersion) {
+    return transactionVersion >= SAPLING_TX_VERSION ? 1698 : 1802;
+}
+
 /**
  * A shielded input to a transaction. It contains data that describes a Spend transfer.
  */

src/wallet/rpcwallet.cpp

@@ -4462,7 +4462,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
     unsigned int max_tx_size = saplingActive ? MAX_TX_SIZE_AFTER_SAPLING : MAX_TX_SIZE_BEFORE_SAPLING;
     size_t estimatedTxSize = 200;  // tx overhead + wiggle room
     if (isToSproutZaddr) {
-        estimatedTxSize += JOINSPLIT_SIZE;
+        estimatedTxSize += JOINSPLIT_SIZE(SAPLING_TX_VERSION); // We assume that sapling has activated
     } else if (isToSaplingZaddr) {
         estimatedTxSize += OUTPUTDESCRIPTION_SIZE;
     }
@@ -4550,7 +4550,8 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
             if (!maxedOutNotesFlag) {
                 // If we haven't added any notes yet and the merge is to a
                 // z-address, we have already accounted for the first JoinSplit.
-                size_t increase = (sproutNoteInputs.empty() && !isToSproutZaddr) || (sproutNoteInputs.size() % 2 == 0) ? JOINSPLIT_SIZE : 0;
+                size_t increase = (sproutNoteInputs.empty() && !isToSproutZaddr) || (sproutNoteInputs.size() % 2 == 0) ?
+                    JOINSPLIT_SIZE(SAPLING_TX_VERSION) : 0;
                 if (estimatedTxSize + increase >= max_tx_size ||
                     (sproutNoteLimit > 0 && noteCounter > sproutNoteLimit))
                 {

src/zcash/JoinSplit.hpp

@@ -21,6 +21,9 @@ static constexpr size_t GROTH_PROOF_SIZE = (
     48); // π_C
 
 typedef std::array<unsigned char, GROTH_PROOF_SIZE> GrothProof;
+// TODO: Because PHGRProof is listed first, using the default
+// constructor for JSDescription() will create a JSDescription
+// with a PHGRProof. The default however should be GrothProof.
 typedef boost::variant<PHGRProof, GrothProof> SproutProof;
 
 class JSInput {