From 8f1290c60159a3171c27250bc95687548c5c1b84 Mon Sep 17 00:00:00 2001 From: gzhao408 Date: Sat, 25 Jul 2020 10:05:15 -0700 Subject: [PATCH] [rpc/node] check for high fee before ATMP in clients Check absurd fee in BroadcastTransaction and RPC, return TransactionError::MAX_FEE_EXCEEDED instead of TxValidationResult::TX_NOT_STANDARD because this is client preference, not a node-wide policy. --- src/node/transaction.cpp | 38 +++++++++++++++++++-------- src/rpc/rawtransaction.cpp | 10 ++++++- src/validation.cpp | 3 +-- test/functional/rpc_rawtransaction.py | 8 +++--- 4 files changed, 41 insertions(+), 18 deletions(-) diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 9ae4700743..8162b79a0b 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -13,6 +13,18 @@ #include +static TransactionError HandleATMPError(const TxValidationState& state, std::string& err_string_out) { + err_string_out = state.ToString(); + if (state.IsInvalid()) { + if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { + return TransactionError::MISSING_INPUTS; + } + return TransactionError::MEMPOOL_REJECTED; + } else { + return TransactionError::MEMPOOL_ERROR; + } +} + TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback) { // BroadcastTransaction can be called by either sendrawtransaction RPC or wallet RPCs. @@ -36,20 +48,24 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t if (!existingCoin.IsSpent()) return TransactionError::ALREADY_IN_CHAIN; } if (!node.mempool->exists(hashTx)) { - // Transaction is not already in the mempool. Submit it. + // Transaction is not already in the mempool. TxValidationState state; - if (!AcceptToMemoryPool(*node.mempool, state, tx, - nullptr /* plTxnReplaced */, false /* bypass_limits */, max_tx_fee)) { - err_string = state.ToString(); - if (state.IsInvalid()) { - if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { - return TransactionError::MISSING_INPUTS; - } - return TransactionError::MEMPOOL_REJECTED; - } else { - return TransactionError::MEMPOOL_ERROR; + CAmount fee{0}; + if (max_tx_fee) { + // First, call ATMP with test_accept and check the fee. If ATMP + // fails here, return error immediately. + if (!AcceptToMemoryPool(*node.mempool, state, tx, + nullptr /* plTxnReplaced */, false /* bypass_limits */, /* absurdfee*/ 0, /* test_accept */ true, &fee)) { + return HandleATMPError(state, err_string); + } else if (fee > max_tx_fee) { + return TransactionError::MAX_FEE_EXCEEDED; } } + // Try to submit the transaction to the mempool. + if (!AcceptToMemoryPool(*node.mempool, state, tx, + nullptr /* plTxnReplaced */, false /* bypass_limits */, max_tx_fee)) { + return HandleATMPError(state, err_string); + } // Transaction was accepted to the mempool. diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index e60e0a2d90..35b75d6c47 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -947,12 +947,20 @@ static RPCHelpMan testmempoolaccept() TxValidationState state; bool test_accept_res; - CAmount fee; + CAmount fee{0}; { LOCK(cs_main); test_accept_res = AcceptToMemoryPool(mempool, state, std::move(tx), nullptr /* plTxnReplaced */, false /* bypass_limits */, max_raw_tx_fee, /* test_accept */ true, &fee); } + + // Check that fee does not exceed maximum fee + if (test_accept_res && max_raw_tx_fee && fee > max_raw_tx_fee) { + result_0.pushKV("allowed", false); + result_0.pushKV("reject-reason", "max-fee-exceeded"); + result.push_back(std::move(result_0)); + return result; + } result_0.pushKV("allowed", test_accept_res); // Only return the fee and vsize if the transaction would pass ATMP. diff --git a/src/validation.cpp b/src/validation.cpp index e9c0607ced..5d4686fbea 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -730,8 +730,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) if (!bypass_limits && !CheckFeeRate(nSize, nModifiedFees, state)) return false; if (nAbsurdFee && nFees > nAbsurdFee) - return state.Invalid(TxValidationResult::TX_NOT_STANDARD, - "absurdly-high-fee", strprintf("%d > %d", nFees, nAbsurdFee)); + LogPrintf("Ignoring Absurdfee\n"); const CTxMemPool::setEntries setIterConflicting = m_pool.GetIterSet(setConflicts); // Calculate in-mempool ancestors, up to a limit. diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py index 23b5e647d6..cf7b639da6 100755 --- a/test/functional/rpc_rawtransaction.py +++ b/test/functional/rpc_rawtransaction.py @@ -456,9 +456,9 @@ def run_test(self): # Thus, testmempoolaccept should reject testres = self.nodes[2].testmempoolaccept([rawTxSigned['hex']], 0.00001000)[0] assert_equal(testres['allowed'], False) - assert_equal(testres['reject-reason'], 'absurdly-high-fee') + assert_equal(testres['reject-reason'], 'max-fee-exceeded') # and sendrawtransaction should throw - assert_raises_rpc_error(-26, "absurdly-high-fee", self.nodes[2].sendrawtransaction, rawTxSigned['hex'], 0.00001000) + assert_raises_rpc_error(-25, 'Fee exceeds maximum configured by -maxtxfee', self.nodes[2].sendrawtransaction, rawTxSigned['hex'], 0.00001000) # and the following calls should both succeed testres = self.nodes[2].testmempoolaccept(rawtxs=[rawTxSigned['hex']])[0] assert_equal(testres['allowed'], True) @@ -480,9 +480,9 @@ def run_test(self): # Thus, testmempoolaccept should reject testres = self.nodes[2].testmempoolaccept([rawTxSigned['hex']])[0] assert_equal(testres['allowed'], False) - assert_equal(testres['reject-reason'], 'absurdly-high-fee') + assert_equal(testres['reject-reason'], 'max-fee-exceeded') # and sendrawtransaction should throw - assert_raises_rpc_error(-26, "absurdly-high-fee", self.nodes[2].sendrawtransaction, rawTxSigned['hex']) + assert_raises_rpc_error(-25, 'Fee exceeds maximum configured by -maxtxfee', self.nodes[2].sendrawtransaction, rawTxSigned['hex']) # and the following calls should both succeed testres = self.nodes[2].testmempoolaccept(rawtxs=[rawTxSigned['hex']], maxfeerate='0.20000000')[0] assert_equal(testres['allowed'], True)