From 6267534c0e51a76eeb14526876fdf9adbca7b163 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Mon, 25 Jan 2021 11:24:12 +0100 Subject: [PATCH 01/17] [MOVE] rename files specialtx_validation.* --> specialtx.* Also rename 'tiertwo' folder to 'evo' --- CMakeLists.txt | 2 +- src/Makefile.am | 4 ++-- src/consensus/tx_verify.cpp | 2 +- src/{tiertwo/specialtx_validation.cpp => evo/specialtx.cpp} | 3 ++- src/{tiertwo/specialtx_validation.h => evo/specialtx.h} | 2 +- src/test/validation_tests.cpp | 2 +- 6 files changed, 8 insertions(+), 7 deletions(-) rename src/{tiertwo/specialtx_validation.cpp => evo/specialtx.cpp} (98%) rename src/{tiertwo/specialtx_validation.h => evo/specialtx.h} (97%) diff --git a/CMakeLists.txt b/CMakeLists.txt index 243a847df20b..bd3710375d41 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -383,7 +383,7 @@ set(COMMON_SOURCES ./src/coins.cpp ./src/key_io.cpp ./src/compressor.cpp - ./src/tiertwo/specialtx_validation.cpp + ./src/evo/specialtx.cpp ./src/consensus/merkle.cpp ./src/consensus/zerocoin_verify.cpp ./src/primitives/block.cpp diff --git a/src/Makefile.am b/src/Makefile.am index 7c5df62ac0e1..59e2e67dc249 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -181,6 +181,7 @@ BITCOIN_CORE_H = \ cuckoocache.h \ crypter.h \ cyclingvector.h \ + evo/specialtx.h \ pairresult.h \ addressbook.h \ wallet/db.h \ @@ -243,7 +244,6 @@ BITCOIN_CORE_H = \ rpc/protocol.h \ rpc/register.h \ rpc/server.h \ - tiertwo/specialtx_validation.h \ scheduler.h \ script/interpreter.h \ script/keyorigin.h \ @@ -322,6 +322,7 @@ libbitcoin_server_a_SOURCES = \ consensus/params.cpp \ consensus/tx_verify.cpp \ consensus/zerocoin_verify.cpp \ + evo/specialtx.cpp \ httprpc.cpp \ httpserver.cpp \ init.cpp \ @@ -346,7 +347,6 @@ libbitcoin_server_a_SOURCES = \ rpc/net.cpp \ rpc/rawtransaction.cpp \ rpc/server.cpp \ - tiertwo/specialtx_validation.cpp \ script/sigcache.cpp \ script/ismine.cpp \ sporkdb.cpp \ diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index 3b829b48810d..122c68f1afdf 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -5,9 +5,9 @@ #include "tx_verify.h" #include "consensus/consensus.h" +#include "evo/specialtx.h" #include "consensus/zerocoin_verify.h" #include "sapling/sapling_validation.h" -#include "tiertwo/specialtx_validation.h" #include "../validation.h" bool IsFinalTx(const CTransactionRef& tx, int nBlockHeight, int64_t nBlockTime) diff --git a/src/tiertwo/specialtx_validation.cpp b/src/evo/specialtx.cpp similarity index 98% rename from src/tiertwo/specialtx_validation.cpp rename to src/evo/specialtx.cpp index 952a0b8a765f..f2d578ff73d5 100644 --- a/src/tiertwo/specialtx_validation.cpp +++ b/src/evo/specialtx.cpp @@ -4,7 +4,7 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#include "tiertwo/specialtx_validation.h" +#include "evo/specialtx.h" #include "chain.h" #include "chainparams.h" @@ -71,3 +71,4 @@ bool UndoSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindexPrev) /* undo special txes in batches */ return true; } + diff --git a/src/tiertwo/specialtx_validation.h b/src/evo/specialtx.h similarity index 97% rename from src/tiertwo/specialtx_validation.h rename to src/evo/specialtx.h index 2dd142d721fd..7ff41eb8bfda 100644 --- a/src/tiertwo/specialtx_validation.h +++ b/src/evo/specialtx.h @@ -27,4 +27,4 @@ bool CheckSpecialTx(const CTransaction& tx, CValidationState& state, bool fIsSap bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindexPrev, CValidationState& state); bool UndoSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindexPrev); -#endif //PIVX_SAPLING_VALIDATION_H +#endif // PIVX_SPECIALTX_H diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp index dfe5e59e108f..af8097a49a5a 100644 --- a/src/test/validation_tests.cpp +++ b/src/test/validation_tests.cpp @@ -5,7 +5,7 @@ #include "test/test_pivx.h" #include "primitives/transaction.h" #include "sapling/sapling_validation.h" -#include "tiertwo/specialtx_validation.h" +#include "evo/specialtx.h" #include "test/librust/utiltest.h" #include From a0a5170b68bfbc7b273fb51af0bf0335ba9d8811 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Mon, 25 Jan 2021 10:29:07 +0100 Subject: [PATCH 02/17] [Refactor] Introduce GetUTXO* functions relying on pcoinsTip --- src/validation.cpp | 17 +++++++++++++++++ src/validation.h | 6 ++++++ 2 files changed, 23 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index 037033e25387..259db8be0853 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -251,6 +251,23 @@ bool CheckFinalTx(const CTransactionRef& tx, int flags) return IsFinalTx(tx, nBlockHeight, nBlockTime); } +bool GetUTXOCoin(const COutPoint& outpoint, Coin& coin) +{ + LOCK(cs_main); + if (!pcoinsTip->GetCoin(outpoint, coin)) + return false; + if (coin.IsSpent()) + return false; + return true; +} + +Optional GetUTXOHeight(const COutPoint& outpoint) +{ + // nullopt means UTXO is yet unknown or already spent + Coin coin; + return GetUTXOCoin(outpoint, coin) ? Optional(coin.nHeight) : nullopt; +} + void LimitMempoolSize(CTxMemPool& pool, size_t limit, unsigned long age) { int expired = pool.Expire(GetTime() - age); if (expired != 0) diff --git a/src/validation.h b/src/validation.h index 3c2b61cebf2a..eeb0e8f822da 100644 --- a/src/validation.h +++ b/src/validation.h @@ -271,6 +271,12 @@ bool IsBlockHashInChain(const uint256& hashBlock); */ bool CheckFinalTx(const CTransactionRef& tx, int flags = -1); +/* + * Retrieve an unspent coin in pcoinsTip. Lock cs_main. + */ +bool GetUTXOCoin(const COutPoint& outpoint, Coin& coin); +Optional GetUTXOHeight(const COutPoint& outpoint); + /** * Closure representing one script verification * Note that this stores references to the spending transaction From 211b36d908a2261f900a63141fe8896ab6274c95 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 11 Feb 2021 09:42:41 +0100 Subject: [PATCH 03/17] [Tests] Fix proper sapling tx version in unit tests --- src/test/sighash_tests.cpp | 6 +----- src/test/transaction_tests.cpp | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/test/sighash_tests.cpp b/src/test/sighash_tests.cpp index 2ac55d7f0ede..e773db30388e 100644 --- a/src/test/sighash_tests.cpp +++ b/src/test/sighash_tests.cpp @@ -95,11 +95,7 @@ void static RandomScript(CScript &script) { void static RandomTransaction(CMutableTransaction &tx, bool fSingle) { bool isSapling = !(InsecureRand32() % 7); - if (isSapling) { - tx.nVersion = 2; - } else { - do tx.nVersion = InsecureRand32(); while (tx.nVersion == 2); - } + tx.nVersion = isSapling ? CTransaction::TxVersion::SAPLING : CTransaction::TxVersion::LEGACY; tx.vin.clear(); tx.vout.clear(); tx.sapData->vShieldedSpend.clear(); diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index e9ef4131e30f..6457884ccffb 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -327,7 +327,7 @@ BOOST_AUTO_TEST_CASE(test_Get) BOOST_AUTO_TEST_CASE(test_big_witness_transaction) { CMutableTransaction mtx; - mtx.nVersion = 2; // Sapling future version + mtx.nVersion = CTransaction::TxVersion::SAPLING; CKey key; key.MakeNewKey(false); From 69662f8b19a41d3a16545443da50cc56f77228f7 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 11 Feb 2021 12:07:27 +0100 Subject: [PATCH 04/17] [Core] Add GetSerializeSizeNetwork for generic (non-array) Optionals --- src/serialize.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/serialize.h b/src/serialize.h index 333586d66a3c..4c6f9d2b8428 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -581,6 +581,7 @@ template void Unserialize(Stream& is * optional */ template unsigned int GetSerializeSize(const Optional &item); +template unsigned int GetSerializeSizeNetwork(const Optional &item); template void Serialize(Stream& os, const Optional& item); template void Unserialize(Stream& is, Optional& item); @@ -610,6 +611,16 @@ unsigned int GetSerializeSize(const Optional &item) } } +template +unsigned int GetSerializeSizeNetwork(const Optional &item) +{ + if (item) { + return 1 + GetSerializeSize(*item, SER_NETWORK, 0); + } else { + return 1; + } +} + template void Serialize(Stream& os, const Optional& item) { From e288fd63c4f5db71b29d6d2aae55df1cad5c85a5 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 11 Feb 2021 12:17:26 +0100 Subject: [PATCH 05/17] [RPC] Return full debug message for ATMP failures in sendrawtransaction --- src/rpc/rawtransaction.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 3623b76033ed..4720d95e3ff2 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -909,12 +909,12 @@ UniValue sendrawtransaction(const JSONRPCRequest& request) { if (!AcceptToMemoryPool(mempool, state, MakeTransactionRef(std::move(mtx)), true, &fMissingInputs, false, !fOverrideFees)) { if (state.IsInvalid()) { - throw JSONRPCError(RPC_TRANSACTION_REJECTED, strprintf("%i: %s", state.GetRejectCode(), state.GetRejectReason())); + throw JSONRPCError(RPC_TRANSACTION_REJECTED, strprintf("%s: %s", state.GetRejectReason(), state.GetDebugMessage())); } else { if (fMissingInputs) { throw JSONRPCError(RPC_TRANSACTION_ERROR, "Missing inputs"); } - throw JSONRPCError(RPC_TRANSACTION_ERROR, state.GetRejectReason()); + throw JSONRPCError(RPC_TRANSACTION_ERROR, strprintf("%s: %s", state.GetRejectReason(), state.GetDebugMessage())); } } else { // If wallet is enabled, ensure that the wallet has been made aware From 286a35e59059fd727aec0bd036f17a61d217f3b2 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 11 Mar 2021 00:37:18 +0100 Subject: [PATCH 06/17] [Refactoring] Make CWallet::FundTransaction atomic >>> Inspired by bitcoin#11864 (+bitcoin#10784) Since CreateTransaction still needs cs_main we need to obtain both locks in FundTransaction, to preserve locking order. After introducing the chain interface (bitcoin#14437) we can change the LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet) --- src/wallet/wallet.cpp | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index be3fc351f44f..aebf5f44a48d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2854,8 +2854,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool ov // Turn the txout set into a CRecipient vector for (const CTxOut& txOut : tx.vout) { - CRecipient recipient = {txOut.scriptPubKey, txOut.nValue, false}; - vecSend.push_back(recipient); + vecSend.emplace_back(txOut.scriptPubKey, txOut.nValue, false); } CCoinControl coinControl; @@ -2865,26 +2864,31 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool ov coinControl.fOverrideFeeRate = overrideEstimatedFeeRate; coinControl.nFeeRate = specificFeeRate; - for (const CTxIn& txin : tx.vin) + for (const CTxIn& txin : tx.vin) { coinControl.Select(txin.prevout); + } + + // Acquire the locks to prevent races to the new locked unspents between the + // CreateTransaction call and LockCoin calls (when lockUnspents is true). + LOCK2(cs_main, cs_wallet); CReserveKey reservekey(this); CTransactionRef wtx; if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, &coinControl, ALL_COINS, false)) return false; - if (nChangePosInOut != -1) + if (nChangePosInOut != -1) { tx.vout.insert(tx.vout.begin() + nChangePosInOut, wtx->vout[nChangePosInOut]); + // We don't have the normal Create/Commit cycle, and don't want to risk + // reusing change, so just remove the key from the keypool here. + reservekey.KeepKey(); + } - // Add new txins (keeping original txin scriptSig/order) + // Add new txins while keeping original txin scriptSig/order. for (const CTxIn& txin : wtx->vin) { if (!coinControl.IsSelected(txin.prevout)) { - tx.vin.push_back(txin); - - if (lockUnspents) { - LOCK(cs_wallet); - LockCoin(txin.prevout); - } + tx.vin.emplace_back(txin); + if (lockUnspents) LockCoin(txin.prevout); } } From 517fe0c461621c9d75971d87edc24e51899986e1 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 11 Mar 2021 00:51:19 +0100 Subject: [PATCH 07/17] [Trivial][Cleanup] Remove unused variable in ScanForWalletTransactions --- src/wallet/wallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index aebf5f44a48d..8a6b4e476179 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1821,7 +1821,6 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock { CBlockIndex* ret = nullptr; int64_t nNow = GetTime(); - const Consensus::Params& consensus = Params().GetConsensus(); if (pindexStop) { assert(pindexStop->nHeight >= pindexStart->nHeight); @@ -1829,6 +1828,7 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock fAbortRescan = false; fScanningWallet = true; + CBlockIndex* pindex = pindexStart; { LOCK(cs_main); From 9ef803167dd95cd315c5c8c8ae353b3b563271b2 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 11 Feb 2021 12:18:22 +0100 Subject: [PATCH 08/17] [Wallet] Account for extra payload sizes in Fund/CreateTransaction --- src/wallet/wallet.cpp | 12 +++++++++--- src/wallet/wallet.h | 4 +++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8a6b4e476179..2b26cfef99be 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2864,6 +2864,9 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool ov coinControl.fOverrideFeeRate = overrideEstimatedFeeRate; coinControl.nFeeRate = specificFeeRate; + const int nExtraSize = tx.isSaplingVersion() ? + (int)(GetSerializeSizeNetwork(tx.sapData) + GetSerializeSizeNetwork(tx.extraPayload)) : 0; + for (const CTxIn& txin : tx.vin) { coinControl.Select(txin.prevout); } @@ -2874,8 +2877,9 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool ov CReserveKey reservekey(this); CTransactionRef wtx; - if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, &coinControl, ALL_COINS, false)) + if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, &coinControl, ALL_COINS, false, 0, false, nullptr, nExtraSize)) { return false; + } if (nChangePosInOut != -1) { tx.vout.insert(tx.vout.begin() + nChangePosInOut, wtx->vout[nChangePosInOut]); @@ -2906,7 +2910,8 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, bool sign, CAmount nFeePay, bool fIncludeDelegated, - bool* fStakeDelegationVoided) + bool* fStakeDelegationVoided, + int nExtraSize) { CAmount nValue = 0; int nChangePosRequest = nChangePosInOut; @@ -3057,7 +3062,8 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, nIn++; } - const unsigned int nBytes = ::GetSerializeSize(txNew, SER_NETWORK, PROTOCOL_VERSION); + // account for additional payloads in fee calculation + const unsigned int nBytes = ::GetSerializeSize(txNew, SER_NETWORK, PROTOCOL_VERSION) + nExtraSize; CAmount nFeeNeeded = std::max(nFeePay, GetMinimumFee(nBytes, nTxConfirmTarget, mempool)); // Remove scriptSigs to eliminate the fee calculation dummy signatures diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 838799b7a614..818f43e434ae 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1005,7 +1005,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface bool sign = true, CAmount nFeePay = 0, bool fIncludeDelegated = false, - bool* fStakeDelegationVoided = nullptr); + bool* fStakeDelegationVoided = nullptr, + int nExtraSize = 0); + bool CreateTransaction(CScript scriptPubKey, const CAmount& nValue, CTransactionRef& tx, CReserveKey& reservekey, CAmount& nFeeRet, std::string& strFailReason, const CCoinControl* coinControl = NULL, AvailableCoinsType coin_type = ALL_COINS, CAmount nFeePay = 0, bool fIncludeDelegated = false); // enumeration for CommitResult (return status of CommitTransaction) From e1eb3f5d32ad692178817dbdb4b7d31398e78f6f Mon Sep 17 00:00:00 2001 From: random-zebra Date: Wed, 27 Jan 2021 15:13:12 +0100 Subject: [PATCH 09/17] [Cleanup] Remove fSaplingActive from CheckSpecialTx --- src/consensus/tx_verify.cpp | 2 +- src/evo/specialtx.cpp | 9 +-------- src/evo/specialtx.h | 2 +- src/test/validation_tests.cpp | 24 ++++++++---------------- 4 files changed, 11 insertions(+), 26 deletions(-) diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index 122c68f1afdf..1bb25cb705c3 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -86,7 +86,7 @@ bool CheckTransaction(const CTransaction& tx, bool fZerocoinActive, CValidationS } // Dispatch to SpecialTx validator - if (!CheckSpecialTx(tx, state, fSaplingActive)) { + if (!CheckSpecialTx(tx, state)) { return false; } diff --git a/src/evo/specialtx.cpp b/src/evo/specialtx.cpp index f2d578ff73d5..d0960905ed80 100644 --- a/src/evo/specialtx.cpp +++ b/src/evo/specialtx.cpp @@ -12,17 +12,10 @@ #include "consensus/validation.h" #include "primitives/block.h" -bool CheckSpecialTx(const CTransaction& tx, CValidationState& state, bool fIsSaplingActive) +bool CheckSpecialTx(const CTransaction& tx, CValidationState& state) { bool hasExtraPayload = tx.hasExtraPayload(); - // Before Sapling activation, the tx has a single 32bit version and no type. - // Versions > 1 are not standard, but still accepted. No TX can have extra payload. - if (!fIsSaplingActive) { - return !hasExtraPayload; - } - - // After Sapling activation. // v1/v2 can only be Type=0 if (!tx.isSaplingVersion() && tx.nType != CTransaction::TxType::NORMAL) { return state.DoS(100, error("%s: Type %d not supported with version %d", __func__, tx.nType, tx.nVersion), diff --git a/src/evo/specialtx.h b/src/evo/specialtx.h index 7ff41eb8bfda..58b8fdce6218 100644 --- a/src/evo/specialtx.h +++ b/src/evo/specialtx.h @@ -21,7 +21,7 @@ static const unsigned int MAX_SPECIALTX_EXTRAPAYLOAD = 10000; /** Context-independent validity checks */ // Note: for +v2, if the tx is not a special tx, this method returns true. // Note2: This function only performs extra payload related checks, it does NOT checks regular inputs and outputs. -bool CheckSpecialTx(const CTransaction& tx, CValidationState& state, bool fIsSaplingActive); +bool CheckSpecialTx(const CTransaction& tx, CValidationState& state); // Update internal tiertwo data when blocks containing special txes get connected/disconnected bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindexPrev, CValidationState& state); diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp index af8097a49a5a..bb8e1337108d 100644 --- a/src/test/validation_tests.cpp +++ b/src/test/validation_tests.cpp @@ -14,48 +14,40 @@ BOOST_FIXTURE_TEST_SUITE(validation_tests, TestingSetup) BOOST_AUTO_TEST_CASE(special_tx_validation_test) { - // First check, sapling not active, transaction with extra payload CMutableTransaction mtx; - mtx.extraPayload = std::vector(10, 1); CValidationState state; - BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), state, false)); - - // Now activate sapling - RegtestActivateSapling(); - // After Sapling activation. // v1 can only be Type=0 mtx.nType = 1; mtx.nVersion = CTransaction::TxVersion::LEGACY; - BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), state, true)); + BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), state)); BOOST_CHECK(state.GetRejectReason().find("not supported with version 0")); // version >= Sapling, type = 0, payload != null. - mtx.nType = 0; + mtx.nType = CTransaction::TxType::NORMAL; + mtx.extraPayload = std::vector(10, 1); mtx.nVersion = CTransaction::TxVersion::SAPLING; - BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), state, true)); + BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), state)); BOOST_CHECK(state.GetRejectReason().find("doesn't support extra payload")); // version >= Sapling, type = 0, payload == null --> pass mtx.extraPayload = nullopt; - BOOST_CHECK(CheckSpecialTx(CTransaction(mtx), state, true)); + BOOST_CHECK(CheckSpecialTx(CTransaction(mtx), state)); // nVersion>=2 and nType!=0 without extrapayload mtx.nType = 1; - BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), state, true)); + BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), state)); BOOST_CHECK(state.GetRejectReason().find("without extra payload")); // Size limits mtx.extraPayload = std::vector(MAX_SPECIALTX_EXTRAPAYLOAD + 1, 1); - BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), state, true)); + BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), state)); BOOST_CHECK(state.GetRejectReason().find("Special tx payload oversize")); // Remove one element, so now it passes the size check mtx.extraPayload->pop_back(); - BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), state, true)); + BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), state)); BOOST_CHECK(state.GetRejectReason().find("with invalid type")); - - RegtestDeactivateSapling(); } void test_simple_sapling_invalidity(CMutableTransaction& tx) From dab383088a8f705bae9a2760a26b34ff46e2a348 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Sat, 13 Feb 2021 04:51:21 +0100 Subject: [PATCH 10/17] [Tests][Trivial] Remove annoying warning for unintended optimization --- src/test/budget_tests.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/budget_tests.cpp b/src/test/budget_tests.cpp index 5ec1972b3286..0cf309efdc40 100644 --- a/src/test/budget_tests.cpp +++ b/src/test/budget_tests.cpp @@ -22,7 +22,7 @@ class BudgetManagerTest : CBudgetManager mapFeeTxToBudget.emplace(finalizedBudget.GetFeeTXHash(), nHash); } - bool IsBlockValueValid(int nHeight, CAmount& nExpectedValue, CAmount nMinted, bool fSporkActive = true) + bool IsBlockValueValid(int nHeight, CAmount nExpectedValue, CAmount nMinted, bool fSporkActive = true) { // suppose masternodeSync is complete if (fSporkActive) { @@ -66,7 +66,7 @@ BOOST_AUTO_TEST_CASE(block_value) // regular block int nHeight = 100; - CAmount nExpected = GetBlockValue(nHeight); + volatile CAmount nExpected = GetBlockValue(nHeight); BOOST_CHECK(t_budgetman.IsBlockValueValid(nHeight, nExpected, nExpected-1)); BOOST_CHECK(t_budgetman.IsBlockValueValid(nHeight, nExpected, nExpected)); BOOST_CHECK(!t_budgetman.IsBlockValueValid(nHeight, nExpected, nExpected+1)); From 11d20254b998ce4854f519ac85351b6ad21586e0 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Mon, 8 Feb 2021 18:42:46 +0100 Subject: [PATCH 11/17] [Refactoring] move sapling contextual checks out of CheckTransaction --- src/consensus/tx_verify.cpp | 14 ++++++-------- src/consensus/tx_verify.h | 2 +- src/qt/walletmodel.cpp | 3 +-- src/sapling/sapling_validation.cpp | 13 ++++++------- src/sapling/sapling_validation.h | 2 +- src/test/sighash_tests.cpp | 2 +- src/test/transaction_tests.cpp | 8 ++++---- src/test/validation_tests.cpp | 8 ++++---- src/validation.cpp | 8 +++----- ...llet_sapling_transactions_validations_tests.cpp | 2 +- 10 files changed, 28 insertions(+), 34 deletions(-) diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index 1bb25cb705c3..b289d39c4e07 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -52,7 +52,7 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& in return nSigOps; } -bool CheckTransaction(const CTransaction& tx, bool fZerocoinActive, CValidationState& state, bool fFakeSerialAttack, bool fColdStakingActive, bool fSaplingActive) +bool CheckTransaction(const CTransaction& tx, bool fZerocoinActive, CValidationState& state, bool fFakeSerialAttack, bool fColdStakingActive) { // Basic checks that don't depend on any context // Transactions containing empty `vin` must have non-empty `vShieldedSpend`. @@ -63,12 +63,10 @@ bool CheckTransaction(const CTransaction& tx, bool fZerocoinActive, CValidationS return state.DoS(10, false, REJECT_INVALID, "bad-txns-vout-empty"); // Version check - if (fSaplingActive) { - // After sapling activation we require 1 <= tx.nVersion < TxVersion::TOOHIGH - if (tx.nVersion < 1 || tx.nVersion >= CTransaction::TxVersion::TOOHIGH) - return state.DoS(10, - error("%s: Transaction version (%d) too high. Max: %d", __func__, tx.nVersion, int(CTransaction::TxVersion::TOOHIGH) - 1), - REJECT_INVALID, "bad-tx-version-too-high"); + if (tx.nVersion < 1 || tx.nVersion >= CTransaction::TxVersion::TOOHIGH) { + return state.DoS(10, + error("%s: Transaction version (%d) too high. Max: %d", __func__, tx.nVersion, int(CTransaction::TxVersion::TOOHIGH) - 1), + REJECT_INVALID, "bad-tx-version-too-high"); } // Size limits @@ -81,7 +79,7 @@ bool CheckTransaction(const CTransaction& tx, bool fZerocoinActive, CValidationS // Dispatch to Sapling validator CAmount nValueOut = 0; - if (!SaplingValidation::CheckTransaction(tx, state, nValueOut, fSaplingActive)) { + if (!SaplingValidation::CheckTransaction(tx, state, nValueOut)) { return false; } diff --git a/src/consensus/tx_verify.h b/src/consensus/tx_verify.h index 7ad826a5f243..65bcd1950678 100644 --- a/src/consensus/tx_verify.h +++ b/src/consensus/tx_verify.h @@ -17,7 +17,7 @@ class CValidationState; /** Transaction validation functions */ /** Context-independent validity checks */ -bool CheckTransaction(const CTransaction& tx, bool fZerocoinActive, CValidationState& state, bool fFakeSerialAttack = false, bool fColdStakingActive=false, bool fSaplingActive=false); +bool CheckTransaction(const CTransaction& tx, bool fZerocoinActive, CValidationState& state, bool fFakeSerialAttack, bool fColdStakingActive); /** * Count ECDSA signature operations the old-fashioned (pre-0.6) way diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 5caf65e2a216..6f69425a77af 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -516,12 +516,11 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction& transaction) { bool fColdStakingActive = isColdStakingNetworkelyEnabled(); - bool fSaplingActive = Params().GetConsensus().NetworkUpgradeActive(cachedNumBlocks, Consensus::UPGRADE_V5_0); // Double check the tx before doing anything CTransactionRef& newTx = transaction.getTransaction(); CValidationState state; - if (!CheckTransaction(*newTx, true, state, true, fColdStakingActive, fSaplingActive)) { + if (!CheckTransaction(*newTx, true, state, true, fColdStakingActive)) { return TransactionCheckFailed; } diff --git a/src/sapling/sapling_validation.cpp b/src/sapling/sapling_validation.cpp index 67ddb5786cee..e2ac6f248c99 100644 --- a/src/sapling/sapling_validation.cpp +++ b/src/sapling/sapling_validation.cpp @@ -16,7 +16,7 @@ namespace SaplingValidation { // Verifies that Shielded txs are properly formed and performs content-independent checks -bool CheckTransaction(const CTransaction& tx, CValidationState& state, CAmount& nValueOut, bool fIsSaplingActive) +bool CheckTransaction(const CTransaction& tx, CValidationState& state, CAmount& nValueOut) { bool hasSaplingData = tx.hasSaplingData(); @@ -38,12 +38,6 @@ bool CheckTransaction(const CTransaction& tx, CValidationState& state, CAmount& return state.DoS(100, error("%s: Sapling version with invalid data", __func__), REJECT_INVALID, "bad-txns-invalid-sapling"); - // If the v5 upgrade was not enforced, then let's not perform any check - if (!fIsSaplingActive) { - return state.DoS(100, error("%s: Sapling not activated", __func__), - REJECT_INVALID, "bad-txns-invalid-sapling-act"); - } - // Upgrade enforced, basic version rules passing, let's check it return CheckTransactionWithoutProofVerification(tx, state, nValueOut); } @@ -138,6 +132,11 @@ bool ContextualCheckTransaction( // If Sapling is not active return quickly and don't perform any check here. // basic data checks are performed in CheckTransaction which is ALWAYS called before ContextualCheckTransaction. if (!chainparams.GetConsensus().NetworkUpgradeActive(nHeight, Consensus::UPGRADE_V5_0)) { + // If the v5 upgrade was not enforced, then let's not perform any check + if (tx.IsShieldedTx()) { + return state.DoS(dosLevelConstricting, error("%s: Sapling not activated", __func__), + REJECT_INVALID, "bad-txns-invalid-sapling-act"); + } return true; } diff --git a/src/sapling/sapling_validation.h b/src/sapling/sapling_validation.h index d670f0be9e8d..61959a20d92a 100644 --- a/src/sapling/sapling_validation.h +++ b/src/sapling/sapling_validation.h @@ -16,7 +16,7 @@ namespace SaplingValidation { /** Context-independent validity checks */ // Note: for v3+, if the tx has no shielded data, this method returns true. // Note2: This function only performs shielded data related checks, it does NOT checks regular inputs and outputs. -bool CheckTransaction(const CTransaction& tx, CValidationState& state, CAmount& nValueOut, bool fIsSaplingActive); +bool CheckTransaction(const CTransaction& tx, CValidationState& state, CAmount& nValueOut); bool CheckTransactionWithoutProofVerification(const CTransaction& tx, CValidationState &state, CAmount& nValueOut); /** Check a transaction contextually against a set of consensus rules */ diff --git a/src/test/sighash_tests.cpp b/src/test/sighash_tests.cpp index e773db30388e..537fed3caef8 100644 --- a/src/test/sighash_tests.cpp +++ b/src/test/sighash_tests.cpp @@ -228,7 +228,7 @@ BOOST_AUTO_TEST_CASE(sighash_from_data) stream >> tx; CValidationState state; - BOOST_CHECK_MESSAGE(CheckTransaction(*tx, false, state), strTest); + BOOST_CHECK_MESSAGE(CheckTransaction(*tx, false, state, false, false), strTest); BOOST_CHECK(state.IsValid()); std::vector raw = ParseHex(raw_script); diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 6457884ccffb..d396a20e929c 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -139,7 +139,7 @@ BOOST_AUTO_TEST_CASE(tx_valid) CTransaction tx(deserialize, stream); CValidationState state; - BOOST_CHECK_MESSAGE(CheckTransaction(tx, false, state), strTest); + BOOST_CHECK_MESSAGE(CheckTransaction(tx, false, state, false, false), strTest); BOOST_CHECK(state.IsValid()); PrecomputedTransactionData precomTxData(tx); @@ -215,7 +215,7 @@ BOOST_AUTO_TEST_CASE(tx_invalid) CTransaction tx(deserialize, stream); CValidationState state; - fValid = CheckTransaction(tx, false, state) && state.IsValid(); + fValid = CheckTransaction(tx, false, state, false, false) && state.IsValid(); PrecomputedTransactionData precomTxData(tx); for (unsigned int i = 0; i < tx.vin.size() && fValid; i++) @@ -246,11 +246,11 @@ BOOST_AUTO_TEST_CASE(basic_transaction_tests) CMutableTransaction tx; stream >> tx; CValidationState state; - BOOST_CHECK_MESSAGE(CheckTransaction(tx, false, state) && state.IsValid(), "Simple deserialized transaction should be valid."); + BOOST_CHECK_MESSAGE(CheckTransaction(tx, false, state, false, false) && state.IsValid(), "Simple deserialized transaction should be valid."); // Check that duplicate txins fail tx.vin.push_back(tx.vin[0]); - BOOST_CHECK_MESSAGE(!CheckTransaction(tx, false, state) || !state.IsValid(), "Transaction with duplicate txins should be invalid."); + BOOST_CHECK_MESSAGE(!CheckTransaction(tx, false, state, false, false) || !state.IsValid(), "Transaction with duplicate txins should be invalid."); } // diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp index bb8e1337108d..b1cad33e339b 100644 --- a/src/test/validation_tests.cpp +++ b/src/test/validation_tests.cpp @@ -57,7 +57,7 @@ void test_simple_sapling_invalidity(CMutableTransaction& tx) CMutableTransaction newTx(tx); CValidationState state; - BOOST_CHECK(!CheckTransaction(newTx, false, state, false)); + BOOST_CHECK(!CheckTransaction(newTx, false, state, false, false)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-vin-empty"); } { @@ -67,7 +67,7 @@ void test_simple_sapling_invalidity(CMutableTransaction& tx) newTx.sapData->vShieldedSpend.emplace_back(); newTx.sapData->vShieldedSpend[0].nullifier = GetRandHash(); - BOOST_CHECK(!CheckTransaction(newTx, false, state, false)); + BOOST_CHECK(!CheckTransaction(newTx, false, state, false, false)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-vout-empty"); } { @@ -104,7 +104,7 @@ void test_simple_sapling_invalidity(CMutableTransaction& tx) newTx.sapData->vShieldedSpend.emplace_back(); - BOOST_CHECK(!CheckTransaction(newTx, false, state, false, false, true)); + BOOST_CHECK(!CheckTransaction(newTx, false, state, false, false)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-invalid-sapling"); } { @@ -123,7 +123,7 @@ void test_simple_sapling_invalidity(CMutableTransaction& tx) newTx.sapData->vShieldedSpend.emplace_back(); - BOOST_CHECK(!CheckTransaction(newTx, false, state, false, false, true)); + BOOST_CHECK(!CheckTransaction(newTx, false, state, false, false)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-invalid-sapling"); } } diff --git a/src/validation.cpp b/src/validation.cpp index 259db8be0853..e07dbb2e4a17 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -405,7 +405,6 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C const CChainParams& params = Params(); const Consensus::Params& consensus = params.GetConsensus(); int chainHeight = chainActive.Height(); - bool fSaplingActive = consensus.NetworkUpgradeActive(chainHeight, Consensus::UPGRADE_V5_0); // Zerocoin txes are not longer accepted in the mempool. if (hasTxZerocoins) { @@ -415,7 +414,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C // Check transaction bool fColdStakingActive = !sporkManager.IsSporkActive(SPORK_19_COLDSTAKING_MAINTENANCE); if (!CheckTransaction(tx, consensus.NetworkUpgradeActive(chainHeight, Consensus::UPGRADE_ZC), - state, isBlockBetweenFakeSerialAttackRange(chainHeight), fColdStakingActive, fSaplingActive)) + state, isBlockBetweenFakeSerialAttackRange(chainHeight), fColdStakingActive)) return error("%s : transaction checks for %s failed with %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state)); // Sapling @@ -2836,11 +2835,11 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo } // Check transactions + bool fSaplingActive = consensus.NetworkUpgradeActive(blockHeight, Consensus::UPGRADE_V5_0); std::vector vBlockSerials; // TODO: Check if this is ok... blockHeight is always the tip or should we look for the prevHash and get the height? int blockHeight = chainActive.Height() + 1; const Consensus::Params& consensus = Params().GetConsensus(); - bool fSaplingActive = consensus.NetworkUpgradeActive(blockHeight, Consensus::UPGRADE_V5_0); for (const auto& txIn : block.vtx) { const CTransaction& tx = *txIn; if (!CheckTransaction( @@ -2848,8 +2847,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo fZerocoinActive, state, isBlockBetweenFakeSerialAttackRange(blockHeight), - fColdStakingActive, - fSaplingActive + fColdStakingActive )) return state.Invalid(false, state.GetRejectCode(), state.GetRejectReason(), strprintf("Transaction check failed (tx hash %s) %s", tx.GetHash().ToString(), state.GetDebugMessage())); diff --git a/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp b/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp index 161aa00e026d..25a9b2790566 100644 --- a/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp +++ b/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp @@ -53,7 +53,7 @@ SaplingOperation createOperationAndBuildTx(std::vector recipi CValidationState state; BOOST_ASSERT_MSG( - CheckTransaction(operation.getFinalTx(), true, state, true, false, true), + CheckTransaction(operation.getFinalTx(), true, state, false, true), "Invalid Sapling transaction"); return operation; } From bfd3fe47ac44504e246cd77f306e3a0ec859ff16 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Mon, 8 Feb 2021 19:31:45 +0100 Subject: [PATCH 12/17] [Refactoring] unify contextual checks for txes --- src/consensus/tx_verify.cpp | 13 +++++++++++++ src/consensus/tx_verify.h | 3 +++ src/validation.cpp | 11 +++++------ 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index b289d39c4e07..e7db37e5cca1 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -169,3 +169,16 @@ bool CheckTransaction(const CTransaction& tx, bool fZerocoinActive, CValidationS return true; } + +bool ContextualCheckTransaction(const CTransactionRef& tx, CValidationState& state, const CChainParams& chainparams, int nHeight, bool isMined, bool fIBD) +{ + // Dispatch to Sapling validator + if (!SaplingValidation::ContextualCheckTransaction(*tx, state, chainparams, nHeight, isMined, fIBD)) { + return false; // Failure reason has been set in validation state object + } + + // Dispatch to ZerocoinTx validator + // !TODO + + return true; +} diff --git a/src/consensus/tx_verify.h b/src/consensus/tx_verify.h index 65bcd1950678..47327a6c617b 100644 --- a/src/consensus/tx_verify.h +++ b/src/consensus/tx_verify.h @@ -11,6 +11,7 @@ #include class CBlockIndex; +class CChainParams; class CCoinsViewCache; class CValidationState; @@ -18,6 +19,8 @@ class CValidationState; /** Context-independent validity checks */ bool CheckTransaction(const CTransaction& tx, bool fZerocoinActive, CValidationState& state, bool fFakeSerialAttack, bool fColdStakingActive); +/** Context-dependent validity checks */ +bool ContextualCheckTransaction(const CTransactionRef& tx, CValidationState& state, const CChainParams& chainparams, int nHeight, bool isMined, bool fIBD); /** * Count ECDSA signature operations the old-fashioned (pre-0.6) way diff --git a/src/validation.cpp b/src/validation.cpp index e07dbb2e4a17..ed9b132d1af6 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -417,10 +417,9 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C state, isBlockBetweenFakeSerialAttackRange(chainHeight), fColdStakingActive)) return error("%s : transaction checks for %s failed with %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state)); - // Sapling int nextBlockHeight = chainHeight + 1; - // Check transaction contextually against the set of consensus rules which apply in the next block to be mined. - if (!SaplingValidation::ContextualCheckTransaction(tx, state, params, nextBlockHeight, false, IsInitialBlockDownload())) { + // Check transaction contextually against consensus rules at block height + if (!ContextualCheckTransaction(_tx, state, params, nextBlockHeight, false /* isMined */, IsInitialBlockDownload())) { return error("AcceptToMemoryPool: ContextualCheckTransaction failed"); } @@ -3012,9 +3011,9 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIn // Check that all transactions are finalized for (const auto& tx : block.vtx) { - // Sapling: Check transaction contextually against consensus rules at block height - if (!SaplingValidation::ContextualCheckTransaction(*tx, state, chainparams, nHeight, true, IsInitialBlockDownload())) { - return false; // Failure reason has been set in validation state object + // Check transaction contextually against consensus rules at block height + if (!ContextualCheckTransaction(tx, state, chainparams, nHeight, true /* isMined */, IsInitialBlockDownload())) { + return false; } if (!IsFinalTx(tx, nHeight, block.GetBlockTime())) { From 63a87f918ddcf3289ad10860b98b31650373e241 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Mon, 8 Feb 2021 22:59:19 +0100 Subject: [PATCH 13/17] [Refactoring] move zerocoin contextual checks out of CheckTransaction --- src/chainparams.cpp | 3 + src/consensus/params.h | 1 + src/consensus/tx_verify.cpp | 47 ++------------ src/consensus/tx_verify.h | 2 +- src/consensus/zerocoin_verify.cpp | 62 ++++++++++++++++++- src/consensus/zerocoin_verify.h | 3 +- src/qt/walletmodel.cpp | 2 +- src/test/sighash_tests.cpp | 2 +- src/test/transaction_tests.cpp | 8 +-- src/test/validation_tests.cpp | 8 +-- src/validation.cpp | 21 ++----- ...sapling_transactions_validations_tests.cpp | 2 +- 12 files changed, 90 insertions(+), 71 deletions(-) diff --git a/src/chainparams.cpp b/src/chainparams.cpp index 78e3a83711e2..c31665b595a5 100644 --- a/src/chainparams.cpp +++ b/src/chainparams.cpp @@ -183,6 +183,7 @@ class CMainParams : public CChainParams consensus.ZC_MinMintFee = 1 * CENT; consensus.ZC_MinStakeDepth = 200; consensus.ZC_TimeStart = 1508214600; // October 17, 2017 4:30:00 AM + consensus.ZC_HeightStart = 863735; // Network upgrades consensus.vUpgrades[Consensus::BASE_NETWORK].nActivationHeight = @@ -302,6 +303,7 @@ class CTestNetParams : public CChainParams consensus.height_last_ZC_AccumCheckpoint = -1; consensus.height_last_ZC_WrappedSerials = -1; consensus.height_ZC_RecalcAccumulators = 999999999; + consensus.ZC_HeightStart = 0; // Zerocoin-related params consensus.ZC_Modulus = "25195908475657893494027183240048398571429282126204032027777137836043662020707595556264018525880784" @@ -441,6 +443,7 @@ class CRegTestParams : public CChainParams consensus.ZC_MinMintFee = 1 * CENT; consensus.ZC_MinStakeDepth = 10; consensus.ZC_TimeStart = 0; // not implemented on regtest + consensus.ZC_HeightStart = 0; // Network upgrades consensus.vUpgrades[Consensus::BASE_NETWORK].nActivationHeight = diff --git a/src/consensus/params.h b/src/consensus/params.h index 7dfe60aaffc3..b57d7a687486 100644 --- a/src/consensus/params.h +++ b/src/consensus/params.h @@ -169,6 +169,7 @@ struct Params { CAmount ZC_MinMintFee; int ZC_MinStakeDepth; int ZC_TimeStart; + int ZC_HeightStart; libzerocoin::ZerocoinParams* Zerocoin_Params(bool useModulusV1) const { diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index e7db37e5cca1..3468879a9898 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -52,7 +52,7 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& in return nSigOps; } -bool CheckTransaction(const CTransaction& tx, bool fZerocoinActive, CValidationState& state, bool fFakeSerialAttack, bool fColdStakingActive) +bool CheckTransaction(const CTransaction& tx, CValidationState& state, bool fColdStakingActive) { // Basic checks that don't depend on any context // Transactions containing empty `vin` must have non-empty `vShieldedSpend`. @@ -110,60 +110,21 @@ bool CheckTransaction(const CTransaction& tx, bool fZerocoinActive, CValidationS } std::set vInOutPoints; - std::set vZerocoinSpendSerials; - int nZCSpendCount = 0; - for (const CTxIn& txin : tx.vin) { // Check for duplicate inputs if (vInOutPoints.count(txin.prevout)) return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputs-duplicate"); - - //duplicate zcspend serials are checked in CheckZerocoinSpend() if (!txin.IsZerocoinSpend()) { vInOutPoints.insert(txin.prevout); - } else if (!txin.IsZerocoinPublicSpend()) { - nZCSpendCount++; - } - } - - if (fZerocoinActive) { - if (nZCSpendCount > consensus.ZC_MaxSpendsPerTx) - return state.DoS(100, error("CheckTransaction() : there are more zerocoin spends than are allowed in one transaction")); - - //require that a zerocoinspend only has inputs that are zerocoins - if (tx.HasZerocoinSpendInputs()) { - for (const CTxIn& in : tx.vin) { - if (!in.IsZerocoinSpend() && !in.IsZerocoinPublicSpend()) - return state.DoS(100, - error("CheckTransaction() : zerocoinspend contains inputs that are not zerocoins")); - } - - // Do not require signature verification if this is initial sync and a block over 24 hours old - bool fVerifySignature = !IsInitialBlockDownload() && (GetTime() - chainActive.Tip()->GetBlockTime() < (60*60*24)); - if (!CheckZerocoinSpend(tx, fVerifySignature, state, fFakeSerialAttack)) - return state.DoS(100, error("CheckTransaction() : invalid zerocoin spend")); } } if (tx.IsCoinBase()) { if (tx.vin[0].scriptSig.size() < 2 || tx.vin[0].scriptSig.size() > 150) return state.DoS(100, false, REJECT_INVALID, "bad-cb-length"); - } else if (fZerocoinActive && tx.HasZerocoinSpendInputs()) { - if (tx.vin.size() < 1) - return state.DoS(10, false, REJECT_INVALID, "bad-zc-spend-min-inputs"); - if (tx.HasZerocoinPublicSpendInputs()) { - // tx has public zerocoin spend inputs - if(static_cast(tx.vin.size()) > consensus.ZC_MaxPublicSpendsPerTx) - return state.DoS(10, false, REJECT_INVALID, "bad-zc-spend-max-inputs"); - } else { - // tx has regular zerocoin spend inputs - if(static_cast(tx.vin.size()) > consensus.ZC_MaxSpendsPerTx) - return state.DoS(10, false, REJECT_INVALID, "bad-zc-spend-max-inputs"); - } - } else { for (const CTxIn& txin : tx.vin) - if (txin.prevout.IsNull() && (fZerocoinActive && !txin.IsZerocoinSpend())) + if (txin.prevout.IsNull() && !txin.IsZerocoinSpend()) return state.DoS(10, false, REJECT_INVALID, "bad-txns-prevout-null"); } @@ -178,7 +139,9 @@ bool ContextualCheckTransaction(const CTransactionRef& tx, CValidationState& sta } // Dispatch to ZerocoinTx validator - // !TODO + if (!ContextualCheckZerocoinTx(tx, state, chainparams.GetConsensus(), nHeight)) { + return false; // Failure reason has been set in validation state object + } return true; } diff --git a/src/consensus/tx_verify.h b/src/consensus/tx_verify.h index 47327a6c617b..82bef304eaea 100644 --- a/src/consensus/tx_verify.h +++ b/src/consensus/tx_verify.h @@ -18,7 +18,7 @@ class CValidationState; /** Transaction validation functions */ /** Context-independent validity checks */ -bool CheckTransaction(const CTransaction& tx, bool fZerocoinActive, CValidationState& state, bool fFakeSerialAttack, bool fColdStakingActive); +bool CheckTransaction(const CTransaction& tx, CValidationState& state, bool fColdStakingActive); /** Context-dependent validity checks */ bool ContextualCheckTransaction(const CTransactionRef& tx, CValidationState& state, const CChainParams& chainparams, int nHeight, bool isMined, bool fIBD); diff --git a/src/consensus/zerocoin_verify.cpp b/src/consensus/zerocoin_verify.cpp index df84490c881b..a0e7fb762b22 100644 --- a/src/consensus/zerocoin_verify.cpp +++ b/src/consensus/zerocoin_verify.cpp @@ -18,8 +18,9 @@ #include "zpiv/zpivmodule.h" -bool CheckZerocoinSpend(const CTransaction& tx, bool fVerifySignature, CValidationState& state, bool fFakeSerialAttack) +bool CheckZerocoinSpend(const CTransactionRef _tx, bool fVerifySignature, CValidationState& state, bool fFakeSerialAttack) { + const CTransaction& tx = *_tx; //max needed non-mint outputs should be 2 - one for redemption address and a possible 2nd for change if (tx.vout.size() > 2) { int outs = 0; @@ -135,6 +136,65 @@ bool CheckPublicCoinSpendVersion(int version) { return version == CurrentPublicCoinSpendVersion(); } +bool ContextualCheckZerocoinTx(const CTransactionRef& tx, CValidationState& state, const Consensus::Params& consensus, int nHeight) +{ + // zerocoin enforced via block time. First block with a zc mint is 863735 + const bool fZerocoinEnforced = (nHeight >= consensus.ZC_HeightStart); + const bool fPublicSpendEnforced = consensus.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_ZC_PUBLIC); + const bool fRejectMintsAndPrivateSpends = !fZerocoinEnforced || fPublicSpendEnforced; + const bool fRejectPublicSpends = !fPublicSpendEnforced || consensus.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_V5_0); + + const bool hasPrivateSpendInputs = !tx->vin.empty() && tx->vin[0].IsZerocoinSpend(); + const bool hasPublicSpendInputs = !tx->vin.empty() && tx->vin[0].IsZerocoinPublicSpend(); + const std::string& txId = tx->GetHash().ToString(); + + int nSpendCount{0}; + for (const CTxIn& in : tx->vin) { + if (in.IsZerocoinSpend()) { + if (fRejectMintsAndPrivateSpends) + return state.DoS(100, error("%s: zerocoin spend tx %s not accepted at height %d", + __func__, txId, nHeight), REJECT_INVALID, "bad-txns-zc-private-spend"); + if (!hasPrivateSpendInputs) + return state.DoS(100, error("%s: zerocoin spend tx %s has mixed spend inputs", + __func__, txId), REJECT_INVALID, "bad-txns-zc-private-spend-mixed-types"); + if (++nSpendCount > consensus.ZC_MaxSpendsPerTx) + return state.DoS(100, error("%s: zerocoin spend tx %s has more than %d inputs", + __func__, txId, consensus.ZC_MaxSpendsPerTx), REJECT_INVALID, "bad-txns-zc-private-spend-max-inputs"); + + } else if (in.IsZerocoinPublicSpend()) { + if (fRejectPublicSpends) + return state.DoS(100, error("%s: zerocoin public spend tx %s not accepted at height %d", + __func__, txId, nHeight), REJECT_INVALID, "bad-txns-zc-public-spend"); + if (!hasPublicSpendInputs) + return state.DoS(100, error("%s: zerocoin spend tx %s has mixed spend inputs", + __func__, txId), REJECT_INVALID, "bad-txns-zc-public-spend-mixed-types"); + if (++nSpendCount > consensus.ZC_MaxPublicSpendsPerTx) + return state.DoS(100, error("%s: zerocoin spend tx %s has more than %d inputs", + __func__, txId, consensus.ZC_MaxPublicSpendsPerTx), REJECT_INVALID, "bad-txns-zc-public-spend-max-inputs"); + + } else { + // this is a transparent input + if (hasPrivateSpendInputs || hasPublicSpendInputs) + return state.DoS(100, error("%s: zerocoin spend tx %s has mixed spend inputs", + __func__, txId), REJECT_INVALID, "bad-txns-zc-spend-mixed-types"); + } + } + + if (hasPrivateSpendInputs || hasPublicSpendInputs) { + if (!CheckZerocoinSpend(tx, false, state, false)) + return false; // failure reason logged in validation state + } + + for (const CTxOut& o : tx->vout) { + if (o.IsZerocoinMint() && fRejectMintsAndPrivateSpends) { + return state.DoS(100, error("%s: zerocoin mint tx %s not accepted at height %d", + __func__, txId, nHeight), REJECT_INVALID, "bad-txns-zc-mint"); + } + } + + return true; +} + bool ContextualCheckZerocoinSpend(const CTransaction& tx, const libzerocoin::CoinSpend* spend, int nHeight, const uint256& hashBlock) { if(!ContextualCheckZerocoinSpendNoSerialCheck(tx, spend, nHeight, hashBlock)){ diff --git a/src/consensus/zerocoin_verify.h b/src/consensus/zerocoin_verify.h index 3cc19277f38c..76fae8f6a7e6 100644 --- a/src/consensus/zerocoin_verify.h +++ b/src/consensus/zerocoin_verify.h @@ -10,13 +10,14 @@ #include "zpivchain.h" /** Context-independent validity checks */ -bool CheckZerocoinSpend(const CTransaction& tx, bool fVerifySignature, CValidationState& state, bool fFakeSerialAttack = false); +bool CheckZerocoinSpend(const CTransactionRef _tx, bool fVerifySignature, CValidationState& state, bool fFakeSerialAttack = false); // Fake Serial attack Range bool isBlockBetweenFakeSerialAttackRange(int nHeight); // Public coin spend bool CheckPublicCoinSpendEnforced(int blockHeight, bool isPublicSpend); int CurrentPublicCoinSpendVersion(); bool CheckPublicCoinSpendVersion(int version); +bool ContextualCheckZerocoinTx(const CTransactionRef& tx, CValidationState& state, const Consensus::Params& consensus, int nHeight); bool ContextualCheckZerocoinSpend(const CTransaction& tx, const libzerocoin::CoinSpend* spend, int nHeight, const uint256& hashBlock); bool ContextualCheckZerocoinSpendNoSerialCheck(const CTransaction& tx, const libzerocoin::CoinSpend* spend, int nHeight, const uint256& hashBlock); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 6f69425a77af..636c74fb7958 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -520,7 +520,7 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction& tran // Double check the tx before doing anything CTransactionRef& newTx = transaction.getTransaction(); CValidationState state; - if (!CheckTransaction(*newTx, true, state, true, fColdStakingActive)) { + if (!CheckTransaction(*newTx, state, fColdStakingActive)) { return TransactionCheckFailed; } diff --git a/src/test/sighash_tests.cpp b/src/test/sighash_tests.cpp index 537fed3caef8..79ebc1aaad1a 100644 --- a/src/test/sighash_tests.cpp +++ b/src/test/sighash_tests.cpp @@ -228,7 +228,7 @@ BOOST_AUTO_TEST_CASE(sighash_from_data) stream >> tx; CValidationState state; - BOOST_CHECK_MESSAGE(CheckTransaction(*tx, false, state, false, false), strTest); + BOOST_CHECK_MESSAGE(CheckTransaction(*tx, state, false), strTest); BOOST_CHECK(state.IsValid()); std::vector raw = ParseHex(raw_script); diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index d396a20e929c..57c719336cdf 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -139,7 +139,7 @@ BOOST_AUTO_TEST_CASE(tx_valid) CTransaction tx(deserialize, stream); CValidationState state; - BOOST_CHECK_MESSAGE(CheckTransaction(tx, false, state, false, false), strTest); + BOOST_CHECK_MESSAGE(CheckTransaction(tx, state, false), strTest); BOOST_CHECK(state.IsValid()); PrecomputedTransactionData precomTxData(tx); @@ -215,7 +215,7 @@ BOOST_AUTO_TEST_CASE(tx_invalid) CTransaction tx(deserialize, stream); CValidationState state; - fValid = CheckTransaction(tx, false, state, false, false) && state.IsValid(); + fValid = CheckTransaction(tx, state, false) && state.IsValid(); PrecomputedTransactionData precomTxData(tx); for (unsigned int i = 0; i < tx.vin.size() && fValid; i++) @@ -246,11 +246,11 @@ BOOST_AUTO_TEST_CASE(basic_transaction_tests) CMutableTransaction tx; stream >> tx; CValidationState state; - BOOST_CHECK_MESSAGE(CheckTransaction(tx, false, state, false, false) && state.IsValid(), "Simple deserialized transaction should be valid."); + BOOST_CHECK_MESSAGE(CheckTransaction(tx, state, false) && state.IsValid(), "Simple deserialized transaction should be valid."); // Check that duplicate txins fail tx.vin.push_back(tx.vin[0]); - BOOST_CHECK_MESSAGE(!CheckTransaction(tx, false, state, false, false) || !state.IsValid(), "Transaction with duplicate txins should be invalid."); + BOOST_CHECK_MESSAGE(!CheckTransaction(tx, state, false) || !state.IsValid(), "Transaction with duplicate txins should be invalid."); } // diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp index b1cad33e339b..cfbb0a4492d1 100644 --- a/src/test/validation_tests.cpp +++ b/src/test/validation_tests.cpp @@ -57,7 +57,7 @@ void test_simple_sapling_invalidity(CMutableTransaction& tx) CMutableTransaction newTx(tx); CValidationState state; - BOOST_CHECK(!CheckTransaction(newTx, false, state, false, false)); + BOOST_CHECK(!CheckTransaction(newTx, state, false)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-vin-empty"); } { @@ -67,7 +67,7 @@ void test_simple_sapling_invalidity(CMutableTransaction& tx) newTx.sapData->vShieldedSpend.emplace_back(); newTx.sapData->vShieldedSpend[0].nullifier = GetRandHash(); - BOOST_CHECK(!CheckTransaction(newTx, false, state, false, false)); + BOOST_CHECK(!CheckTransaction(newTx, state, false)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-vout-empty"); } { @@ -104,7 +104,7 @@ void test_simple_sapling_invalidity(CMutableTransaction& tx) newTx.sapData->vShieldedSpend.emplace_back(); - BOOST_CHECK(!CheckTransaction(newTx, false, state, false, false)); + BOOST_CHECK(!CheckTransaction(newTx, state, false)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-invalid-sapling"); } { @@ -123,7 +123,7 @@ void test_simple_sapling_invalidity(CMutableTransaction& tx) newTx.sapData->vShieldedSpend.emplace_back(); - BOOST_CHECK(!CheckTransaction(newTx, false, state, false, false)); + BOOST_CHECK(!CheckTransaction(newTx, state, false)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-invalid-sapling"); } } diff --git a/src/validation.cpp b/src/validation.cpp index ed9b132d1af6..c774f4863cfe 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -413,8 +413,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C // Check transaction bool fColdStakingActive = !sporkManager.IsSporkActive(SPORK_19_COLDSTAKING_MAINTENANCE); - if (!CheckTransaction(tx, consensus.NetworkUpgradeActive(chainHeight, Consensus::UPGRADE_ZC), - state, isBlockBetweenFakeSerialAttackRange(chainHeight), fColdStakingActive)) + if (!CheckTransaction(tx, state, fColdStakingActive)) return error("%s : transaction checks for %s failed with %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state)); int nextBlockHeight = chainHeight + 1; @@ -2834,24 +2833,16 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo } // Check transactions - bool fSaplingActive = consensus.NetworkUpgradeActive(blockHeight, Consensus::UPGRADE_V5_0); + bool fSaplingActive = Params().GetConsensus().NetworkUpgradeActive(nHeight, Consensus::UPGRADE_V5_0); std::vector vBlockSerials; - // TODO: Check if this is ok... blockHeight is always the tip or should we look for the prevHash and get the height? - int blockHeight = chainActive.Height() + 1; - const Consensus::Params& consensus = Params().GetConsensus(); for (const auto& txIn : block.vtx) { const CTransaction& tx = *txIn; - if (!CheckTransaction( - tx, - fZerocoinActive, - state, - isBlockBetweenFakeSerialAttackRange(blockHeight), - fColdStakingActive - )) + if (!CheckTransaction(tx, state, fColdStakingActive)) { return state.Invalid(false, state.GetRejectCode(), state.GetRejectReason(), - strprintf("Transaction check failed (tx hash %s) %s", tx.GetHash().ToString(), state.GetDebugMessage())); + strprintf("Transaction check failed (tx hash %s) %s", tx.GetHash().ToString(), state.GetDebugMessage())); + } - // No need to check for zerocoin anymore, they are networkely disabled + // No need to check for zerocoin anymore after sapling, they are networkely disabled // and checkpoints are preventing the chain for any massive reorganization. if (fSaplingActive && tx.ContainsZerocoins()) { return state.DoS(100, error("%s : v5 upgrade enforced, zerocoin disabled", __func__)); diff --git a/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp b/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp index 25a9b2790566..80262b645969 100644 --- a/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp +++ b/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp @@ -53,7 +53,7 @@ SaplingOperation createOperationAndBuildTx(std::vector recipi CValidationState state; BOOST_ASSERT_MSG( - CheckTransaction(operation.getFinalTx(), true, state, false, true), + CheckTransaction(operation.getFinalTx(), state, true), "Invalid Sapling transaction"); return operation; } From eba2377fe4bfa2fee097def87be6b2e6ce1650c9 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Mon, 8 Mar 2021 02:53:53 +0100 Subject: [PATCH 14/17] [Refactoring] Move CheckSpecialTx out of CheckTransaction Since we will need the current tip index in CheckProRegTx to check for duplicate unique-properties in the deterministic mn list. --- src/consensus/tx_verify.cpp | 5 ----- src/evo/specialtx.cpp | 21 ++++++++++++++++++--- src/evo/specialtx.h | 6 +++--- src/test/validation_tests.cpp | 12 ++++++------ 4 files changed, 27 insertions(+), 17 deletions(-) diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index 3468879a9898..f8d9d0f4115c 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -83,11 +83,6 @@ bool CheckTransaction(const CTransaction& tx, CValidationState& state, bool fCol return false; } - // Dispatch to SpecialTx validator - if (!CheckSpecialTx(tx, state)) { - return false; - } - // Check for negative or overflow output values const Consensus::Params& consensus = Params().GetConsensus(); for (const CTxOut& txout : tx.vout) { diff --git a/src/evo/specialtx.cpp b/src/evo/specialtx.cpp index d0960905ed80..2f6070e4b4da 100644 --- a/src/evo/specialtx.cpp +++ b/src/evo/specialtx.cpp @@ -12,7 +12,7 @@ #include "consensus/validation.h" #include "primitives/block.h" -bool CheckSpecialTx(const CTransaction& tx, CValidationState& state) +bool CheckSpecialTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state) { bool hasExtraPayload = tx.hasExtraPayload(); @@ -33,6 +33,14 @@ bool CheckSpecialTx(const CTransaction& tx, CValidationState& state) // --- From here on, tx has nVersion>=2 and nType!=0 + // !TODO: Add enforcement-height check + + // Cannot be coinbase/coinstake tx + if (tx.IsCoinBase() || tx.IsCoinStake()) { + return state.DoS(10, error("%s: Special tx is coinbase or coinstake", __func__), + REJECT_INVALID, "bad-txns-special-coinbase"); + } + // Special txes must have a non-empty payload if (!hasExtraPayload) { return state.DoS(100, error("%s: Special tx (type=%d) without extra payload", __func__, tx.nType), @@ -53,9 +61,16 @@ bool CheckSpecialTx(const CTransaction& tx, CValidationState& state) __func__, tx.GetHash().ToString(), tx.nType), REJECT_INVALID, "bad-tx-type"); } -bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindexPrev, CValidationState& state) +bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, CValidationState& state) { - /* process special txes in batches */ + // check special txes + for (const CTransactionRef& tx: block.vtx) { + if (!CheckSpecialTx(*tx, pindex->pprev, state)) { + // pass the state returned by the function above + return false; + } + } + // !TODO: Process batch of special txes in deterministic manager return true; } diff --git a/src/evo/specialtx.h b/src/evo/specialtx.h index 58b8fdce6218..e91aa54d702b 100644 --- a/src/evo/specialtx.h +++ b/src/evo/specialtx.h @@ -18,13 +18,13 @@ class uint256; /** The maximum allowed size of the extraPayload (for any TxType) */ static const unsigned int MAX_SPECIALTX_EXTRAPAYLOAD = 10000; -/** Context-independent validity checks */ +/** Payload validity checks (including duplicate unique properties against list at pindexPrev)*/ // Note: for +v2, if the tx is not a special tx, this method returns true. // Note2: This function only performs extra payload related checks, it does NOT checks regular inputs and outputs. -bool CheckSpecialTx(const CTransaction& tx, CValidationState& state); +bool CheckSpecialTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state); // Update internal tiertwo data when blocks containing special txes get connected/disconnected -bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindexPrev, CValidationState& state); +bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, CValidationState& state); bool UndoSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindexPrev); #endif // PIVX_SPECIALTX_H diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp index cfbb0a4492d1..d47c46ad7643 100644 --- a/src/test/validation_tests.cpp +++ b/src/test/validation_tests.cpp @@ -20,33 +20,33 @@ BOOST_AUTO_TEST_CASE(special_tx_validation_test) // v1 can only be Type=0 mtx.nType = 1; mtx.nVersion = CTransaction::TxVersion::LEGACY; - BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), state)); + BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), nullptr, state)); BOOST_CHECK(state.GetRejectReason().find("not supported with version 0")); // version >= Sapling, type = 0, payload != null. mtx.nType = CTransaction::TxType::NORMAL; mtx.extraPayload = std::vector(10, 1); mtx.nVersion = CTransaction::TxVersion::SAPLING; - BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), state)); + BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), nullptr, state)); BOOST_CHECK(state.GetRejectReason().find("doesn't support extra payload")); // version >= Sapling, type = 0, payload == null --> pass mtx.extraPayload = nullopt; - BOOST_CHECK(CheckSpecialTx(CTransaction(mtx), state)); + BOOST_CHECK(CheckSpecialTx(CTransaction(mtx), nullptr, state)); // nVersion>=2 and nType!=0 without extrapayload mtx.nType = 1; - BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), state)); + BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), nullptr, state)); BOOST_CHECK(state.GetRejectReason().find("without extra payload")); // Size limits mtx.extraPayload = std::vector(MAX_SPECIALTX_EXTRAPAYLOAD + 1, 1); - BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), state)); + BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), nullptr, state)); BOOST_CHECK(state.GetRejectReason().find("Special tx payload oversize")); // Remove one element, so now it passes the size check mtx.extraPayload->pop_back(); - BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), state)); + BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), nullptr, state)); BOOST_CHECK(state.GetRejectReason().find("with invalid type")); } From 34882e65ea91d36b491a3a82090d29ee47ec68f0 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 23 Mar 2021 19:23:46 +0100 Subject: [PATCH 15/17] [Refactoring][Consensus] Connect SpecialTx processing to validation code the boolean argument fJustCheck in ProcessSpecialTxsInBlock will be used later, after the connection to the deterministic manager --- src/evo/specialtx.cpp | 2 +- src/evo/specialtx.h | 2 +- src/validation.cpp | 23 ++++++++++++++++------- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/evo/specialtx.cpp b/src/evo/specialtx.cpp index 2f6070e4b4da..782a7b6c5ffc 100644 --- a/src/evo/specialtx.cpp +++ b/src/evo/specialtx.cpp @@ -61,7 +61,7 @@ bool CheckSpecialTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVali __func__, tx.GetHash().ToString(), tx.nType), REJECT_INVALID, "bad-tx-type"); } -bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, CValidationState& state) +bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, CValidationState& state, bool fJustCheck) { // check special txes for (const CTransactionRef& tx: block.vtx) { diff --git a/src/evo/specialtx.h b/src/evo/specialtx.h index e91aa54d702b..6dc165d86361 100644 --- a/src/evo/specialtx.h +++ b/src/evo/specialtx.h @@ -24,7 +24,7 @@ static const unsigned int MAX_SPECIALTX_EXTRAPAYLOAD = 10000; bool CheckSpecialTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state); // Update internal tiertwo data when blocks containing special txes get connected/disconnected -bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, CValidationState& state); +bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, CValidationState& state, bool fJustCheck); bool UndoSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindexPrev); #endif // PIVX_SPECIALTX_H diff --git a/src/validation.cpp b/src/validation.cpp index c774f4863cfe..7070816ae2c6 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -22,6 +22,7 @@ #include "consensus/tx_verify.h" #include "consensus/validation.h" #include "consensus/zerocoin_verify.h" +#include "evo/specialtx.h" #include "fs.h" #include "guiinterface.h" #include "init.h" @@ -612,6 +613,11 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C return state.DoS(0, error("%s : %s", __func__, errString), REJECT_NONSTANDARD, "too-long-mempool-chain", false); } + if (!CheckSpecialTx(tx, chainActive.Tip(), state)) { + // pass the state returned by the function above + return false; + } + bool fCLTVIsActivated = consensus.NetworkUpgradeActive(chainHeight, Consensus::UPGRADE_BIP65); // Check against previous transactions @@ -1431,9 +1437,9 @@ void ThreadScriptCheck() } static int64_t nTimeVerify = 0; +static int64_t nTimeProcessSpecial = 0; static int64_t nTimeConnect = 0; static int64_t nTimeIndex = 0; -static int64_t nTimeCallbacks = 0; static int64_t nTimeTotal = 0; /** Apply the effects of this block (with given index) on the UTXO set represented by coins. @@ -1709,6 +1715,13 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd nTimeVerify += nTime2 - nTimeStart; LogPrint(BCLog::BENCH, " - Verify %u txins: %.2fms (%.3fms/txin) [%.2fs]\n", nInputs - 1, 0.001 * (nTime2 - nTimeStart), nInputs <= 1 ? 0 : 0.001 * (nTime2 - nTimeStart) / (nInputs - 1), nTimeVerify * 0.000001); + if (!ProcessSpecialTxsInBlock(block, pindex, state, fJustCheck)) { + return error("%s: Special tx processing failed with %s", __func__, FormatStateMessage(state)); + } + int64_t nTime3 = GetTimeMicros(); + nTimeProcessSpecial += nTime3 - nTime2; + LogPrint(BCLog::BENCH, " - Process special tx: %.2fms [%.2fs]\n", 0.001 * (nTime3 - nTime2), nTimeProcessSpecial * 0.000001); + //IMPORTANT NOTE: Nothing before this point should actually store to disk (or even memory) if (fJustCheck) return true; @@ -1744,13 +1757,9 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd // add this block to the view's block chain view.SetBestBlock(pindex->GetBlockHash()); - int64_t nTime3 = GetTimeMicros(); - nTimeIndex += nTime3 - nTime2; - LogPrint(BCLog::BENCH, " - Index writing: %.2fms [%.2fs]\n", 0.001 * (nTime3 - nTime2), nTimeIndex * 0.000001); - int64_t nTime4 = GetTimeMicros(); - nTimeCallbacks += nTime4 - nTime3; - LogPrint(BCLog::BENCH, " - Callbacks: %.2fms [%.2fs]\n", 0.001 * (nTime4 - nTime3), nTimeCallbacks * 0.000001); + nTimeIndex += nTime4 - nTime3; + LogPrint(BCLog::BENCH, " - Index writing: %.2fms [%.2fs]\n", 0.001 * (nTime4 - nTime3), nTimeIndex * 0.000001); if (consensus.NetworkUpgradeActive(pindex->nHeight, Consensus::UPGRADE_ZC_V2) && pindex->nHeight < consensus.height_last_ZC_AccumCheckpoint) { From 50a55b74f96a3f1ebb41d848e1ebb2afa55aeb81 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Wed, 7 Apr 2021 12:06:50 +0200 Subject: [PATCH 16/17] [Cleanup] Remove unused boolean arguments in CheckZerocoinSpend Also don't expose it (as it is only called by ContextualCheckZerocoinTx) and cleanup logs with __func__ --- src/consensus/zerocoin_verify.cpp | 24 ++++++++++++------------ src/consensus/zerocoin_verify.h | 2 -- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/consensus/zerocoin_verify.cpp b/src/consensus/zerocoin_verify.cpp index a0e7fb762b22..cd8053ae56c8 100644 --- a/src/consensus/zerocoin_verify.cpp +++ b/src/consensus/zerocoin_verify.cpp @@ -18,7 +18,7 @@ #include "zpiv/zpivmodule.h" -bool CheckZerocoinSpend(const CTransactionRef _tx, bool fVerifySignature, CValidationState& state, bool fFakeSerialAttack) +static bool CheckZerocoinSpend(const CTransactionRef _tx, CValidationState& state) { const CTransaction& tx = *_tx; //max needed non-mint outputs should be 2 - one for redemption address and a possible 2nd for change @@ -30,7 +30,7 @@ bool CheckZerocoinSpend(const CTransactionRef _tx, bool fVerifySignature, CValid outs++; } if (outs > 2 && !tx.IsCoinStake()) - return state.DoS(100, error("CheckZerocoinSpend(): over two non-mint outputs in a zerocoinspend transaction")); + return state.DoS(100, error("%s: over two non-mint outputs in a zerocoinspend transaction", __func__)); } //compute the txout hash that is used for the zerocoinspend signatures @@ -55,12 +55,12 @@ bool CheckZerocoinSpend(const CTransactionRef _tx, bool fVerifySignature, CValid CTxOut prevOut; if (isPublicSpend) { if(!GetOutput(txin.prevout.hash, txin.prevout.n, state, prevOut)){ - return state.DoS(100, error("CheckZerocoinSpend(): public zerocoin spend prev output not found, prevTx %s, index %d", txin.prevout.hash.GetHex(), txin.prevout.n)); + return state.DoS(100, error("%s: public zerocoin spend prev output not found, prevTx %s, index %d", __func__, txin.prevout.hash.GetHex(), txin.prevout.n)); } libzerocoin::ZerocoinParams* params = consensus.Zerocoin_Params(false); PublicCoinSpend publicSpend(params); if (!ZPIVModule::parseCoinSpend(txin, tx, prevOut, publicSpend)){ - return state.DoS(100, error("CheckZerocoinSpend(): public zerocoin spend parse failed")); + return state.DoS(100, error("%s: public zerocoin spend parse failed", __func__)); } newSpend = publicSpend; } else { @@ -69,26 +69,26 @@ bool CheckZerocoinSpend(const CTransactionRef _tx, bool fVerifySignature, CValid //check that the denomination is valid if (newSpend.getDenomination() == libzerocoin::ZQ_ERROR) - return state.DoS(100, error("Zerocoinspend does not have the correct denomination")); + return state.DoS(100, error("%s: Zerocoinspend does not have the correct denomination", __func__)); //check that denomination is what it claims to be in nSequence if (newSpend.getDenomination() != txin.nSequence) - return state.DoS(100, error("Zerocoinspend nSequence denomination does not match CoinSpend")); + return state.DoS(100, error("%s: Zerocoinspend nSequence denomination does not match CoinSpend", __func__)); //make sure the txout has not changed if (newSpend.getTxOutHash() != hashTxOut) - return state.DoS(100, error("Zerocoinspend does not use the same txout that was used in the SoK")); + return state.DoS(100, error("%s: Zerocoinspend does not use the same txout that was used in the SoK", __func__)); if (isPublicSpend) { libzerocoin::ZerocoinParams* params = consensus.Zerocoin_Params(false); PublicCoinSpend ret(params); if (!ZPIVModule::validateInput(txin, prevOut, tx, ret)){ - return state.DoS(100, error("CheckZerocoinSpend(): public zerocoin spend did not verify")); + return state.DoS(100, error("%s: public zerocoin spend did not verify", __func__)); } } if (serials.count(newSpend.getCoinSerialNumber())) - return state.DoS(100, error("Zerocoinspend serial is used twice in the same tx")); + return state.DoS(100, error("%s: Zerocoinspend serial is used twice in the same tx", __func__)); serials.insert(newSpend.getCoinSerialNumber()); //make sure that there is no over redemption of coins @@ -97,8 +97,8 @@ bool CheckZerocoinSpend(const CTransactionRef _tx, bool fVerifySignature, CValid } if (!tx.IsCoinStake() && nTotalRedeemed < tx.GetValueOut()) { - LogPrintf("redeemed = %s , spend = %s \n", FormatMoney(nTotalRedeemed), FormatMoney(tx.GetValueOut())); - return state.DoS(100, error("Transaction spend more than was redeemed in zerocoins")); + LogPrintf("%s: redeemed = %s , spend = %s \n", __func__, FormatMoney(nTotalRedeemed), FormatMoney(tx.GetValueOut())); + return state.DoS(100, error("%s: Transaction spend more than was redeemed in zerocoins", __func__)); } return fValidated; @@ -181,7 +181,7 @@ bool ContextualCheckZerocoinTx(const CTransactionRef& tx, CValidationState& stat } if (hasPrivateSpendInputs || hasPublicSpendInputs) { - if (!CheckZerocoinSpend(tx, false, state, false)) + if (!CheckZerocoinSpend(tx, state)) return false; // failure reason logged in validation state } diff --git a/src/consensus/zerocoin_verify.h b/src/consensus/zerocoin_verify.h index 76fae8f6a7e6..18906785be42 100644 --- a/src/consensus/zerocoin_verify.h +++ b/src/consensus/zerocoin_verify.h @@ -9,8 +9,6 @@ #include "script/interpreter.h" #include "zpivchain.h" -/** Context-independent validity checks */ -bool CheckZerocoinSpend(const CTransactionRef _tx, bool fVerifySignature, CValidationState& state, bool fFakeSerialAttack = false); // Fake Serial attack Range bool isBlockBetweenFakeSerialAttackRange(int nHeight); // Public coin spend From b408de3e30f12ed208275d1188b97cd43accc3d7 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Wed, 7 Apr 2021 12:39:31 +0200 Subject: [PATCH 17/17] [Refactoring] non-contextual checks for special txes in CheckBlock --- src/evo/specialtx.cpp | 48 ++++++++++++++++++++++++++--------- src/evo/specialtx.h | 4 +++ src/test/validation_tests.cpp | 12 ++++----- src/validation.cpp | 6 +++++ 4 files changed, 52 insertions(+), 18 deletions(-) diff --git a/src/evo/specialtx.cpp b/src/evo/specialtx.cpp index 782a7b6c5ffc..d9c82dc61680 100644 --- a/src/evo/specialtx.cpp +++ b/src/evo/specialtx.cpp @@ -12,16 +12,11 @@ #include "consensus/validation.h" #include "primitives/block.h" -bool CheckSpecialTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state) +bool CheckSpecialTxNoContext(const CTransaction& tx, CValidationState& state) { bool hasExtraPayload = tx.hasExtraPayload(); - // v1/v2 can only be Type=0 - if (!tx.isSaplingVersion() && tx.nType != CTransaction::TxType::NORMAL) { - return state.DoS(100, error("%s: Type %d not supported with version %d", __func__, tx.nType, tx.nVersion), - REJECT_INVALID, "bad-txns-type-version"); - } - if (tx.nType == CTransaction::TxType::NORMAL) { + if (tx.IsNormalType()) { // Type-0 txes don't have extra payload if (hasExtraPayload) { return state.DoS(100, error("%s: Type 0 doesn't support extra payload", __func__), @@ -31,9 +26,13 @@ bool CheckSpecialTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVali return true; } - // --- From here on, tx has nVersion>=2 and nType!=0 + // Special txes need at least version 2 + if (!tx.isSaplingVersion()) { + return state.DoS(100, error("%s: Type %d not supported with version %d", __func__, tx.nType, tx.nVersion), + REJECT_INVALID, "bad-txns-type-version"); + } - // !TODO: Add enforcement-height check + // --- From here on, tx has nVersion>=2 and nType!=0 // Cannot be coinbase/coinstake tx if (tx.IsCoinBase() || tx.IsCoinStake()) { @@ -54,11 +53,36 @@ bool CheckSpecialTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVali } switch (tx.nType) { - /* per-tx-type checking */ + /* per-tx-type non-contextual checking */ + } + + return state.DoS(10, error("%s: special tx %s with invalid type %d", __func__, tx.GetHash().ToString(), tx.nType), + REJECT_INVALID, "bad-tx-type"); +} + +bool CheckSpecialTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state) +{ + assert(pindexPrev != nullptr); + + if (!CheckSpecialTxNoContext(tx, state)) { + // pass the state returned by the function above + return false; + } + + if (tx.IsNormalType()) { + // nothing to check + return true; + } + + // !TODO: Add enforcement-height check + + switch (tx.nType) { + /* per-tx-type contextual checking */ } - return state.DoS(10, error("%s : special tx %s with invalid type %d", - __func__, tx.GetHash().ToString(), tx.nType), REJECT_INVALID, "bad-tx-type"); + // should never get here, as we already checked the type in CheckSpecialTxNoContext + return state.DoS(10, error("%s: special tx %s with invalid type %d", __func__, tx.GetHash().ToString(), tx.nType), + REJECT_INVALID, "bad-tx-type"); } bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, CValidationState& state, bool fJustCheck) diff --git a/src/evo/specialtx.h b/src/evo/specialtx.h index 6dc165d86361..6c8373043c35 100644 --- a/src/evo/specialtx.h +++ b/src/evo/specialtx.h @@ -23,6 +23,10 @@ static const unsigned int MAX_SPECIALTX_EXTRAPAYLOAD = 10000; // Note2: This function only performs extra payload related checks, it does NOT checks regular inputs and outputs. bool CheckSpecialTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state); +// Basic non-contextual checks for special txes +// Note: for +v2, if the tx is not a special tx, this method returns true. +bool CheckSpecialTxNoContext(const CTransaction& tx, CValidationState& state); + // Update internal tiertwo data when blocks containing special txes get connected/disconnected bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, CValidationState& state, bool fJustCheck); bool UndoSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindexPrev); diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp index d47c46ad7643..8ad91add541f 100644 --- a/src/test/validation_tests.cpp +++ b/src/test/validation_tests.cpp @@ -20,33 +20,33 @@ BOOST_AUTO_TEST_CASE(special_tx_validation_test) // v1 can only be Type=0 mtx.nType = 1; mtx.nVersion = CTransaction::TxVersion::LEGACY; - BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), nullptr, state)); + BOOST_CHECK(!CheckSpecialTxNoContext(CTransaction(mtx), state)); BOOST_CHECK(state.GetRejectReason().find("not supported with version 0")); // version >= Sapling, type = 0, payload != null. mtx.nType = CTransaction::TxType::NORMAL; mtx.extraPayload = std::vector(10, 1); mtx.nVersion = CTransaction::TxVersion::SAPLING; - BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), nullptr, state)); + BOOST_CHECK(!CheckSpecialTxNoContext(CTransaction(mtx), state)); BOOST_CHECK(state.GetRejectReason().find("doesn't support extra payload")); // version >= Sapling, type = 0, payload == null --> pass mtx.extraPayload = nullopt; - BOOST_CHECK(CheckSpecialTx(CTransaction(mtx), nullptr, state)); + BOOST_CHECK(CheckSpecialTxNoContext(CTransaction(mtx), state)); // nVersion>=2 and nType!=0 without extrapayload mtx.nType = 1; - BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), nullptr, state)); + BOOST_CHECK(!CheckSpecialTxNoContext(CTransaction(mtx), state)); BOOST_CHECK(state.GetRejectReason().find("without extra payload")); // Size limits mtx.extraPayload = std::vector(MAX_SPECIALTX_EXTRAPAYLOAD + 1, 1); - BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), nullptr, state)); + BOOST_CHECK(!CheckSpecialTxNoContext(CTransaction(mtx), state)); BOOST_CHECK(state.GetRejectReason().find("Special tx payload oversize")); // Remove one element, so now it passes the size check mtx.extraPayload->pop_back(); - BOOST_CHECK(!CheckSpecialTx(CTransaction(mtx), nullptr, state)); + BOOST_CHECK(!CheckSpecialTxNoContext(CTransaction(mtx), state)); BOOST_CHECK(state.GetRejectReason().find("with invalid type")); } diff --git a/src/validation.cpp b/src/validation.cpp index 7070816ae2c6..7a4cec641eeb 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2851,6 +2851,12 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo strprintf("Transaction check failed (tx hash %s) %s", tx.GetHash().ToString(), state.GetDebugMessage())); } + // Non-contextual checks for special txes + if (!CheckSpecialTxNoContext(tx, state)) { + // pass the state returned by the function above + return false; + } + // No need to check for zerocoin anymore after sapling, they are networkely disabled // and checkpoints are preventing the chain for any massive reorganization. if (fSaplingActive && tx.ContainsZerocoins()) {