From dc392cae6a354ea8875aaad98d9bcd6149ac03a9 Mon Sep 17 00:00:00 2001 From: Anthony Fieroni Date: Mon, 14 Dec 2020 08:57:15 +0200 Subject: [PATCH 1/9] Prevent bad tx propagation Signed-off-by: Anthony Fieroni --- src/validation.cpp | 16 +++++- .../feature_prevent_bad_tx_propagation.py | 57 +++++++++++++++++++ test/functional/test_runner.py | 1 + 3 files changed, 73 insertions(+), 1 deletion(-) create mode 100755 test/functional/feature_prevent_bad_tx_propagation.py diff --git a/src/validation.cpp b/src/validation.cpp index aaba18dc40..392fcd274f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -561,6 +561,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool { CCoinsView dummy; CCoinsViewCache view(&dummy); + CCustomCSView mnview(*pcustomcsview); LockPoints lp; CCoinsViewCache& coins_cache = ::ChainstateActive().CoinsTip(); @@ -601,8 +602,21 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // Bring the best block into scope view.GetBestBlock(); + const auto height = GetSpendHeight(view); + std::vector metadata; + + // check for txs in mempool + for (const auto& e : mempool.mapTx) { + const auto& tx = e.GetTx(); + const auto txType = GuessCustomTxType(tx, metadata); + if (NotAllowedToFail(txType)) { + auto res = ApplyCustomTx(mnview, view, tx, chainparams.GetConsensus(), height, 0, false); + assert(res.ok || !(res.code & CustomTxErrCodes::Fatal)); // inconsistent mempool + } + } + CAmount nFees = 0; - if (!Consensus::CheckTxInputs(tx, state, view, pcustomcsview.get(), GetSpendHeight(view), nFees, chainparams)) { + if (!Consensus::CheckTxInputs(tx, state, view, &mnview, height, nFees, chainparams)) { return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state)); } diff --git a/test/functional/feature_prevent_bad_tx_propagation.py b/test/functional/feature_prevent_bad_tx_propagation.py new file mode 100755 index 0000000000..8cc0e292e8 --- /dev/null +++ b/test/functional/feature_prevent_bad_tx_propagation.py @@ -0,0 +1,57 @@ +#!/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.authproxy import JSONRPCException +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(120) + + # Get addresses and set up account + account = node.getnewaddress() + destination = node.getnewaddress() + + node.utxostoaccount({account: "5@0"}) + + node.generate(1) + + # Check we have expected balance + assert_equal(node.getaccount(account)[0], "5.00000000@DFI") + + # Corrent account to utxo tx - entering mempool + node.accounttoutxos(account, {destination: "4@DFI"}) + + try: + # Not enough amount - rejected + node.accounttoutxos(account, {destination: "2@DFI"}) + except JSONRPCException as e: + errorString = e.error['message'] + + assert('bad-txns-customtx' in errorString) + + # Store block height + blockcount = node.getblockcount() + + # One minted block with correct tx + node.generate(1) + + # Check the blockchain height + assert_equal(node.getblockcount(), blockcount + 1) + + # Account should have 1@DFI + assert_equal(node.getaccount(account)[0], "1.00000000@DFI") + +if __name__ == '__main__': + AccountMiningTest().main () diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index f05b97137c..a272aa6e1b 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -233,6 +233,7 @@ 'rpc_help.py', 'feature_help.py', 'feature_shutdown.py', + 'feature_prevent_bad_tx_propagation.py' # Don't append tests at the end to avoid merge conflicts # Put them in a random line within the section that fits their approximate run-time ] From b8632a9a658279154379ef090973209f7371ed4b Mon Sep 17 00:00:00 2001 From: Anthony Fieroni Date: Mon, 14 Dec 2020 11:12:16 +0200 Subject: [PATCH 2/9] Do not allow all invalid tx Signed-off-by: Anthony Fieroni --- src/validation.cpp | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 392fcd274f..385791f1be 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -603,16 +603,12 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool view.GetBestBlock(); const auto height = GetSpendHeight(view); - std::vector metadata; // check for txs in mempool for (const auto& e : mempool.mapTx) { const auto& tx = e.GetTx(); - const auto txType = GuessCustomTxType(tx, metadata); - if (NotAllowedToFail(txType)) { - auto res = ApplyCustomTx(mnview, view, tx, chainparams.GetConsensus(), height, 0, false); - assert(res.ok || !(res.code & CustomTxErrCodes::Fatal)); // inconsistent mempool - } + auto res = ApplyCustomTx(mnview, view, tx, chainparams.GetConsensus(), height, 0, false); + assert(res.ok || !(res.code & CustomTxErrCodes::Fatal)); // inconsistent mempool } CAmount nFees = 0; @@ -620,6 +616,17 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state)); } + // NOTE Consensus::CheckTxInputs will check for NotAllowToFail + std::vector metadata; + const auto txType = GuessCustomTxType(tx, metadata); + + if (!NotAllowedToFail(txType)) { + auto res = ApplyCustomTx(mnview, view, tx, chainparams.GetConsensus(), height, 0, false); + if (!res.ok) { + return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INVALID, res.msg); + } + } + // we have all inputs cached now, so switch back to dummy, so we don't need to keep lock on mempool view.SetBackend(dummy); From c8fad484ef24f2cfee03064a332eb0d212edbeb4 Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Mon, 14 Dec 2020 08:00:06 +0000 Subject: [PATCH 3/9] Reject all custom TXs that fail from block creation --- src/miner.cpp | 2 +- test/functional/feature_account_mining.py | 27 +++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/miner.cpp b/src/miner.cpp index a6b21e9f1e..cdb3932a23 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -564,7 +564,7 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda 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)) { + if (!res.ok) { customTxPassed = false; LogPrintf("%s: Failed %s TX %s: %s\n", __func__, ToString(txType), tx.GetHash().GetHex(), res.msg); diff --git a/test/functional/feature_account_mining.py b/test/functional/feature_account_mining.py index 82413d1bc9..074d61c906 100755 --- a/test/functional/feature_account_mining.py +++ b/test/functional/feature_account_mining.py @@ -40,8 +40,35 @@ def run_test(self): # Check the blockchain has extended as expected assert_equal(node.getblockcount(), blockcount + 1) + # Generate 10 more blocks + node.generate(10) + + # Check the blockchain has extended as expected + assert_equal(node.getblockcount(), blockcount + 11) + # Account should now be empty assert_equal(node.getaccount(account), []) + # Update block height + blockcount = node.getblockcount() + + # Send more UTXOs to account + node.utxostoaccount({account: "1@0"}) + node.generate(1) + + # Update block height + blockcount = node.getblockcount() + + # Test mixture of account TXs + for _ in range(10): + node.accounttoaccount(account, {destination: "1@DFI"}) + node.accounttoutxos(account, {destination: "1@DFI"}) + + # Generate 1 more blocks + node.generate(1) + + # Check the blockchain has extended as expected + assert_equal(node.getblockcount(), blockcount + 1) + if __name__ == '__main__': AccountMiningTest().main () From 1c31a20f01f90e87427d813f46b0176a488f4524 Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Mon, 14 Dec 2020 10:10:02 +0000 Subject: [PATCH 4/9] Remove redundant code to avoid acindex tracking --- src/miner.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/miner.cpp b/src/miner.cpp index cdb3932a23..0dab148e57 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -561,7 +561,7 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda // Only check custom TXs if (txType != CustomTxType::None) { - auto res = ApplyCustomTx(view, ::ChainstateActive().CoinsTip(), tx, chainparams.GetConsensus(), nHeight, std::numeric_limits::max(), false, true); + auto res = ApplyCustomTx(view, ::ChainstateActive().CoinsTip(), tx, chainparams.GetConsensus(), nHeight, 0, false, true); // Not okay invalidate, undo and skip if (!res.ok) { From c29fa28919cb8d26161d982dc76b95c14d261ad1 Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Mon, 14 Dec 2020 10:41:45 +0000 Subject: [PATCH 5/9] [tests] Expect RPC errors from now invalid account transfers --- test/functional/feature_account_mining.py | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/test/functional/feature_account_mining.py b/test/functional/feature_account_mining.py index 074d61c906..e11f11cef2 100755 --- a/test/functional/feature_account_mining.py +++ b/test/functional/feature_account_mining.py @@ -7,6 +7,7 @@ from test_framework.test_framework import DefiTestFramework from test_framework.util import assert_equal +from test_framework.authproxy import JSONRPCException class AccountMiningTest(DefiTestFramework): def set_test_params(self): @@ -28,8 +29,13 @@ def run_test(self): assert_equal(node.getaccount(account)[0], "10.00000000@DFI") # Send double the amount we have in account + thrown = False for _ in range(100): - node.accounttoutxos(account, {destination: "2@DFI"}) + try: + node.accounttoutxos(account, {destination: "2@DFI"}) + except JSONRPCException: + thrown = True + assert_equal(thrown, True) # Store block height blockcount = node.getblockcount() @@ -60,9 +66,14 @@ def run_test(self): blockcount = node.getblockcount() # Test mixture of account TXs + thrown = False for _ in range(10): - node.accounttoaccount(account, {destination: "1@DFI"}) - node.accounttoutxos(account, {destination: "1@DFI"}) + try: + node.accounttoaccount(account, {destination: "1@DFI"}) + node.accounttoutxos(account, {destination: "1@DFI"}) + except JSONRPCException: + thrown = True + assert_equal(thrown, True) # Generate 1 more blocks node.generate(1) From 5a3334e019c81a1828de417f769a1f79b671886c Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Mon, 14 Dec 2020 14:02:54 +0000 Subject: [PATCH 6/9] Skip auth when called from adPackageTxs --- src/masternodes/mn_checks.cpp | 113 ++++++++++++++++++---------------- src/masternodes/mn_checks.h | 24 ++++---- 2 files changed, 72 insertions(+), 65 deletions(-) diff --git a/src/masternodes/mn_checks.cpp b/src/masternodes/mn_checks.cpp index bf9df6ab22..bb330f18e2 100644 --- a/src/masternodes/mn_checks.cpp +++ b/src/masternodes/mn_checks.cpp @@ -141,34 +141,34 @@ Res ApplyCustomTx(CCustomCSView & base_mnview, CCoinsViewCache const & coins, CT res = ApplyCreateMasternodeTx(mnview, tx, height, metadata); break; case CustomTxType::ResignMasternode: - res = ApplyResignMasternodeTx(mnview, coins, tx, height, metadata); + res = ApplyResignMasternodeTx(mnview, coins, tx, height, metadata, skipAuth); break; case CustomTxType::CreateToken: - res = ApplyCreateTokenTx(mnview, coins, tx, height, metadata, consensusParams); + res = ApplyCreateTokenTx(mnview, coins, tx, height, metadata, consensusParams, skipAuth); break; case CustomTxType::UpdateToken: - res = ApplyUpdateTokenTx(mnview, coins, tx, height, metadata, consensusParams); + res = ApplyUpdateTokenTx(mnview, coins, tx, height, metadata, consensusParams, skipAuth); break; case CustomTxType::UpdateTokenAny: - res = ApplyUpdateTokenAnyTx(mnview, coins, tx, height, metadata, consensusParams); + res = ApplyUpdateTokenAnyTx(mnview, coins, tx, height, metadata, consensusParams, skipAuth); break; case CustomTxType::MintToken: res = ApplyMintTokenTx(mnview, coins, tx, height, metadata, consensusParams, skipAuth); break; case CustomTxType::CreatePoolPair: - res = ApplyCreatePoolPairTx(mnview, coins, tx, height, metadata, consensusParams); + res = ApplyCreatePoolPairTx(mnview, coins, tx, height, metadata, consensusParams, skipAuth); break; case CustomTxType::UpdatePoolPair: - res = ApplyUpdatePoolPairTx(mnview, coins, tx, height, metadata, consensusParams); + res = ApplyUpdatePoolPairTx(mnview, coins, tx, height, metadata, consensusParams, skipAuth); break; case CustomTxType::PoolSwap: - res = ApplyPoolSwapTx(mnview, coins, tx, height, metadata, consensusParams); + res = ApplyPoolSwapTx(mnview, coins, tx, height, metadata, consensusParams, skipAuth); break; case CustomTxType::AddPoolLiquidity: - res = ApplyAddPoolLiquidityTx(mnview, coins, tx, height, metadata, consensusParams); + res = ApplyAddPoolLiquidityTx(mnview, coins, tx, height, metadata, consensusParams, skipAuth); break; case CustomTxType::RemovePoolLiquidity: - res = ApplyRemovePoolLiquidityTx(mnview, coins, tx, height, metadata, consensusParams); + res = ApplyRemovePoolLiquidityTx(mnview, coins, tx, height, metadata, consensusParams, skipAuth); break; case CustomTxType::UtxosToAccount: res = ApplyUtxosToAccountTx(mnview, tx, height, metadata, consensusParams); @@ -177,13 +177,13 @@ Res ApplyCustomTx(CCustomCSView & base_mnview, CCoinsViewCache const & coins, CT res = ApplyAccountToUtxosTx(mnview, coins, tx, height, metadata, consensusParams, skipAuth); break; case CustomTxType::AccountToAccount: - res = ApplyAccountToAccountTx(mnview, coins, tx, height, metadata, consensusParams); + res = ApplyAccountToAccountTx(mnview, coins, tx, height, metadata, consensusParams, skipAuth); break; case CustomTxType::SetGovVariable: - res = ApplySetGovernanceTx(mnview, coins, tx, height, metadata, consensusParams); + res = ApplySetGovernanceTx(mnview, coins, tx, height, metadata, consensusParams, skipAuth); break; case CustomTxType::AnyAccountsToAccounts: - res = ApplyAnyAccountsToAccountsTx(mnview, coins, tx, height, metadata, consensusParams); + res = ApplyAnyAccountsToAccountsTx(mnview, coins, tx, height, metadata, consensusParams, skipAuth); break; default: return Res::Ok(); // not "custom" tx @@ -258,7 +258,7 @@ Res ApplyCreateMasternodeTx(CCustomCSView & mnview, CTransaction const & tx, uin return Res::Ok(base); } -Res ApplyResignMasternodeTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, const std::vector & metadata) +Res ApplyResignMasternodeTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, const std::vector & metadata, bool skipAuth) { const std::string base{"Resigning of masternode"}; @@ -270,7 +270,7 @@ Res ApplyResignMasternodeTx(CCustomCSView & mnview, CCoinsViewCache const & coin if (!node) { return Res::Err("%s: node %s does not exist", base, nodeId.ToString()); } - if (!HasCollateralAuth(tx, coins, nodeId)) { + if (!skipAuth && !HasCollateralAuth(tx, coins, nodeId)) { return Res::Err("%s %s: %s", base, nodeId.ToString(), "tx must have at least one input from masternode owner"); } @@ -281,7 +281,7 @@ Res ApplyResignMasternodeTx(CCustomCSView & mnview, CCoinsViewCache const & coin return Res::Ok(base); } -Res ApplyCreateTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams) +Res ApplyCreateTokenTx(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); } @@ -307,7 +307,7 @@ Res ApplyCreateTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CT token.creationHeight = height; //check foundation auth - if((token.IsDAT()) && !HasFoundationAuth(tx, coins, consensusParams)) + if((token.IsDAT()) && !skipAuth && !HasFoundationAuth(tx, coins, consensusParams)) {//no need to check Authority if we don't create isDAT return Res::Err("%s: %s", base, "tx not from foundation member"); } @@ -327,7 +327,7 @@ Res ApplyCreateTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CT } /// @deprecated version of updatetoken tx, prefer using UpdateTokenAny after "bayfront" fork -Res ApplyUpdateTokenTx(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, bool skipAuth) { if((int)height < consensusParams.AMKHeight) { return Res::Err("Token tx before AMK height (block %d)", consensusParams.AMKHeight); } @@ -353,7 +353,7 @@ Res ApplyUpdateTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CT CTokenImplementation const & token = pair->second; //check foundation auth - if (!HasFoundationAuth(tx, coins, consensusParams)) { + if (!skipAuth && !HasFoundationAuth(tx, coins, consensusParams)) { return Res::Err("%s: %s", base, "Is not a foundation owner"); } @@ -371,7 +371,7 @@ Res ApplyUpdateTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CT } -Res ApplyUpdateTokenAnyTx(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, bool skipAuth) { if ((int)height < consensusParams.BayfrontHeight) { return Res::Err("Improved updatetoken tx before Bayfront height"); @@ -406,20 +406,22 @@ Res ApplyUpdateTokenAnyTx(CCustomCSView & mnview, CCoinsViewCache const & coins, const Coin& auth = coins.AccessCoin(COutPoint(token.creationTx, 1)); // always n=1 output bool isFoundersToken = consensusParams.foundationMembers.find(auth.out.scriptPubKey) != consensusParams.foundationMembers.end(); - if (isFoundersToken && !HasFoundationAuth(tx, coins, consensusParams)) { - return Res::Err("%s: %s", base, "tx not from foundation member"); - } - else if (!HasCollateralAuth(tx, coins, token.creationTx)) { - return Res::Err("%s: %s", base, "tx must have at least one input from token owner"); - } + if (!skipAuth) { + if (isFoundersToken && !HasFoundationAuth(tx, coins, consensusParams)) { + return Res::Err("%s: %s", base, "tx not from foundation member"); + } + else if (!HasCollateralAuth(tx, coins, token.creationTx)) { + return Res::Err("%s: %s", base, "tx must have at least one input from token owner"); + } - // Check for isDAT change in non-foundation token after set height - if (static_cast(height) >= consensusParams.BayfrontMarinaHeight) - { - //check foundation auth - if((newToken.IsDAT() != token.IsDAT()) && !HasFoundationAuth(tx, coins, consensusParams)) - {//no need to check Authority if we don't create isDAT - return Res::Err("%s: %s", base, "can't set isDAT to true, tx not from foundation member"); + // Check for isDAT change in non-foundation token after set height + if (static_cast(height) >= consensusParams.BayfrontMarinaHeight) + { + //check foundation auth + if((newToken.IsDAT() != token.IsDAT()) && !HasFoundationAuth(tx, coins, consensusParams)) + {//no need to check Authority if we don't create isDAT + return Res::Err("%s: %s", base, "can't set isDAT to true, tx not from foundation member"); + } } } @@ -464,7 +466,7 @@ Res ApplyMintTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTra if (tokenId < CTokensView::DCT_ID_START) return Res::Err("%s: token %s is a 'stable coin', can't mint stable coin!", base, tokenId.ToString()); - if (!HasAuth(tx, coins, auth.out.scriptPubKey)) { + if (!skipAuth && !HasAuth(tx, coins, auth.out.scriptPubKey)) { return Res::Err("%s: %s", base, "tx must have at least one input from token owner"); } } @@ -502,7 +504,7 @@ Res ApplyMintTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTra return Res::Ok(base); } -Res ApplyAddPoolLiquidityTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams) +Res ApplyAddPoolLiquidityTx(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.BayfrontHeight) { return Res::Err("LP tx before Bayfront height (block %d)", consensusParams.BayfrontHeight); @@ -542,9 +544,11 @@ Res ApplyAddPoolLiquidityTx(CCustomCSView & mnview, CCoinsViewCache const & coin return Res::Err("%s: there is no such pool pair", base); } - for (const auto& kv : msg.from) { - if (!HasAuth(tx, coins, kv.first)) { - return Res::Err("%s: %s", base, "tx must have at least one input from account owner"); + if (!skipAuth) { + for (const auto& kv : msg.from) { + if (!HasAuth(tx, coins, kv.first)) { + return Res::Err("%s: %s", base, "tx must have at least one input from account owner"); + } } } @@ -585,7 +589,7 @@ Res ApplyAddPoolLiquidityTx(CCustomCSView & mnview, CCoinsViewCache const & coin return mnview.SetPoolPair(lpTokenID, pool); } -Res ApplyRemovePoolLiquidityTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams) +Res ApplyRemovePoolLiquidityTx(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.BayfrontHeight) { return Res::Err("LP tx before Bayfront height (block %d)", consensusParams.BayfrontHeight); @@ -615,7 +619,7 @@ Res ApplyRemovePoolLiquidityTx(CCustomCSView & mnview, CCoinsViewCache const & c return Res::Err("%s: there is no such pool pair", base); } - if (!HasAuth(tx, coins, from)) { + if (!skipAuth && !HasAuth(tx, coins, from)) { return Res::Err("%s: %s", base, "tx must have at least one input from account owner"); } @@ -758,7 +762,7 @@ Res ApplyAccountToUtxosTx(CCustomCSView & mnview, CCoinsViewCache const & coins, return Res::Ok(base); } -Res ApplyAccountToAccountTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams) +Res ApplyAccountToAccountTx(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); } @@ -772,7 +776,7 @@ Res ApplyAccountToAccountTx(CCustomCSView & mnview, CCoinsViewCache const & coin const auto base = strprintf("Transfer AccountToAccount: %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"); } // transfer @@ -815,7 +819,7 @@ Res ApplyAccountToAccountTx(CCustomCSView & mnview, CCoinsViewCache const & coin return Res::Ok(base); } -Res ApplyAnyAccountsToAccountsTx(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, bool skipAuth) { if((int)height < consensusParams.BayfrontGardensHeight) { return Res::Err("Token tx before BayfrontGardensHeight (block %d)", consensusParams.BayfrontGardensHeight ); } @@ -829,11 +833,14 @@ Res ApplyAnyAccountsToAccountsTx(CCustomCSView & mnview, CCoinsViewCache const & const auto base = strprintf("Transfer AnyAccountsToAccounts: %s", msg.ToString()); // check auth - for (auto const & kv : msg.from) { - if (!HasAuth(tx, coins, kv.first)) { - return Res::Err("%s: %s", base, "tx must have at least one input from account owner"); + if (!skipAuth) { + for (auto const & kv : msg.from) { + if (!HasAuth(tx, coins, kv.first)) { + return Res::Err("%s: %s", base, "tx must have at least one input from account owner"); + } } } + // compare auto const sumFrom = SumAllTransfers(msg.from); auto const sumTo = SumAllTransfers(msg.to); @@ -886,7 +893,7 @@ Res ApplyAnyAccountsToAccountsTx(CCustomCSView & mnview, CCoinsViewCache const & return Res::Ok(base); } -Res ApplyCreatePoolPairTx(CCustomCSView &mnview, const CCoinsViewCache &coins, const CTransaction &tx, uint32_t height, const std::vector &metadata, Consensus::Params const & consensusParams) +Res ApplyCreatePoolPairTx(CCustomCSView &mnview, const CCoinsViewCache &coins, const CTransaction &tx, uint32_t height, const std::vector &metadata, Consensus::Params const & consensusParams, bool skipAuth) { if ((int)height < consensusParams.BayfrontHeight) { return Res::Err("LP tx before Bayfront height (block %d)", consensusParams.BayfrontHeight); @@ -904,7 +911,7 @@ Res ApplyCreatePoolPairTx(CCustomCSView &mnview, const CCoinsViewCache &coins, c } //check foundation auth - if(!HasFoundationAuth(tx, coins, consensusParams)) { + if(!skipAuth && !HasFoundationAuth(tx, coins, consensusParams)) { return Res::Err("%s: %s", base, "tx not from foundation member"); } if(poolPairMsg.commission < 0 || poolPairMsg.commission > COIN) { @@ -961,7 +968,7 @@ Res ApplyCreatePoolPairTx(CCustomCSView &mnview, const CCoinsViewCache &coins, c return Res::Ok(base); } -Res ApplyUpdatePoolPairTx(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, bool skipAuth) { if((int)height < consensusParams.BayfrontHeight) { return Res::Err("LP tx before Bayfront height (block %d)", consensusParams.BayfrontHeight); @@ -988,7 +995,7 @@ Res ApplyUpdatePoolPairTx(CCustomCSView & mnview, CCoinsViewCache const & coins, } //check foundation auth - if (!HasFoundationAuth(tx, coins, consensusParams)) { + if (!skipAuth && !HasFoundationAuth(tx, coins, consensusParams)) { return Res::Err("%s: %s", base, "tx not from foundation member"); } @@ -999,7 +1006,7 @@ Res ApplyUpdatePoolPairTx(CCustomCSView & mnview, CCoinsViewCache const & coins, return Res::Ok(base); } -Res ApplyPoolSwapTx(CCustomCSView &mnview, const CCoinsViewCache &coins, const CTransaction &tx, uint32_t height, const std::vector &metadata, Consensus::Params const & consensusParams) +Res ApplyPoolSwapTx(CCustomCSView &mnview, const CCoinsViewCache &coins, const CTransaction &tx, uint32_t height, const std::vector &metadata, Consensus::Params const & consensusParams, bool skipAuth) { if ((int)height < consensusParams.BayfrontHeight) { return Res::Err("LP tx before Bayfront height (block %d)", consensusParams.BayfrontHeight); @@ -1015,7 +1022,7 @@ Res ApplyPoolSwapTx(CCustomCSView &mnview, const CCoinsViewCache &coins, const C const std::string base{"PoolSwap creation: " + poolSwapMsg.ToString()}; // check auth - if (!HasAuth(tx, coins, poolSwapMsg.from)) { + if (!skipAuth && !HasAuth(tx, coins, poolSwapMsg.from)) { return Res::Err("%s: %s", base, "tx must have at least one input from account owner"); } @@ -1061,7 +1068,7 @@ Res ApplyPoolSwapTx(CCustomCSView &mnview, const CCoinsViewCache &coins, const C return Res::Ok(); } -Res ApplySetGovernanceTx(CCustomCSView &mnview, const CCoinsViewCache &coins, const CTransaction &tx, uint32_t height, const std::vector &metadata, Consensus::Params const & consensusParams) +Res ApplySetGovernanceTx(CCustomCSView &mnview, const CCoinsViewCache &coins, const CTransaction &tx, uint32_t height, const std::vector &metadata, Consensus::Params const & consensusParams, bool skipAuth) { if ((int)height < consensusParams.BayfrontHeight) { return Res::Err("Governance tx before Bayfront height (block %d)", consensusParams.BayfrontHeight); @@ -1070,7 +1077,7 @@ Res ApplySetGovernanceTx(CCustomCSView &mnview, const CCoinsViewCache &coins, co const std::string base{"Set governance variable"}; //check foundation auth - if(!HasFoundationAuth(tx, coins, consensusParams)) + if(!skipAuth && !HasFoundationAuth(tx, coins, consensusParams)) { return Res::Err("%s: %s", base, "tx not from foundation member"); } diff --git a/src/masternodes/mn_checks.h b/src/masternodes/mn_checks.h index 4b579bab25..07b34598d7 100644 --- a/src/masternodes/mn_checks.h +++ b/src/masternodes/mn_checks.h @@ -112,25 +112,25 @@ bool HasCollateralAuth(CTransaction const & tx, CCoinsViewCache const & coins, u 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); +Res ApplyResignMasternodeTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, bool skipAuth = false); -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 ApplyCreateTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false); +Res ApplyUpdateTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false); +Res ApplyUpdateTokenAnyTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false); 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); -Res ApplyPoolSwapTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams); -Res ApplyAddPoolLiquidityTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams); -Res ApplyRemovePoolLiquidityTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams); +Res ApplyCreatePoolPairTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false); +Res ApplyUpdatePoolPairTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false); +Res ApplyPoolSwapTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false); +Res ApplyAddPoolLiquidityTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false); +Res ApplyRemovePoolLiquidityTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false); 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, 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); +Res ApplyAccountToAccountTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false); +Res ApplyAnyAccountsToAccountsTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false); -Res ApplySetGovernanceTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams); +Res ApplySetGovernanceTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false); ResVal ApplyAnchorRewardTx(CCustomCSView & mnview, CTransaction const & tx, int height, uint256 const & prevStakeModifier, std::vector const & metadata, Consensus::Params const & consensusParams); From 8688da22e374a338ee7dba34a8a51c037194adbf Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Mon, 14 Dec 2020 14:29:12 +0000 Subject: [PATCH 7/9] [tests] Catch expected RPC call failure --- test/functional/feature_tokens_multisig.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/test/functional/feature_tokens_multisig.py b/test/functional/feature_tokens_multisig.py index 3e31f462e8..d9c1ddb406 100755 --- a/test/functional/feature_tokens_multisig.py +++ b/test/functional/feature_tokens_multisig.py @@ -98,13 +98,12 @@ def run_test(self): signed_rawtx_1 = self.nodes[0].signrawtransactionwithkey(rawtx_1, [owner_1_privkey], [{"txid":txid_owner_1,"vout":vout_owner_1,"scriptPubKey":owner_1_scriptpubkey}]) assert_equal(signed_rawtx_1['complete'], True) - # Send first name change TX - self.nodes[0].sendrawtransaction(signed_rawtx_1['hex']) - self.nodes[0].generate(1) - - # Check that name has not changed - t128 = self.nodes[0].gettoken(128) - assert_equal(t128['128']['name'], "shiny") + # Send should fail as transaction is invalid + try: + self.nodes[0].sendrawtransaction(signed_rawtx_1['hex']) + except JSONRPCException as e: + errorString = e.error['message'] + assert("tx must have at least one input from token owner" in errorString) # Test that multisig TXs can change names rawtx_1 = self.nodes[0].createrawtransaction([{"txid":txid_1,"vout":vout_1}], [{"data":name_change_1},{owner_1:0.9999}]) From f8b0a3bc42b64e59d3a9830319ea073adcb0b594 Mon Sep 17 00:00:00 2001 From: Peter Bushnell Date: Tue, 15 Dec 2020 09:52:30 +0000 Subject: [PATCH 8/9] Abandon wallet custom TXs that fail to enter mempool on client start --- src/wallet/wallet.cpp | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 32592f36fc..3b1099352b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2152,8 +2152,19 @@ void CWallet::ReacceptWalletTransactions(interfaces::Chain::Lock& locked_chain) // Try to add wallet transactions to memory pool for (const std::pair& item : mapSorted) { CWalletTx& wtx = *(item.second); - std::string unused_err_string; - wtx.SubmitMemoryPoolAndRelay(unused_err_string, false, locked_chain); + std::string err_string; + bool result = wtx.SubmitMemoryPoolAndRelay(err_string, false, locked_chain); + + // err_string only set on mempool acceptance failure + if (!result && !err_string.empty()) { + std::vector metadata; + CustomTxType txType = GuessCustomTxType(*item.second->tx, metadata); + + // Abandon custom TXs that are rejected by mempool + if (txType != CustomTxType::None) { + AbandonTransaction(locked_chain, item.second->tx->GetHash()); + } + } } } From 733ca1127bfd7edcc647d08a43784976dcdc37bb Mon Sep 17 00:00:00 2001 From: Anthony Fieroni Date: Tue, 15 Dec 2020 17:14:40 +0200 Subject: [PATCH 9/9] Use tx order by entry time Signed-off-by: Anthony Fieroni --- src/validation.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 385791f1be..f5dadba8ca 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -605,7 +605,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool const auto height = GetSpendHeight(view); // check for txs in mempool - for (const auto& e : mempool.mapTx) { + for (const auto& e : mempool.mapTx.get()) { const auto& tx = e.GetTx(); auto res = ApplyCustomTx(mnview, view, tx, chainparams.GetConsensus(), height, 0, false); assert(res.ok || !(res.code & CustomTxErrCodes::Fatal)); // inconsistent mempool @@ -622,7 +622,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool if (!NotAllowedToFail(txType)) { auto res = ApplyCustomTx(mnview, view, tx, chainparams.GetConsensus(), height, 0, false); - if (!res.ok) { + if (!res.ok || (res.code & CustomTxErrCodes::Fatal)) { return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INVALID, res.msg); } }