From 2d421fce5d5f66c0b6cc5759d6f4a7d0dc7be18f Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Thu, 10 Dec 2020 10:26:57 +0000 Subject: [PATCH 1/8] [tests] Initial account mining test --- test/functional/feature_account_mining.py | 47 +++++++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 48 insertions(+) create mode 100755 test/functional/feature_account_mining.py diff --git a/test/functional/feature_account_mining.py b/test/functional/feature_account_mining.py new file mode 100755 index 0000000000..3d204f87cc --- /dev/null +++ b/test/functional/feature_account_mining.py @@ -0,0 +1,47 @@ +#!/usr/bin/env python3 +# Copyright (c) 2014-2019 The Bitcoin Core developers +# Copyright (c) DeFi Blockchain Developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test account mining behaviour""" + +from test_framework.test_framework import DefiTestFramework +from test_framework.util import assert_equal + +class AccountMiningTest(DefiTestFramework): + def set_test_params(self): + self.num_nodes = 1 + self.setup_clean_chain = True + self.extra_args = [['-txnotokens=0', '-amkheight=50']] + + def run_test(self): + node = self.nodes[0] + node.generate(101) + + # Get addresses and set up account + account = node.getnewaddress() + destination = node.getnewaddress() + node.utxostoaccount({account: "10@0"}) + node.generate(1) + + # Check we have expected balance + assert_equal(node.getaccount(account)[0], "10.00000000@DFI") + + # Send double the amount we have in account + node.accounttoutxos(account, {destination: "10@0"}) + node.accounttoutxos(account, {destination: "10@0"}) + + # Store block height + blockcount = node.getblockcount() + + # Generate a block + node.generate(1) + + # Check the blockchain has extended as expected + assert_equal(node.getblockcount(), blockcount + 1) + + # Account should now be empty + assert_equal(node.getaccount(account), []) + +if __name__ == '__main__': + AccountMiningTest().main () diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index a786c0f7e8..f05b97137c 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -229,6 +229,7 @@ 'p2p_permissions.py', 'feature_blocksdir.py', 'feature_config_args.py', + 'feature_account_mining.py', 'rpc_help.py', 'feature_help.py', 'feature_shutdown.py', From 6e674f5e403c2a61ec59d2b75121875a158de333 Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Fri, 11 Dec 2020 09:11:16 +0000 Subject: [PATCH 2/8] Remove conflicting custom TX from mempool after block creation --- src/txmempool.cpp | 27 +++++++++++++++++++++++++ test/lint/lint-circular-dependencies.sh | 1 + 2 files changed, 28 insertions(+) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 0a946ff4c4..014990550f 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -5,6 +5,7 @@ #include +#include #include #include #include @@ -577,6 +578,32 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne removeConflicts(*tx); ClearPrioritisation(tx->GetHash()); } + + // Check custom TX consensus types are now not in conflict with account layer + std::set txsToRemove; + for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); ++it) { + std::vector metadata; + CustomTxType txType = GuessCustomTxType(it->GetTx(), metadata); + if (NotAllowedToFail(txType)) { + auto res = ApplyCustomTx(*pcustomcsview, g_chainstate->CoinsTip(), it->GetTx(), Params().GetConsensus(), nBlockHeight, 0, true); + if (!res.ok && (res.code & CustomTxErrCodes::Fatal)) { + LogPrintf("%s: Remove custom TX: %s\n", __func__, res.msg); + txsToRemove.insert(it->GetSharedTx()); + } + } + } + + for (auto& tx : txsToRemove) { + txiter it = mapTx.find(tx->GetHash()); + if (it != mapTx.end()) { + setEntries stage; + stage.insert(it); + RemoveStaged(stage, true, MemPoolRemovalReason::CONFLICT); + } + removeConflicts(*tx); + ClearPrioritisation(tx->GetHash()); + } + lastRollingFeeUpdate = GetTime(); blockSinceLastRollingFeeBump = true; } diff --git a/test/lint/lint-circular-dependencies.sh b/test/lint/lint-circular-dependencies.sh index a94c21010b..9dffa4f0e8 100755 --- a/test/lint/lint-circular-dependencies.sh +++ b/test/lint/lint-circular-dependencies.sh @@ -49,6 +49,7 @@ EXPECTED_CIRCULAR_DEPENDENCIES=( "validation -> wallet/wallet -> validation" "policy/fees -> txmempool -> validation -> wallet/wallet -> policy/fees" "policy/fees -> txmempool -> validation -> wallet/wallet -> util/fees -> policy/fees" + "chainparams -> masternodes/mn_checks -> txmempool -> chainparams" ) EXIT_CODE=0 From f0aa70a4b6d6d517c196451a408a4d5f70dfb7d9 Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Fri, 11 Dec 2020 12:02:06 +0000 Subject: [PATCH 3/8] Validate NotAllowedToFail TX types in CreateNewblock --- src/masternodes/accountshistory.cpp | 5 +- src/masternodes/mn_checks.cpp | 14 ++--- src/masternodes/mn_checks.h | 6 +- src/miner.cpp | 71 ++++++++++++++++++++++- src/miner.h | 2 +- src/validation.cpp | 1 - test/functional/feature_account_mining.py | 8 ++- 7 files changed, 88 insertions(+), 19 deletions(-) diff --git a/src/masternodes/accountshistory.cpp b/src/masternodes/accountshistory.cpp index 92dc8abc96..849a5dbecf 100644 --- a/src/masternodes/accountshistory.cpp +++ b/src/masternodes/accountshistory.cpp @@ -6,6 +6,8 @@ #include #include +#include + /// @attention make sure that it does not overlap with those in masternodes.cpp/tokens.cpp/undos.cpp/accounts.cpp !!! const unsigned char CAccountsHistoryView::ByAccountHistoryKey::prefix = 'h'; // don't intersects with CMintedHeadersView::MintedHeaders::prefix due to different DB @@ -32,7 +34,8 @@ Res CAccountsHistoryView::SetAccountHistory(const CScript & owner, uint32_t heig } bool CAccountsHistoryView::TrackAffectedAccounts(CStorageKV const & before, MapKV const & diff, uint32_t height, uint32_t txn, const uint256 & txid, unsigned char category) { - if (!gArgs.GetBoolArg("-acindex", false)) + // txn set to max if called from CreateNewBlock to check account balances, do not track here. + if (!gArgs.GetBoolArg("-acindex", false) || txn == std::numeric_limits::max()) return false; std::map balancesDiff; diff --git a/src/masternodes/mn_checks.cpp b/src/masternodes/mn_checks.cpp index bc35fc139e..58b9dda8c2 100644 --- a/src/masternodes/mn_checks.cpp +++ b/src/masternodes/mn_checks.cpp @@ -113,7 +113,7 @@ bool HasFoundationAuth(CTransaction const & tx, CCoinsViewCache const & coins, C return false; } -Res ApplyCustomTx(CCustomCSView & base_mnview, CCoinsViewCache const & coins, CTransaction const & tx, Consensus::Params const & consensusParams, uint32_t height, uint32_t txn, bool isCheck) +Res ApplyCustomTx(CCustomCSView & base_mnview, CCoinsViewCache const & coins, CTransaction const & tx, Consensus::Params const & consensusParams, uint32_t height, uint32_t txn, bool isCheck, bool skipAuth) { Res res = Res::Ok(); @@ -145,7 +145,7 @@ Res ApplyCustomTx(CCustomCSView & base_mnview, CCoinsViewCache const & coins, CT res = ApplyUpdateTokenAnyTx(mnview, coins, tx, height, metadata, consensusParams); break; case CustomTxType::MintToken: - res = ApplyMintTokenTx(mnview, coins, tx, height, metadata, consensusParams); + res = ApplyMintTokenTx(mnview, coins, tx, height, metadata, consensusParams, skipAuth); break; case CustomTxType::CreatePoolPair: res = ApplyCreatePoolPairTx(mnview, coins, tx, height, metadata, consensusParams); @@ -166,7 +166,7 @@ Res ApplyCustomTx(CCustomCSView & base_mnview, CCoinsViewCache const & coins, CT res = ApplyUtxosToAccountTx(mnview, tx, height, metadata, consensusParams); break; case CustomTxType::AccountToUtxos: - res = ApplyAccountToUtxosTx(mnview, coins, tx, height, metadata, consensusParams); + res = ApplyAccountToUtxosTx(mnview, coins, tx, height, metadata, consensusParams, skipAuth); break; case CustomTxType::AccountToAccount: res = ApplyAccountToAccountTx(mnview, coins, tx, height, metadata, consensusParams); @@ -423,7 +423,7 @@ Res ApplyUpdateTokenAnyTx(CCustomCSView & mnview, CCoinsViewCache const & coins, return Res::Ok(base); } -Res ApplyMintTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams) +Res ApplyMintTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams, bool skipAuth) { if((int)height < consensusParams.AMKHeight) { return Res::Err("Token tx before AMK height (block %d)", consensusParams.AMKHeight); } @@ -473,7 +473,7 @@ Res ApplyMintTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTra return Res::Err("%s: token not mintable!", tokenId.ToString()); } - if (!HasAuth(tx, coins, auth.out.scriptPubKey)) { // in the case of DAT, it's ok to do not check foundation auth cause exact DAT owner is foundation member himself + if (!skipAuth && !HasAuth(tx, coins, auth.out.scriptPubKey)) { // in the case of DAT, it's ok to do not check foundation auth cause exact DAT owner is foundation member himself if (!tokenImpl.IsDAT()) return Res::Err("%s: %s", base, "tx must have at least one input from token owner"); @@ -696,7 +696,7 @@ Res ApplyUtxosToAccountTx(CCustomCSView & mnview, CTransaction const & tx, uint3 return Res::Ok(base); } -Res ApplyAccountToUtxosTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams) +Res ApplyAccountToUtxosTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams, bool skipAuth) { if((int)height < consensusParams.AMKHeight) { return Res::Err("Token tx before AMK height (block %d)", consensusParams.AMKHeight); } @@ -710,7 +710,7 @@ Res ApplyAccountToUtxosTx(CCustomCSView & mnview, CCoinsViewCache const & coins, const auto base = strprintf("Transfer AccountToUtxos: %s", msg.ToString()); // check auth - if (!HasAuth(tx, coins, msg.from)) { + if (!skipAuth && !HasAuth(tx, coins, msg.from)) { return Res::Err("%s: %s", base, "tx must have at least one input from account owner"); } // check that all tokens are minted, and no excess tokens are minted diff --git a/src/masternodes/mn_checks.h b/src/masternodes/mn_checks.h index abd7f1d7c4..8c1057249b 100644 --- a/src/masternodes/mn_checks.h +++ b/src/masternodes/mn_checks.h @@ -113,7 +113,7 @@ bool HasAuth(CTransaction const & tx, CKeyID const & auth); bool HasAuth(CTransaction const & tx, CCoinsViewCache const & coins, CScript const & auth); bool HasCollateralAuth(CTransaction const & tx, CCoinsViewCache const & coins, uint256 const & collateralTx); -Res ApplyCustomTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, const Consensus::Params& consensusParams, uint32_t height, uint32_t txn, bool isCheck = true); +Res ApplyCustomTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, const Consensus::Params& consensusParams, uint32_t height, uint32_t txn, bool isCheck = true, bool skipAuth = false); //! Deep check (and write) Res ApplyCreateMasternodeTx(CCustomCSView & mnview, CTransaction const & tx, uint32_t height, std::vector const & metadata); Res ApplyResignMasternodeTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata); @@ -121,7 +121,7 @@ Res ApplyResignMasternodeTx(CCustomCSView & mnview, CCoinsViewCache const & coin Res ApplyCreateTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams); Res ApplyUpdateTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams); Res ApplyUpdateTokenAnyTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams); -Res ApplyMintTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams); +Res ApplyMintTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false); Res ApplyCreatePoolPairTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams); Res ApplyUpdatePoolPairTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams); @@ -130,7 +130,7 @@ Res ApplyAddPoolLiquidityTx(CCustomCSView & mnview, CCoinsViewCache const & coin Res ApplyRemovePoolLiquidityTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams); Res ApplyUtxosToAccountTx(CCustomCSView & mnview, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams); -Res ApplyAccountToUtxosTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams); +Res ApplyAccountToUtxosTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false); Res ApplyAccountToAccountTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams); Res ApplyAnyAccountsToAccountsTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams); diff --git a/src/miner.cpp b/src/miner.cpp index eef2ca8d84..e9c81b3640 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -226,7 +226,7 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc int nPackagesSelected = 0; int nDescendantsUpdated = 0; - addPackageTxs(nPackagesSelected, nDescendantsUpdated); + addPackageTxs(nPackagesSelected, nDescendantsUpdated, nHeight); int64_t nTime1 = GetTimeMicros(); @@ -421,7 +421,7 @@ void BlockAssembler::SortForBlock(const CTxMemPool::setEntries& package, std::ve // Each time through the loop, we compare the best transaction in // mapModifiedTxs with the next transaction in the mempool to decide what // transaction package to work on next. -void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated) +void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated, int nHeight) { // mapModifiedTx will store sorted packages after they are modified // because some of their txs are already in the block @@ -442,6 +442,9 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda const int64_t MAX_CONSECUTIVE_FAILURES = 1000; int64_t nConsecutiveFailed = 0; + // Custom TXs to undo at the end + std::set accountUndo; + while (mi != mempool.mapTx.get().end() || !mapModifiedTx.empty()) { // First try to find a new transaction in mapTx to evaluate. @@ -492,7 +495,7 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda if (packageFees < blockMinFeeRate.GetFee(packageSize)) { // Everything else we might consider has a lower fee rate - return; + break; } if (!TestPackage(packageSize, packageSigOpsCost)) { @@ -538,6 +541,60 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda std::vector sortedEntries; SortForBlock(ancestors, sortedEntries); + // Account check + bool customTxPassed{true}; + + // Custom TXs to undo at the end of following loop if the TX fails. + std::set localUndo; + + // Apply and check custom TXs in order + for (size_t i = 0; i < sortedEntries.size(); ++i) { + const CTransaction& tx = sortedEntries[i]->GetTx(); + + // Do not double check already checked custom TX. This will be an ancestor of current TX. + if (accountUndo.find(tx.GetHash()) != accountUndo.end()) { + continue; + } + + std::vector metadata; + CustomTxType txType = GuessCustomTxType(sortedEntries[i]->GetTx(), metadata); + + // If custom TX wll need to be undone afterwards + if (txType != CustomTxType::None) { + auto res = ApplyCustomTx(*pcustomcsview, g_chainstate->CoinsTip(), sortedEntries[i]->GetTx(), chainparams.GetConsensus(), nHeight, std::numeric_limits::max(), false, true); + + // Add to recently applied custom TXs from sortedEntries + localUndo.insert(tx.GetHash()); + + // Not okay invalidate, undo and skip + if (!res.ok && NotAllowedToFail(txType)) { + customTxPassed = false; + + // Undo local applied custom TXs + for (auto const hash : localUndo) { + pcustomcsview->OnUndoTx(hash, nHeight); + } + pcustomcsview->SetLastHeight(nHeight - 1); + + LogPrintf("%s: Failed %s TX %s: %s\n", __func__, ToString(txType), sortedEntries[i]->GetTx().GetHash().GetHex(), res.msg); + + break; + } + + // Will be undone at the end + accountUndo.insert(tx.GetHash()); + } + } + + // Failed, let's move on! + if (!customTxPassed) { + if (fUsingModified) { + mapModifiedTx.get().erase(modit); + failedTx.insert(iter); + } + continue; + } + for (size_t i=0; iOnUndoTx(hash, nHeight); + } + pcustomcsview->SetLastHeight(nHeight - 1); + } } void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned int& nExtraNonce) diff --git a/src/miner.h b/src/miner.h index 65aa89361a..2793e28e17 100644 --- a/src/miner.h +++ b/src/miner.h @@ -180,7 +180,7 @@ class BlockAssembler /** Add transactions based on feerate including unconfirmed ancestors * Increments nPackagesSelected / nDescendantsUpdated with corresponding * statistics from the package selection (for logging statistics). */ - void addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs); + void addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated, int nHeight) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs); // helper functions for addPackageTxs() /** Remove confirmed (inBlock) entries from given set */ diff --git a/src/validation.cpp b/src/validation.cpp index 17dce66c2b..0bde469e51 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1835,7 +1835,6 @@ Res ApplyGeneralCoinbaseTx(CCustomCSView & mnview, CTransaction const & tx, int return Res::ErrDbg("bad-cb-wrong-tokens", "coinbase should pay only Defi coins"); if (height >= consensus.AMKHeight) { - LogPrintf("ApplyGeneralCoinbaseTx() post AMK logic\n"); // check classic UTXO foundation share: if (!consensus.foundationShareScript.empty() && consensus.foundationShareDFIP1 != 0) { CAmount foundationReward = blockReward * consensus.foundationShareDFIP1 / COIN; diff --git a/test/functional/feature_account_mining.py b/test/functional/feature_account_mining.py index 3d204f87cc..f73b552546 100755 --- a/test/functional/feature_account_mining.py +++ b/test/functional/feature_account_mining.py @@ -16,7 +16,7 @@ def set_test_params(self): def run_test(self): node = self.nodes[0] - node.generate(101) + node.generate(110) # Get addresses and set up account account = node.getnewaddress() @@ -28,8 +28,10 @@ def run_test(self): assert_equal(node.getaccount(account)[0], "10.00000000@DFI") # Send double the amount we have in account - node.accounttoutxos(account, {destination: "10@0"}) - node.accounttoutxos(account, {destination: "10@0"}) + node.accounttoutxos(account, {destination: "10@DFI"}) + node.accounttoutxos(account, {destination: "2@DFI"}) + node.accounttoutxos(account, {destination: "2@DFI"}) + node.accounttoutxos(account, {destination: "2@DFI"}) # Store block height blockcount = node.getblockcount() From da1658d8de4dbb701b818bf2346af12596385cf6 Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Fri, 11 Dec 2020 14:20:54 +0000 Subject: [PATCH 4/8] Use copy of pcustomcsview --- src/miner.cpp | 35 +++++++++-------------------------- 1 file changed, 9 insertions(+), 26 deletions(-) diff --git a/src/miner.cpp b/src/miner.cpp index e9c81b3640..03bf67b083 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -443,7 +443,10 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda int64_t nConsecutiveFailed = 0; // Custom TXs to undo at the end - std::set accountUndo; + std::set checkedTX; + + // Copy of the view + CCustomCSView view(*pcustomcsview); while (mi != mempool.mapTx.get().end() || !mapModifiedTx.empty()) { @@ -544,45 +547,33 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda // Account check bool customTxPassed{true}; - // Custom TXs to undo at the end of following loop if the TX fails. - std::set localUndo; - // Apply and check custom TXs in order for (size_t i = 0; i < sortedEntries.size(); ++i) { const CTransaction& tx = sortedEntries[i]->GetTx(); // Do not double check already checked custom TX. This will be an ancestor of current TX. - if (accountUndo.find(tx.GetHash()) != accountUndo.end()) { + if (checkedTX.find(tx.GetHash()) != checkedTX.end()) { continue; } std::vector metadata; CustomTxType txType = GuessCustomTxType(sortedEntries[i]->GetTx(), metadata); - // If custom TX wll need to be undone afterwards + // Only check custom TXs if (txType != CustomTxType::None) { - auto res = ApplyCustomTx(*pcustomcsview, g_chainstate->CoinsTip(), sortedEntries[i]->GetTx(), chainparams.GetConsensus(), nHeight, std::numeric_limits::max(), false, true); - - // Add to recently applied custom TXs from sortedEntries - localUndo.insert(tx.GetHash()); + auto res = ApplyCustomTx(view, ::ChainstateActive().CoinsTip(), sortedEntries[i]->GetTx(), chainparams.GetConsensus(), nHeight, std::numeric_limits::max(), false, true); // Not okay invalidate, undo and skip if (!res.ok && NotAllowedToFail(txType)) { customTxPassed = false; - // Undo local applied custom TXs - for (auto const hash : localUndo) { - pcustomcsview->OnUndoTx(hash, nHeight); - } - pcustomcsview->SetLastHeight(nHeight - 1); - LogPrintf("%s: Failed %s TX %s: %s\n", __func__, ToString(txType), sortedEntries[i]->GetTx().GetHash().GetHex(), res.msg); break; } - // Will be undone at the end - accountUndo.insert(tx.GetHash()); + // Track checked TXs to avoid double applying + checkedTX.insert(tx.GetHash()); } } @@ -606,14 +597,6 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda // Update transactions that depend on each of these nDescendantsUpdated += UpdatePackagesForAdded(ancestors, mapModifiedTx); } - - // Undo custom TXs to restore state before adding TXs to block - if (!accountUndo.empty()) { - for (auto const hash : accountUndo) { - pcustomcsview->OnUndoTx(hash, nHeight); - } - pcustomcsview->SetLastHeight(nHeight - 1); - } } void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned int& nExtraNonce) From 2ab5eb72612911132306a3f92d8f394c2b2ec8bc Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Fri, 11 Dec 2020 14:59:38 +0000 Subject: [PATCH 5/8] Use tx shortcut for clearer reading --- src/miner.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/miner.cpp b/src/miner.cpp index 03bf67b083..a6b21e9f1e 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -557,17 +557,17 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda } std::vector metadata; - CustomTxType txType = GuessCustomTxType(sortedEntries[i]->GetTx(), metadata); + CustomTxType txType = GuessCustomTxType(tx, metadata); // Only check custom TXs if (txType != CustomTxType::None) { - auto res = ApplyCustomTx(view, ::ChainstateActive().CoinsTip(), sortedEntries[i]->GetTx(), chainparams.GetConsensus(), nHeight, std::numeric_limits::max(), false, true); + auto res = ApplyCustomTx(view, ::ChainstateActive().CoinsTip(), tx, chainparams.GetConsensus(), nHeight, std::numeric_limits::max(), false, true); // Not okay invalidate, undo and skip if (!res.ok && NotAllowedToFail(txType)) { customTxPassed = false; - LogPrintf("%s: Failed %s TX %s: %s\n", __func__, ToString(txType), sortedEntries[i]->GetTx().GetHash().GetHex(), res.msg); + LogPrintf("%s: Failed %s TX %s: %s\n", __func__, ToString(txType), tx.GetHash().GetHex(), res.msg); break; } From 4bfda3e77e46981334e35eda9b634d8ad387c143 Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Fri, 11 Dec 2020 15:00:44 +0000 Subject: [PATCH 6/8] Remove all TXs from mempool that fail CheckTxInputs --- src/txmempool.cpp | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 014990550f..d49891d9ef 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -579,17 +579,16 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne ClearPrioritisation(tx->GetHash()); } - // Check custom TX consensus types are now not in conflict with account layer std::set txsToRemove; + CCoinsViewCache mempoolDuplicate(const_cast(&::ChainstateActive().CoinsTip())); + CAmount txfee = 0; + + // Check custom TX consensus types are now not in conflict with account layer for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); ++it) { - std::vector metadata; - CustomTxType txType = GuessCustomTxType(it->GetTx(), metadata); - if (NotAllowedToFail(txType)) { - auto res = ApplyCustomTx(*pcustomcsview, g_chainstate->CoinsTip(), it->GetTx(), Params().GetConsensus(), nBlockHeight, 0, true); - if (!res.ok && (res.code & CustomTxErrCodes::Fatal)) { - LogPrintf("%s: Remove custom TX: %s\n", __func__, res.msg); - txsToRemove.insert(it->GetSharedTx()); - } + CValidationState state; + if (!Consensus::CheckTxInputs(it->GetTx(), state, mempoolDuplicate, pcustomcsview.get(), nBlockHeight, txfee, Params())) { + LogPrintf("%s: Remove conflicting TX: %s\n", __func__, it->GetTx().GetHash().GetHex()); + txsToRemove.insert(it->GetSharedTx()); } } From 932a67034e97cc3476d030f94d6e8cd2164e671a Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Fri, 11 Dec 2020 20:18:38 +0000 Subject: [PATCH 7/8] Only check entire mempool if consensus TX fails --- src/txmempool.cpp | 50 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 16 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index d49891d9ef..93a8c7b299 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -579,28 +579,46 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne ClearPrioritisation(tx->GetHash()); } - std::set txsToRemove; - CCoinsViewCache mempoolDuplicate(const_cast(&::ChainstateActive().CoinsTip())); - CAmount txfee = 0; + bool accountConflict{false}; - // Check custom TX consensus types are now not in conflict with account layer + // Check if any custom TXs are in mempool with conflict for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); ++it) { - CValidationState state; - if (!Consensus::CheckTxInputs(it->GetTx(), state, mempoolDuplicate, pcustomcsview.get(), nBlockHeight, txfee, Params())) { - LogPrintf("%s: Remove conflicting TX: %s\n", __func__, it->GetTx().GetHash().GetHex()); - txsToRemove.insert(it->GetSharedTx()); + std::vector metadata; + CustomTxType txType = GuessCustomTxType(it->GetTx(), metadata); + if (NotAllowedToFail(txType)) { + auto res = ApplyCustomTx(*pcustomcsview, g_chainstate->CoinsTip(), it->GetTx(), Params().GetConsensus(), nBlockHeight, 0, true); + if (!res.ok && (res.code & CustomTxErrCodes::Fatal)) { + accountConflict = true; + break; + } } } - for (auto& tx : txsToRemove) { - txiter it = mapTx.find(tx->GetHash()); - if (it != mapTx.end()) { - setEntries stage; - stage.insert(it); - RemoveStaged(stage, true, MemPoolRemovalReason::CONFLICT); + // Account conflict, check entire mempool + if (accountConflict) { + std::set txsToRemove; + CCoinsViewCache mempoolDuplicate(const_cast(&::ChainstateActive().CoinsTip())); + CAmount txfee = 0; + + // Check custom TX consensus types are now not in conflict with account layer + for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); ++it) { + CValidationState state; + if (!Consensus::CheckTxInputs(it->GetTx(), state, mempoolDuplicate, pcustomcsview.get(), nBlockHeight, txfee, Params())) { + LogPrintf("%s: Remove conflicting TX: %s\n", __func__, it->GetTx().GetHash().GetHex()); + txsToRemove.insert(it->GetSharedTx()); + } + } + + for (auto& tx : txsToRemove) { + txiter it = mapTx.find(tx->GetHash()); + if (it != mapTx.end()) { + setEntries stage; + stage.insert(it); + RemoveStaged(stage, true, MemPoolRemovalReason::CONFLICT); + } + removeConflicts(*tx); + ClearPrioritisation(tx->GetHash()); } - removeConflicts(*tx); - ClearPrioritisation(tx->GetHash()); } lastRollingFeeUpdate = GetTime(); From 26cd1eba99891e3977502e962df7af4221dc92c5 Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Sat, 12 Dec 2020 07:07:44 +0000 Subject: [PATCH 8/8] Create 100 sends of 2DFI --- test/functional/feature_account_mining.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/test/functional/feature_account_mining.py b/test/functional/feature_account_mining.py index f73b552546..82413d1bc9 100755 --- a/test/functional/feature_account_mining.py +++ b/test/functional/feature_account_mining.py @@ -16,7 +16,7 @@ def set_test_params(self): def run_test(self): node = self.nodes[0] - node.generate(110) + node.generate(120) # Get addresses and set up account account = node.getnewaddress() @@ -28,10 +28,8 @@ def run_test(self): assert_equal(node.getaccount(account)[0], "10.00000000@DFI") # Send double the amount we have in account - node.accounttoutxos(account, {destination: "10@DFI"}) - node.accounttoutxos(account, {destination: "2@DFI"}) - node.accounttoutxos(account, {destination: "2@DFI"}) - node.accounttoutxos(account, {destination: "2@DFI"}) + for _ in range(100): + node.accounttoutxos(account, {destination: "2@DFI"}) # Store block height blockcount = node.getblockcount()