From 97b46f00cccc7fd803b5a43708d30d5b14480981 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 8 May 2018 13:51:54 +0100 Subject: [PATCH 1/2] Add valueBalance to value balances, and enforce its consensus rules --- src/coins.cpp | 2 +- src/coins.h | 2 +- src/gtest/test_checktransaction.cpp | 48 +++++++++++++++++++++++++++++ src/main.cpp | 26 ++++++++++++++-- src/miner.cpp | 2 +- src/primitives/transaction.cpp | 27 +++++++++++++--- src/primitives/transaction.h | 6 ++-- src/wallet/wallet.cpp | 4 +-- 8 files changed, 102 insertions(+), 15 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 0ae74af1d..e2d85ebe3 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -546,7 +546,7 @@ CAmount CCoinsViewCache::GetValueIn(const CTransaction& tx) const for (unsigned int i = 0; i < tx.vin.size(); i++) nResult += GetOutputFor(tx.vin[i]).nValue; - nResult += tx.GetJoinSplitValueIn(); + nResult += tx.GetShieldedValueIn(); return nResult; } diff --git a/src/coins.h b/src/coins.h index bd5105024..fa174dc12 100644 --- a/src/coins.h +++ b/src/coins.h @@ -526,7 +526,7 @@ public: * so may not be able to calculate this. * * @param[in] tx transaction for which we are checking input total - * @return Sum of value of all inputs (scriptSigs) + * @return Sum of value of all inputs (scriptSigs), (positive valueBalance or zero) and JoinSplit vpub_new */ CAmount GetValueIn(const CTransaction& tx) const; diff --git a/src/gtest/test_checktransaction.cpp b/src/gtest/test_checktransaction.cpp index 467c2cf3b..1faf87340 100644 --- a/src/gtest/test_checktransaction.cpp +++ b/src/gtest/test_checktransaction.cpp @@ -288,6 +288,54 @@ TEST(checktransaction_tests, bad_txns_txouttotal_toolarge_outputs) { CheckTransactionWithoutProofVerification(tx, state); } +TEST(checktransaction_tests, ValueBalanceNonZero) { + CMutableTransaction mtx = GetValidTransaction(); + mtx.valueBalance = 10; + + CTransaction tx(mtx); + + MockCValidationState state; + EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-valuebalance-nonzero", false)).Times(1); + CheckTransactionWithoutProofVerification(tx, state); +} + +TEST(checktransaction_tests, PositiveValueBalanceTooLarge) { + CMutableTransaction mtx = GetValidTransaction(); + mtx.vShieldedSpend.resize(1); + mtx.valueBalance = MAX_MONEY + 1; + + CTransaction tx(mtx); + + MockCValidationState state; + EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-valuebalance-toolarge", false)).Times(1); + CheckTransactionWithoutProofVerification(tx, state); +} + +TEST(checktransaction_tests, NegativeValueBalanceTooLarge) { + CMutableTransaction mtx = GetValidTransaction(); + mtx.vShieldedSpend.resize(1); + mtx.valueBalance = -(MAX_MONEY + 1); + + CTransaction tx(mtx); + + MockCValidationState state; + EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-valuebalance-toolarge", false)).Times(1); + CheckTransactionWithoutProofVerification(tx, state); +} + +TEST(checktransaction_tests, ValueBalanceOverflowsTotal) { + CMutableTransaction mtx = GetValidTransaction(); + mtx.vShieldedSpend.resize(1); + mtx.vout[0].nValue = 1; + mtx.valueBalance = -MAX_MONEY; + + CTransaction tx(mtx); + + MockCValidationState state; + EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-txouttotal-toolarge", false)).Times(1); + CheckTransactionWithoutProofVerification(tx, state); +} + TEST(checktransaction_tests, bad_txns_txouttotal_toolarge_joinsplit) { CMutableTransaction mtx = GetValidTransaction(); mtx.vout[0].nValue = 1; diff --git a/src/main.cpp b/src/main.cpp index f6c211c3b..293dc52c6 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1083,6 +1083,28 @@ bool CheckTransactionWithoutProofVerification(const CTransaction& tx, CValidatio REJECT_INVALID, "bad-txns-txouttotal-toolarge"); } + // Check for non-zero valueBalance when there are no Sapling inputs or outputs + if (tx.vShieldedSpend.empty() && tx.vShieldedOutput.empty() && tx.valueBalance != 0) { + return state.DoS(100, error("CheckTransaction(): tx.valueBalance has no sources or sinks"), + REJECT_INVALID, "bad-txns-valuebalance-nonzero"); + } + + // Check for overflow valueBalance + if (tx.valueBalance > MAX_MONEY || tx.valueBalance < -MAX_MONEY) { + return state.DoS(100, error("CheckTransaction(): abs(tx.valueBalance) too large"), + REJECT_INVALID, "bad-txns-valuebalance-toolarge"); + } + + if (tx.valueBalance <= 0) { + // NB: negative valueBalance "takes" money from the transparent value pool just as outputs do + nValueOut += -tx.valueBalance; + + if (!MoneyRange(nValueOut)) { + return state.DoS(100, error("CheckTransaction(): txout total out of range"), + REJECT_INVALID, "bad-txns-txouttotal-toolarge"); + } + } + // Ensure that joinsplit values are well-formed BOOST_FOREACH(const JSDescription& joinsplit, tx.vjoinsplit) { @@ -1890,9 +1912,9 @@ bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoins } - nValueIn += tx.GetJoinSplitValueIn(); + nValueIn += tx.GetShieldedValueIn(); if (!MoneyRange(nValueIn)) - return state.DoS(100, error("CheckInputs(): vpub_old values out of range"), + return state.DoS(100, error("CheckInputs(): shielded input to transparent value pool out of range"), REJECT_INVALID, "bad-txns-inputvalues-outofrange"); if (nValueIn < tx.GetValueOut()) diff --git a/src/miner.cpp b/src/miner.cpp index 7c6e4140a..65c081ce2 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -216,7 +216,7 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn) dPriority += (double)nValueIn * nConf; } - nTotalIn += tx.GetJoinSplitValueIn(); + nTotalIn += tx.GetShieldedValueIn(); if (fMissingInputs) continue; diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp index 70c308380..a18e8f65a 100644 --- a/src/primitives/transaction.cpp +++ b/src/primitives/transaction.cpp @@ -256,9 +256,18 @@ CAmount CTransaction::GetValueOut() const throw std::runtime_error("CTransaction::GetValueOut(): value out of range"); } + if (valueBalance <= 0) { + // NB: negative valueBalance "takes" money from the transparent value pool just as outputs do + nValueOut += -valueBalance; + + if (!MoneyRange(-valueBalance) || !MoneyRange(nValueOut)) { + throw std::runtime_error("CTransaction::GetValueOut(): value out of range"); + } + } + for (std::vector::const_iterator it(vjoinsplit.begin()); it != vjoinsplit.end(); ++it) { - // NB: vpub_old "takes" money from the value pool just as outputs do + // NB: vpub_old "takes" money from the transparent value pool just as outputs do nValueOut += it->vpub_old; if (!MoneyRange(it->vpub_old) || !MoneyRange(nValueOut)) @@ -267,16 +276,26 @@ CAmount CTransaction::GetValueOut() const return nValueOut; } -CAmount CTransaction::GetJoinSplitValueIn() const +CAmount CTransaction::GetShieldedValueIn() const { CAmount nValue = 0; + + if (valueBalance >= 0) { + // NB: positive valueBalance "gives" money to the transparent value pool just as inputs do + nValue += valueBalance; + + if (!MoneyRange(valueBalance) || !MoneyRange(nValue)) { + throw std::runtime_error("CTransaction::GetShieldedValueIn(): value out of range"); + } + } + for (std::vector::const_iterator it(vjoinsplit.begin()); it != vjoinsplit.end(); ++it) { - // NB: vpub_new "gives" money to the value pool just as inputs do + // NB: vpub_new "gives" money to the transparent value pool just as inputs do nValue += it->vpub_new; if (!MoneyRange(it->vpub_new) || !MoneyRange(nValue)) - throw std::runtime_error("CTransaction::GetJoinSplitValueIn(): value out of range"); + throw std::runtime_error("CTransaction::GetShieldedValueIn(): value out of range"); } return nValue; diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index 74e76858a..0fb7491de 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -651,13 +651,13 @@ public: return header; } - // Return sum of txouts. + // Return sum of txouts, (negative valueBalance or zero) and JoinSplit vpub_old. CAmount GetValueOut() const; // GetValueIn() is a method on CCoinsViewCache, because // inputs must be known to compute value in. - // Return sum of JoinSplit vpub_new - CAmount GetJoinSplitValueIn() const; + // Return sum of (positive valueBalance or zero) and JoinSplit vpub_new + CAmount GetShieldedValueIn() const; // Compute priority, given priority of inputs and (optionally) tx size double ComputePriority(double dPriorityInputs, unsigned int nTxSize=0) const; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9aa4c4918..fe60c3562 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1592,9 +1592,7 @@ void CWalletTx::GetAmounts(list& listReceived, if (isFromMyTaddr) { CAmount nValueOut = GetValueOut(); // transparent outputs plus all vpub_old CAmount nValueIn = 0; - for (const JSDescription & js : vjoinsplit) { - nValueIn += js.vpub_new; - } + nValueIn += GetShieldedValueIn(); nFee = nDebit - nValueOut + nValueIn; } From 0fe0ca7948c7de42b39f33caf27c4606ff537394 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 10 May 2018 09:34:19 -0400 Subject: [PATCH 2/2] Add contextual comment for GetValueOut() and GetShieldedValueIn() --- src/primitives/transaction.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index 0fb7491de..1066601a1 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -651,6 +651,17 @@ public: return header; } + /* + * Context for the two methods below: + * As at most one of vpub_new and vpub_old is non-zero in every JoinSplit, + * we can think of a JoinSplit as an input or output according to which one + * it is (e.g. if vpub_new is non-zero the joinSplit is "giving value" to + * the outputs in the transaction). Similarly, we can think of the Sapling + * shielded part of the transaction as an input or output according to + * whether valueBalance - the sum of shielded input values minus the sum of + * shielded output values - is positive or negative. + */ + // Return sum of txouts, (negative valueBalance or zero) and JoinSplit vpub_old. CAmount GetValueOut() const; // GetValueIn() is a method on CCoinsViewCache, because