From 15ec5525e316d41dfe110263062a97b7530c6be0 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 26 Apr 2018 14:18:36 +0100 Subject: [PATCH 1/5] Raise 100kB transaction size limit from Sapling activation Closes #2864. --- src/consensus/consensus.h | 3 ++- src/gtest/test_checktransaction.cpp | 41 ++++++++++++++++++++++++++--- src/main.cpp | 11 +++++++- src/wallet/rpcwallet.cpp | 34 +++++++++++++++++------- src/wallet/wallet.cpp | 7 ++++- 5 files changed, 81 insertions(+), 15 deletions(-) diff --git a/src/consensus/consensus.h b/src/consensus/consensus.h index 1efaf99ea..112fa626a 100644 --- a/src/consensus/consensus.h +++ b/src/consensus/consensus.h @@ -23,7 +23,8 @@ static const unsigned int MAX_BLOCK_SIZE = 2000000; /** The maximum allowed number of signature check operations in a block (network rule) */ static const unsigned int MAX_BLOCK_SIGOPS = 20000; /** The maximum size of a transaction (network rule) */ -static const unsigned int MAX_TX_SIZE = 100000; +static const unsigned int MAX_TX_SIZE_BEFORE_SAPLING = 100000; +static const unsigned int MAX_TX_SIZE = MAX_BLOCK_SIZE; /** Coinbase transaction outputs can only be spent after this number of new blocks (network rule) */ static const int COINBASE_MATURITY = 100; /** The minimum value which is invalid for expiry height, used by CTransaction and CMutableTransaction */ diff --git a/src/gtest/test_checktransaction.cpp b/src/gtest/test_checktransaction.cpp index d7527dbac..bc60209cb 100644 --- a/src/gtest/test_checktransaction.cpp +++ b/src/gtest/test_checktransaction.cpp @@ -43,6 +43,7 @@ public: MOCK_CONST_METHOD0(GetRejectReason, std::string()); }; +void CreateJoinSplitSignature(CMutableTransaction& mtx, uint32_t consensusBranchId); CMutableTransaction GetValidTransaction() { uint32_t consensusBranchId = SPROUT_BRANCH_ID; @@ -63,7 +64,11 @@ CMutableTransaction GetValidTransaction() { mtx.vjoinsplit[1].nullifiers.at(0) = uint256S("0000000000000000000000000000000000000000000000000000000000000002"); mtx.vjoinsplit[1].nullifiers.at(1) = uint256S("0000000000000000000000000000000000000000000000000000000000000003"); + CreateJoinSplitSignature(mtx, consensusBranchId); + return mtx; +} +void CreateJoinSplitSignature(CMutableTransaction& mtx, uint32_t consensusBranchId) { // Generate an ephemeral keypair. uint256 joinSplitPubKey; unsigned char joinSplitPrivKey[crypto_sign_SECRETKEYBYTES]; @@ -86,7 +91,6 @@ CMutableTransaction GetValidTransaction() { dataToBeSigned.begin(), 32, joinSplitPrivKey ) == 0); - return mtx; } TEST(checktransaction_tests, valid_transaction) { @@ -129,7 +133,8 @@ TEST(checktransaction_tests, bad_txns_vout_empty) { CheckTransactionWithoutProofVerification(tx, state); } -TEST(checktransaction_tests, bad_txns_oversize) { +TEST(checktransaction_tests, BadTxnsOversize) { + SelectParams(CBaseChainParams::REGTEST); CMutableTransaction mtx = GetValidTransaction(); mtx.vin[0].scriptSig = CScript(); @@ -153,9 +158,39 @@ TEST(checktransaction_tests, bad_txns_oversize) { CTransaction tx(mtx); ASSERT_EQ(::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION), 100202); + // Passes non-contextual checks... MockCValidationState state; + EXPECT_TRUE(CheckTransactionWithoutProofVerification(tx, state)); + + // ... but fails contextual ones! EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-oversize", false)).Times(1); - CheckTransactionWithoutProofVerification(tx, state); + EXPECT_FALSE(ContextualCheckTransaction(tx, state, 1, 100)); + } + + { + // But should be fine again once Sapling activates! + UpdateNetworkUpgradeParameters(Consensus::UPGRADE_OVERWINTER, Consensus::NetworkUpgrade::ALWAYS_ACTIVE); + UpdateNetworkUpgradeParameters(Consensus::UPGRADE_SAPLING, Consensus::NetworkUpgrade::ALWAYS_ACTIVE); + + mtx.fOverwintered = true; + mtx.nVersionGroupId = SAPLING_VERSION_GROUP_ID; + mtx.nVersion = SAPLING_TX_VERSION; + + // Change the proof types (which requires re-signing the JoinSplit data) + mtx.vjoinsplit[0].proof = libzcash::GrothProof(); + mtx.vjoinsplit[1].proof = libzcash::GrothProof(); + CreateJoinSplitSignature(mtx, NetworkUpgradeInfo[Consensus::UPGRADE_SAPLING].nBranchId); + + CTransaction tx(mtx); + EXPECT_EQ(::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION), 103713); + + MockCValidationState state; + EXPECT_TRUE(CheckTransactionWithoutProofVerification(tx, state)); + EXPECT_TRUE(ContextualCheckTransaction(tx, state, 1, 100)); + + // Revert to default + UpdateNetworkUpgradeParameters(Consensus::UPGRADE_SAPLING, Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT); + UpdateNetworkUpgradeParameters(Consensus::UPGRADE_OVERWINTER, Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT); } } diff --git a/src/main.cpp b/src/main.cpp index 27d045135..2824f015b 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -943,6 +943,15 @@ bool ContextualCheckTransaction(const CTransaction& tx, CValidationState &state, } } + // Rules that apply before Sapling: + if (!saplingActive) { + // Size limits + BOOST_STATIC_ASSERT(MAX_BLOCK_SIZE > MAX_TX_SIZE_BEFORE_SAPLING); // sanity + if (::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION) > MAX_TX_SIZE_BEFORE_SAPLING) + return state.DoS(100, error("ContextualCheckTransaction(): size limits failed"), + REJECT_INVALID, "bad-txns-oversize"); + } + if (!(tx.IsCoinBase() || tx.vjoinsplit.empty())) { auto consensusBranchId = CurrentEpochBranchId(nHeight, Params().GetConsensus()); // Empty output script. @@ -1051,7 +1060,7 @@ bool CheckTransactionWithoutProofVerification(const CTransaction& tx, CValidatio REJECT_INVALID, "bad-txns-vout-empty"); // Size limits - BOOST_STATIC_ASSERT(MAX_BLOCK_SIZE > MAX_TX_SIZE); // sanity + BOOST_STATIC_ASSERT(MAX_BLOCK_SIZE >= MAX_TX_SIZE); // sanity if (::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION) > MAX_TX_SIZE) return state.DoS(100, error("CheckTransaction(): size limits failed"), REJECT_INVALID, "bad-txns-oversize"); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 23bfea02c..7c3d840b6 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3489,6 +3489,7 @@ UniValue z_getoperationstatus_IMPL(const UniValue& params, bool fRemoveFinishedO // If input notes are small, we might actually require more than one joinsplit per zaddr output. // For now though, we assume we use one joinsplit per zaddr output (and the second output note is change). // We reduce the result by 1 to ensure there is room for non-joinsplit CTransaction data. +#define Z_SENDMANY_MAX_ZADDR_OUTPUTS_BEFORE_SAPLING ((MAX_TX_SIZE_BEFORE_SAPLING / GetSerializeSize(JSDescription(), SER_NETWORK, PROTOCOL_VERSION)) - 1) #define Z_SENDMANY_MAX_ZADDR_OUTPUTS ((MAX_TX_SIZE / GetSerializeSize(JSDescription(), SER_NETWORK, PROTOCOL_VERSION)) - 1) // transaction.h comment: spending taddr output requires CTxIn >= 148 bytes and typical taddr txout is 34 bytes @@ -3620,8 +3621,16 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) nTotalOut += nAmount; } + int nextBlockHeight = chainActive.Height() + 1; + size_t max_zaddr_outputs = Z_SENDMANY_MAX_ZADDR_OUTPUTS; + unsigned int max_tx_size = MAX_TX_SIZE; + if (!NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) { + max_zaddr_outputs = Z_SENDMANY_MAX_ZADDR_OUTPUTS_BEFORE_SAPLING; + max_tx_size = MAX_TX_SIZE_BEFORE_SAPLING; + } + // Check the number of zaddr outputs does not exceed the limit. - if (zaddrRecipients.size() > Z_SENDMANY_MAX_ZADDR_OUTPUTS) { + if (zaddrRecipients.size() > max_zaddr_outputs) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, too many zaddr outputs"); } @@ -3640,8 +3649,8 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) txsize += CTXOUT_REGULAR_SIZE; // There will probably be taddr change } txsize += CTXOUT_REGULAR_SIZE * taddrRecipients.size(); - if (txsize > MAX_TX_SIZE) { - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Too many outputs, size of raw transaction would be larger than limit of %d bytes", MAX_TX_SIZE )); + if (txsize > max_tx_size) { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Too many outputs, size of raw transaction would be larger than limit of %d bytes", max_tx_size )); } // Minimum confirmations @@ -3677,7 +3686,6 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) UniValue contextInfo = o; // Contextual transaction we will build on - int nextBlockHeight = chainActive.Height() + 1; CMutableTransaction contextualTx = CreateNewContextualCMutableTransaction(Params().GetConsensus(), nextBlockHeight); bool isShielded = !fromTaddr || zaddrRecipients.size() > 0; if (contextualTx.nVersion == 1 && isShielded) { @@ -3725,7 +3733,7 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) "\nby the caller. If the limit parameter is set to zero, and Overwinter is not yet active, the -mempooltxinputlimit" "\noption will determine the number of uxtos. Any limit is constrained by the consensus rule defining a maximum" "\ntransaction size of " - + strprintf("%d bytes.", MAX_TX_SIZE) + + strprintf("%d bytes before Sapling, and %d bytes once Sapling activates.", MAX_TX_SIZE_BEFORE_SAPLING, MAX_TX_SIZE) + HelpRequiringPassphrase() + "\n" "\nArguments:\n" "1. \"fromaddress\" (string, required) The address is a taddr or \"*\" for all taddrs belonging to the wallet.\n" @@ -3789,6 +3797,10 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) int nextBlockHeight = chainActive.Height() + 1; bool overwinterActive = NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER); + unsigned int max_tx_size = MAX_TX_SIZE; + if (!NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) { + max_tx_size = MAX_TX_SIZE_BEFORE_SAPLING; + } // Prepare to get coinbase utxos std::vector inputs; @@ -3833,7 +3845,7 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) if (!maxedOutFlag) { size_t increase = (boost::get(&address) != nullptr) ? CTXIN_SPEND_P2SH_SIZE : CTXIN_SPEND_DUST_SIZE; - if (estimatedTxSize + increase >= MAX_TX_SIZE || + if (estimatedTxSize + increase >= max_tx_size || (mempoolLimit > 0 && utxoCounter > mempoolLimit)) { maxedOutFlag = true; @@ -3928,7 +3940,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) "\n\nThe number of UTXOs and notes selected for merging can be limited by the caller. If the transparent limit" "\nparameter is set to zero, and Overwinter is not yet active, the -mempooltxinputlimit option will determine the" "\nnumber of UTXOs. Any limit is constrained by the consensus rule defining a maximum transaction size of " - + strprintf("%d bytes.", MAX_TX_SIZE) + + strprintf("%d bytes before Sapling, and %d bytes once Sapling activates.", MAX_TX_SIZE_BEFORE_SAPLING, MAX_TX_SIZE) + HelpRequiringPassphrase() + "\n" "\nArguments:\n" "1. fromaddresses (string, required) A JSON array with addresses.\n" @@ -4081,6 +4093,10 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) int nextBlockHeight = chainActive.Height() + 1; bool overwinterActive = NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER); + unsigned int max_tx_size = MAX_TX_SIZE; + if (!NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) { + max_tx_size = MAX_TX_SIZE_BEFORE_SAPLING; + } // Prepare to get UTXOs and notes std::vector utxoInputs; @@ -4125,7 +4141,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) if (!maxedOutUTXOsFlag) { size_t increase = (boost::get(&address) != nullptr) ? CTXIN_SPEND_P2SH_SIZE : CTXIN_SPEND_DUST_SIZE; - if (estimatedTxSize + increase >= MAX_TX_SIZE || + if (estimatedTxSize + increase >= max_tx_size || (mempoolLimit > 0 && utxoCounter > mempoolLimit)) { maxedOutUTXOsFlag = true; @@ -4157,7 +4173,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) // 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 = (noteInputs.empty() && !isToZaddr) || (noteInputs.size() % 2 == 0) ? JOINSPLIT_SIZE : 0; - if (estimatedTxSize + increase >= MAX_TX_SIZE || + if (estimatedTxSize + increase >= max_tx_size || (nNoteLimit > 0 && noteCounter > nNoteLimit)) { maxedOutNotesFlag = true; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3db3e26c3..0f2fd649d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2589,6 +2589,11 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt txNew.nExpiryHeight = nextBlockHeight + expiryDelta; } } + + unsigned int max_tx_size = MAX_TX_SIZE; + if (!NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) { + max_tx_size = MAX_TX_SIZE_BEFORE_SAPLING; + } // Discourage fee sniping. // @@ -2828,7 +2833,7 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt *static_cast(&wtxNew) = CTransaction(txNew); // Limit size - if (nBytes >= MAX_TX_SIZE) + if (nBytes >= max_tx_size) { strFailReason = _("Transaction too large"); return false; From ddcee7e13ad169ddea3f83e1412ed3d314744143 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 26 Apr 2018 15:05:10 +0100 Subject: [PATCH 2/5] Benchmark the largest valid Sapling transaction in validatelargetx 11130 inputs results in a transaction between 1992301 and 2003431 bytes. --- src/wallet/rpcwallet.cpp | 2 +- src/zcbenchmarks.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 7c3d840b6..634f27854 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2723,7 +2723,7 @@ UniValue zc_benchmark(const UniValue& params, bool fHelp) sample_times.push_back(benchmark_verify_equihash()); } else if (benchmarktype == "validatelargetx") { // Number of inputs in the spending transaction that we will simulate - int nInputs = 555; + int nInputs = 11130; if (params.size() >= 3) { nInputs = params[2].get_int(); } diff --git a/src/zcbenchmarks.cpp b/src/zcbenchmarks.cpp index 1d4ad78c8..53d433c86 100644 --- a/src/zcbenchmarks.cpp +++ b/src/zcbenchmarks.cpp @@ -243,8 +243,8 @@ double benchmark_large_tx(size_t nInputs) CMutableTransaction spending_tx; spending_tx.fOverwintered = true; - spending_tx.nVersion = OVERWINTER_TX_VERSION; - spending_tx.nVersionGroupId = OVERWINTER_VERSION_GROUP_ID; + spending_tx.nVersionGroupId = SAPLING_VERSION_GROUP_ID; + spending_tx.nVersion = SAPLING_TX_VERSION; auto input_hash = orig_tx.GetHash(); // Add nInputs inputs @@ -253,7 +253,7 @@ double benchmark_large_tx(size_t nInputs) } // Sign for all the inputs - auto consensusBranchId = NetworkUpgradeInfo[Consensus::UPGRADE_OVERWINTER].nBranchId; + auto consensusBranchId = NetworkUpgradeInfo[Consensus::UPGRADE_SAPLING].nBranchId; for (size_t i = 0; i < nInputs; i++) { SignSignature(tempKeystore, prevPubKey, spending_tx, i, 1000000, SIGHASH_ALL, consensusBranchId); } From 25fee3509a67a894f9a01908cc09d3df0cce7856 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 30 Apr 2018 15:13:32 +0100 Subject: [PATCH 3/5] Rename MAX_TX_SIZE to MAX_TX_SIZE_AFTER_SAPLING --- src/consensus/consensus.h | 2 +- src/main.cpp | 5 +++-- src/wallet/rpcwallet.cpp | 12 ++++++------ src/wallet/wallet.cpp | 2 +- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/consensus/consensus.h b/src/consensus/consensus.h index 112fa626a..c0a3138aa 100644 --- a/src/consensus/consensus.h +++ b/src/consensus/consensus.h @@ -24,7 +24,7 @@ static const unsigned int MAX_BLOCK_SIZE = 2000000; static const unsigned int MAX_BLOCK_SIGOPS = 20000; /** The maximum size of a transaction (network rule) */ static const unsigned int MAX_TX_SIZE_BEFORE_SAPLING = 100000; -static const unsigned int MAX_TX_SIZE = MAX_BLOCK_SIZE; +static const unsigned int MAX_TX_SIZE_AFTER_SAPLING = MAX_BLOCK_SIZE; /** Coinbase transaction outputs can only be spent after this number of new blocks (network rule) */ static const int COINBASE_MATURITY = 100; /** The minimum value which is invalid for expiry height, used by CTransaction and CMutableTransaction */ diff --git a/src/main.cpp b/src/main.cpp index 2824f015b..dd27a6014 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1060,8 +1060,9 @@ bool CheckTransactionWithoutProofVerification(const CTransaction& tx, CValidatio REJECT_INVALID, "bad-txns-vout-empty"); // Size limits - BOOST_STATIC_ASSERT(MAX_BLOCK_SIZE >= MAX_TX_SIZE); // sanity - if (::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION) > MAX_TX_SIZE) + BOOST_STATIC_ASSERT(MAX_BLOCK_SIZE >= MAX_TX_SIZE_AFTER_SAPLING); // sanity + BOOST_STATIC_ASSERT(MAX_TX_SIZE_AFTER_SAPLING > MAX_TX_SIZE_BEFORE_SAPLING); // sanity + if (::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION) > MAX_TX_SIZE_AFTER_SAPLING) return state.DoS(100, error("CheckTransaction(): size limits failed"), REJECT_INVALID, "bad-txns-oversize"); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 634f27854..801b2d574 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3490,7 +3490,7 @@ UniValue z_getoperationstatus_IMPL(const UniValue& params, bool fRemoveFinishedO // For now though, we assume we use one joinsplit per zaddr output (and the second output note is change). // We reduce the result by 1 to ensure there is room for non-joinsplit CTransaction data. #define Z_SENDMANY_MAX_ZADDR_OUTPUTS_BEFORE_SAPLING ((MAX_TX_SIZE_BEFORE_SAPLING / GetSerializeSize(JSDescription(), SER_NETWORK, PROTOCOL_VERSION)) - 1) -#define Z_SENDMANY_MAX_ZADDR_OUTPUTS ((MAX_TX_SIZE / GetSerializeSize(JSDescription(), SER_NETWORK, PROTOCOL_VERSION)) - 1) +#define Z_SENDMANY_MAX_ZADDR_OUTPUTS ((MAX_TX_SIZE_AFTER_SAPLING / GetSerializeSize(JSDescription(), SER_NETWORK, PROTOCOL_VERSION)) - 1) // transaction.h comment: spending taddr output requires CTxIn >= 148 bytes and typical taddr txout is 34 bytes #define CTXIN_SPEND_DUST_SIZE 148 @@ -3623,7 +3623,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) int nextBlockHeight = chainActive.Height() + 1; size_t max_zaddr_outputs = Z_SENDMANY_MAX_ZADDR_OUTPUTS; - unsigned int max_tx_size = MAX_TX_SIZE; + unsigned int max_tx_size = MAX_TX_SIZE_AFTER_SAPLING; if (!NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) { max_zaddr_outputs = Z_SENDMANY_MAX_ZADDR_OUTPUTS_BEFORE_SAPLING; max_tx_size = MAX_TX_SIZE_BEFORE_SAPLING; @@ -3733,7 +3733,7 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) "\nby the caller. If the limit parameter is set to zero, and Overwinter is not yet active, the -mempooltxinputlimit" "\noption will determine the number of uxtos. Any limit is constrained by the consensus rule defining a maximum" "\ntransaction size of " - + strprintf("%d bytes before Sapling, and %d bytes once Sapling activates.", MAX_TX_SIZE_BEFORE_SAPLING, MAX_TX_SIZE) + + strprintf("%d bytes before Sapling, and %d bytes once Sapling activates.", MAX_TX_SIZE_BEFORE_SAPLING, MAX_TX_SIZE_AFTER_SAPLING) + HelpRequiringPassphrase() + "\n" "\nArguments:\n" "1. \"fromaddress\" (string, required) The address is a taddr or \"*\" for all taddrs belonging to the wallet.\n" @@ -3797,7 +3797,7 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) int nextBlockHeight = chainActive.Height() + 1; bool overwinterActive = NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER); - unsigned int max_tx_size = MAX_TX_SIZE; + unsigned int max_tx_size = MAX_TX_SIZE_AFTER_SAPLING; if (!NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) { max_tx_size = MAX_TX_SIZE_BEFORE_SAPLING; } @@ -3940,7 +3940,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) "\n\nThe number of UTXOs and notes selected for merging can be limited by the caller. If the transparent limit" "\nparameter is set to zero, and Overwinter is not yet active, the -mempooltxinputlimit option will determine the" "\nnumber of UTXOs. Any limit is constrained by the consensus rule defining a maximum transaction size of " - + strprintf("%d bytes before Sapling, and %d bytes once Sapling activates.", MAX_TX_SIZE_BEFORE_SAPLING, MAX_TX_SIZE) + + strprintf("%d bytes before Sapling, and %d bytes once Sapling activates.", MAX_TX_SIZE_BEFORE_SAPLING, MAX_TX_SIZE_AFTER_SAPLING) + HelpRequiringPassphrase() + "\n" "\nArguments:\n" "1. fromaddresses (string, required) A JSON array with addresses.\n" @@ -4093,7 +4093,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) int nextBlockHeight = chainActive.Height() + 1; bool overwinterActive = NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER); - unsigned int max_tx_size = MAX_TX_SIZE; + unsigned int max_tx_size = MAX_TX_SIZE_AFTER_SAPLING; if (!NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) { max_tx_size = MAX_TX_SIZE_BEFORE_SAPLING; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0f2fd649d..5ab1103ff 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2590,7 +2590,7 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt } } - unsigned int max_tx_size = MAX_TX_SIZE; + unsigned int max_tx_size = MAX_TX_SIZE_AFTER_SAPLING; if (!NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) { max_tx_size = MAX_TX_SIZE_BEFORE_SAPLING; } From 892ae945f71b4887e758c6bd6c397606261583cd Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 1 May 2018 11:23:11 +0100 Subject: [PATCH 4/5] Rework z_sendmany z-address recipient limit From Sapling, the z-address recipients could require either JSDescriptions or OutputDescriptions. Instead of trying to give an exact number in the help text, rely on transaction size estimation to guide user behaviour. --- src/wallet/rpcwallet.cpp | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 801b2d574..9023b80f6 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3485,12 +3485,13 @@ UniValue z_getoperationstatus_IMPL(const UniValue& params, bool fRemoveFinishedO } +// JSDescription size depends on the transaction version +#define V3_JS_DESCRIPTION_SIZE (GetSerializeSize(JSDescription(), SER_NETWORK, (OVERWINTER_TX_VERSION | (1 << 31)))) // Here we define the maximum number of zaddr outputs that can be included in a transaction. // If input notes are small, we might actually require more than one joinsplit per zaddr output. // For now though, we assume we use one joinsplit per zaddr output (and the second output note is change). // We reduce the result by 1 to ensure there is room for non-joinsplit CTransaction data. -#define Z_SENDMANY_MAX_ZADDR_OUTPUTS_BEFORE_SAPLING ((MAX_TX_SIZE_BEFORE_SAPLING / GetSerializeSize(JSDescription(), SER_NETWORK, PROTOCOL_VERSION)) - 1) -#define Z_SENDMANY_MAX_ZADDR_OUTPUTS ((MAX_TX_SIZE_AFTER_SAPLING / GetSerializeSize(JSDescription(), SER_NETWORK, PROTOCOL_VERSION)) - 1) +#define Z_SENDMANY_MAX_ZADDR_OUTPUTS_BEFORE_SAPLING ((MAX_TX_SIZE_BEFORE_SAPLING / V3_JS_DESCRIPTION_SIZE) - 1) // transaction.h comment: spending taddr output requires CTxIn >= 148 bytes and typical taddr txout is 34 bytes #define CTXIN_SPEND_DUST_SIZE 148 @@ -3507,7 +3508,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) "\nSend multiple times. Amounts are double-precision floating point numbers." "\nChange from a taddr flows to a new taddr address, while change from zaddr returns to itself." "\nWhen sending coinbase UTXOs to a zaddr, change is not allowed. The entire value of the UTXO(s) must be consumed." - + strprintf("\nCurrently, the maximum number of zaddr outputs is %d due to transaction size limits.\n", Z_SENDMANY_MAX_ZADDR_OUTPUTS) + + strprintf("\nBefore Sapling activates, the maximum number of zaddr outputs is %d due to transaction size limits.\n", Z_SENDMANY_MAX_ZADDR_OUTPUTS_BEFORE_SAPLING) + HelpRequiringPassphrase() + "\n" "\nArguments:\n" "1. \"fromaddress\" (string, required) The taddr or zaddr to send the funds from.\n" @@ -3622,24 +3623,33 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) } int nextBlockHeight = chainActive.Height() + 1; - size_t max_zaddr_outputs = Z_SENDMANY_MAX_ZADDR_OUTPUTS; + CMutableTransaction mtx; + mtx.fOverwintered = true; + mtx.nVersionGroupId = SAPLING_VERSION_GROUP_ID; + mtx.nVersion = SAPLING_TX_VERSION; unsigned int max_tx_size = MAX_TX_SIZE_AFTER_SAPLING; if (!NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) { - max_zaddr_outputs = Z_SENDMANY_MAX_ZADDR_OUTPUTS_BEFORE_SAPLING; - max_tx_size = MAX_TX_SIZE_BEFORE_SAPLING; - } + if (NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER)) { + mtx.nVersionGroupId = OVERWINTER_VERSION_GROUP_ID; + mtx.nVersion = OVERWINTER_TX_VERSION; + } else { + mtx.fOverwintered = false; + mtx.nVersion = 2; + } - // Check the number of zaddr outputs does not exceed the limit. - if (zaddrRecipients.size() > max_zaddr_outputs) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, too many zaddr outputs"); + max_tx_size = MAX_TX_SIZE_BEFORE_SAPLING; + + // Check the number of zaddr outputs does not exceed the limit. + if (zaddrRecipients.size() > Z_SENDMANY_MAX_ZADDR_OUTPUTS_BEFORE_SAPLING) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, too many zaddr outputs"); + } } // As a sanity check, estimate and verify that the size of the transaction will be valid. // Depending on the input notes, the actual tx size may turn out to be larger and perhaps invalid. size_t txsize = 0; - CMutableTransaction mtx; - mtx.nVersion = 2; for (int i = 0; i < zaddrRecipients.size(); i++) { + // TODO Check whether the recipient is a Sprout or Sapling address mtx.vjoinsplit.push_back(JSDescription()); } CTransaction tx(mtx); From e1dbec49b7a7f03da00f263492153fd092ab2ad1 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 1 May 2018 20:15:04 +0100 Subject: [PATCH 5/5] Add test of Sapling transaction size boundary --- src/gtest/test_checktransaction.cpp | 60 +++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/src/gtest/test_checktransaction.cpp b/src/gtest/test_checktransaction.cpp index bc60209cb..a0efee14e 100644 --- a/src/gtest/test_checktransaction.cpp +++ b/src/gtest/test_checktransaction.cpp @@ -194,6 +194,66 @@ TEST(checktransaction_tests, BadTxnsOversize) { } } +TEST(checktransaction_tests, OversizeSaplingTxns) { + SelectParams(CBaseChainParams::REGTEST); + UpdateNetworkUpgradeParameters(Consensus::UPGRADE_OVERWINTER, Consensus::NetworkUpgrade::ALWAYS_ACTIVE); + UpdateNetworkUpgradeParameters(Consensus::UPGRADE_SAPLING, Consensus::NetworkUpgrade::ALWAYS_ACTIVE); + + CMutableTransaction mtx = GetValidTransaction(); + mtx.fOverwintered = true; + mtx.nVersionGroupId = SAPLING_VERSION_GROUP_ID; + mtx.nVersion = SAPLING_TX_VERSION; + + // Change the proof types (which requires re-signing the JoinSplit data) + mtx.vjoinsplit[0].proof = libzcash::GrothProof(); + mtx.vjoinsplit[1].proof = libzcash::GrothProof(); + CreateJoinSplitSignature(mtx, NetworkUpgradeInfo[Consensus::UPGRADE_SAPLING].nBranchId); + + // Transaction just under the limit + mtx.vin[0].scriptSig = CScript(); + std::vector vchData(520); + for (unsigned int i = 0; i < 3809; ++i) + mtx.vin[0].scriptSig << vchData << OP_DROP; + std::vector vchDataRemainder(453); + mtx.vin[0].scriptSig << vchDataRemainder << OP_DROP; + mtx.vin[0].scriptSig << OP_1; + + { + CTransaction tx(mtx); + EXPECT_EQ(::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION), MAX_TX_SIZE_AFTER_SAPLING - 1); + + CValidationState state; + EXPECT_TRUE(CheckTransactionWithoutProofVerification(tx, state)); + } + + // Transaction equal to the limit + mtx.vin[1].scriptSig << OP_1; + + { + CTransaction tx(mtx); + EXPECT_EQ(::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION), MAX_TX_SIZE_AFTER_SAPLING); + + CValidationState state; + EXPECT_TRUE(CheckTransactionWithoutProofVerification(tx, state)); + } + + // Transaction just over the limit + mtx.vin[1].scriptSig << OP_1; + + { + CTransaction tx(mtx); + EXPECT_EQ(::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION), MAX_TX_SIZE_AFTER_SAPLING + 1); + + MockCValidationState state; + EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-oversize", false)).Times(1); + EXPECT_FALSE(CheckTransactionWithoutProofVerification(tx, state)); + } + + // Revert to default + UpdateNetworkUpgradeParameters(Consensus::UPGRADE_SAPLING, Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT); + UpdateNetworkUpgradeParameters(Consensus::UPGRADE_OVERWINTER, Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT); +} + TEST(checktransaction_tests, bad_txns_vout_negative) { CMutableTransaction mtx = GetValidTransaction(); mtx.vout[0].nValue = -1;