diff --git a/doc/release-notes.md b/doc/release-notes.md --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -6,3 +6,8 @@ - Fixed a bug with Multiwallets that have their own directories (i.e. cases such as `DATADIR/wallets/mywallet/wallet.dat`). Backups of these wallets will now take each wallet's specific directory into account. + - When transactions are rejected for being low fee, the error message will + read "min relay fee not met" instead of "insufficient priority". Also, + a bug was fixed where sometimes low fee transactions would be accepted via + `sendrawtransaction` (and be stuck semipermanently, as they would be unable + to relay to miners). diff --git a/src/init.cpp b/src/init.cpp --- a/src/init.cpp +++ b/src/init.cpp @@ -847,16 +847,6 @@ "fees at this fee rate to spend it. (default: %s)", CURRENCY_UNIT, FormatMoney(DUST_RELAY_TX_FEE)), true, OptionsCategory::NODE_RELAY); - gArgs.AddArg("-limitfreerelay=", - strprintf("Continuously rate-limit free transactions to " - "*1000 bytes per minute (default: %u)", - DEFAULT_LIMITFREERELAY), - true, OptionsCategory::NODE_RELAY); - gArgs.AddArg("-relaypriority", - strprintf("Require high priority for relaying free or low-fee " - "transactions (default: %d)", - DEFAULT_RELAYPRIORITY), - true, OptionsCategory::NODE_RELAY); gArgs.AddArg("-bytespersigop", strprintf("Equivalent bytes per sigop in transactions for " @@ -875,8 +865,8 @@ false, OptionsCategory::NODE_RELAY); gArgs.AddArg( "-minrelaytxfee=", - strprintf("Fees (in %s/kB) smaller than this are considered zero fee " - "for relaying, mining and transaction creation (default: %s)", + strprintf("Fees (in %s/kB) smaller than this are rejected for " + "relaying, mining and transaction creation (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_MIN_RELAY_TX_FEE_PER_KB)), false, OptionsCategory::NODE_RELAY); gArgs.AddArg( diff --git a/src/miner.cpp b/src/miner.cpp --- a/src/miner.cpp +++ b/src/miner.cpp @@ -584,8 +584,7 @@ } void BlockAssembler::addPriorityTxs() { - // How much of the block should be dedicated to high-priority transactions, - // included regardless of the fees they pay. + // How much of the block should be dedicated to high-priority transactions. if (nBlockPriorityPercentage == 0) { return; } @@ -656,12 +655,6 @@ break; } - // if we have dropped below the AllowFreeThreshold, then we're done - // adding priority transactions. - if (!AllowFree(actualPriority)) { - break; - } - // This tx was successfully added, so add transactions that depend // on this one to the priority queue to try again. for (CTxMemPool::txiter child : mempool->GetMemPoolChildren(iter)) { diff --git a/src/net_processing.cpp b/src/net_processing.cpp --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2787,9 +2787,8 @@ nodestate->m_tx_download.m_tx_in_flight.erase(txid); EraseTxRequest(txid); - if (!AlreadyHave(inv) && - AcceptToMemoryPool(config, g_mempool, state, ptx, true, - &fMissingInputs)) { + if (!AlreadyHave(inv) && AcceptToMemoryPool(config, g_mempool, state, + ptx, &fMissingInputs)) { g_mempool.check(pcoinsTip.get()); RelayTransaction(tx, connman); for (size_t i = 0; i < tx.vout.size(); i++) { @@ -2834,7 +2833,7 @@ } if (AcceptToMemoryPool(config, g_mempool, stateDummy, - porphanTx, true, &fMissingInputs2)) { + porphanTx, &fMissingInputs2)) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanId.ToString()); RelayTransaction(orphanTx, connman); @@ -4726,12 +4725,7 @@ CFeeRate(DEFAULT_MIN_RELAY_TX_FEE_PER_KB); static FeeFilterRounder filterRounder(default_feerate); Amount filterToSend = filterRounder.round(currentFilter); - // If we don't allow free transactions, then we always have a fee - // filter of at least minRelayTxFee - if (gArgs.GetArg("-limitfreerelay", DEFAULT_LIMITFREERELAY) <= 0) { - filterToSend = - std::max(filterToSend, ::minRelayTxFee.GetFeePerK()); - } + filterToSend = std::max(filterToSend, ::minRelayTxFee.GetFeePerK()); if (filterToSend != pto->lastSentFeeFilter) { connman->PushMessage( diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -549,7 +549,7 @@ " ,...\n" " ],\n" " \"relayfee\": x.xxxxxxxx, (numeric) minimum " - "relay fee for non-free transactions in " + + "relay fee for transactions in " + CURRENCY_UNIT + "/kB\n" " \"excessutxocharge\": x.xxxxxxxx, (numeric) minimum " diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1179,10 +1179,8 @@ // Push to local node and sync with wallets. CValidationState state; bool fMissingInputs; - bool fLimitFree = false; if (!AcceptToMemoryPool(config, g_mempool, state, std::move(tx), - fLimitFree, &fMissingInputs, false, - nMaxRawTxFee)) { + &fMissingInputs, false, nMaxRawTxFee)) { if (state.IsInvalid()) { throw JSONRPCError(RPC_TRANSACTION_REJECTED, FormatStateMessage(state)); @@ -1278,7 +1276,6 @@ CTransactionRef tx(MakeTransactionRef(std::move(mtx))); const uint256 &txid = tx->GetId(); - bool fLimitFree = false; Amount max_raw_tx_fee = maxTxFee; if (!request.params[1].isNull() && request.params[1].get_bool()) { max_raw_tx_fee = Amount::zero(); @@ -1294,8 +1291,8 @@ { LOCK(cs_main); test_accept_res = AcceptToMemoryPool( - config, g_mempool, state, std::move(tx), fLimitFree, - &missing_inputs, /* bypass_limits */ false, max_raw_tx_fee, + config, g_mempool, state, std::move(tx), &missing_inputs, + /* bypass_limits */ false, max_raw_tx_fee, /* test_accept */ true); } result_0.pushKV("allowed", test_accept_res); diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -38,12 +38,12 @@ unsigned int initialPoolSize = g_mempool.size(); - BOOST_CHECK_EQUAL( - false, - AcceptToMemoryPool( - GetConfig(), g_mempool, state, MakeTransactionRef(coinbaseTx), - false /* pfMissingInputs */, nullptr /* plTxnReplaced */, - true /* bypass_limits */, Amount::zero() /* nAbsurdFee */)); + BOOST_CHECK_EQUAL(false, + AcceptToMemoryPool(GetConfig(), g_mempool, state, + MakeTransactionRef(coinbaseTx), + nullptr /* pfMissingInputs */, + true /* fOverrideMempoolLimit */, + Amount::zero() /* nAbsurdFee */)); // Check that the transaction hasn't been added to mempool. BOOST_CHECK_EQUAL(g_mempool.size(), initialPoolSize); diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -31,7 +31,7 @@ CValidationState state; return AcceptToMemoryPool(GetConfig(), g_mempool, state, - MakeTransactionRef(tx), false, nullptr, true, + MakeTransactionRef(tx), nullptr, true, Amount::zero()); } diff --git a/src/txmempool.h b/src/txmempool.h --- a/src/txmempool.h +++ b/src/txmempool.h @@ -31,15 +31,6 @@ extern CCriticalSection cs_main; -inline double AllowFreeThreshold() { - return (144 * COIN) / (250 * SATOSHI); -} - -inline bool AllowFree(double dPriority) { - // Large (in bytes) low-priority (new, small-coin) transactions need a fee. - return dPriority > AllowFreeThreshold(); -} - /** * Fake height value used in Coins to signify they are only in the memory * pool(since 0.8) diff --git a/src/txmempool.cpp b/src/txmempool.cpp --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1385,8 +1385,8 @@ // ignore validation errors in resurrected transactions CValidationState stateDummy; if (!fAddToMempool || tx->IsCoinBase() || - !AcceptToMemoryPool(config, g_mempool, stateDummy, tx, false, - nullptr, true)) { + !AcceptToMemoryPool(config, g_mempool, stateDummy, tx, nullptr, + true)) { // If the transaction doesn't make it in to the mempool, remove any // transactions that depend on it (which would now be orphans). g_mempool.removeRecursive(*tx, MemPoolRemovalReason::REORG); diff --git a/src/validation.h b/src/validation.h --- a/src/validation.h +++ b/src/validation.h @@ -146,8 +146,6 @@ */ static const int64_t BLOCK_DOWNLOAD_TIMEOUT_PER_PEER = 500000; -static const unsigned int DEFAULT_LIMITFREERELAY = 0; -static const bool DEFAULT_RELAYPRIORITY = true; static const int64_t DEFAULT_MAX_TIP_AGE = 24 * 60 * 60; /** * Maximum age of our tip in seconds for us to be considered current for fee @@ -451,7 +449,7 @@ */ bool AcceptToMemoryPool(const Config &config, CTxMemPool &pool, CValidationState &state, const CTransactionRef &tx, - bool fLimitFree, bool *pfMissingInputs, + bool *pfMissingInputs, bool fOverrideMempoolLimit = false, const Amount nAbsurdFee = Amount::zero(), bool test_accept = false) diff --git a/src/validation.cpp b/src/validation.cpp --- a/src/validation.cpp +++ b/src/validation.cpp @@ -539,12 +539,13 @@ txdata); } -static bool AcceptToMemoryPoolWorker( - const Config &config, CTxMemPool &pool, CValidationState &state, - const CTransactionRef &ptx, bool fLimitFree, bool *pfMissingInputs, - int64_t nAcceptTime, bool fOverrideMempoolLimit, const Amount nAbsurdFee, - std::vector &coins_to_uncache, bool test_accept) - EXCLUSIVE_LOCKS_REQUIRED(cs_main) { +static bool +AcceptToMemoryPoolWorker(const Config &config, CTxMemPool &pool, + CValidationState &state, const CTransactionRef &ptx, + bool *pfMissingInputs, int64_t nAcceptTime, + bool fOverrideMempoolLimit, const Amount nAbsurdFee, + std::vector &coins_to_uncache, + bool test_accept) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { AssertLockHeld(cs_main); const Consensus::Params &consensusParams = @@ -712,59 +713,26 @@ strprintf("%d", nSigOpsCount)); } + // No transactions are allowed below minRelayTxFee except from + // disconnected blocks + if (!fOverrideMempoolLimit && + nModifiedFees < minRelayTxFee.GetFee(nSize)) { + return state.DoS(0, false, REJECT_INSUFFICIENTFEE, + "min relay fee not met"); + } + Amount mempoolRejectFee = pool.GetMinFee( gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000) .GetFee(nSize); - if (mempoolRejectFee > Amount::zero() && + if (!fOverrideMempoolLimit && mempoolRejectFee > Amount::zero() && nModifiedFees < mempoolRejectFee) { return state.DoS( 0, false, REJECT_INSUFFICIENTFEE, "mempool min fee not met", false, strprintf("%d < %d", nModifiedFees, mempoolRejectFee)); } - if (gArgs.GetBoolArg("-relaypriority", DEFAULT_RELAYPRIORITY) && - nModifiedFees < minRelayTxFee.GetFee(nSize) && - !AllowFree(entry.GetPriority(chainActive.Height() + 1))) { - // Require that free transactions have sufficient priority to be - // mined in the next block. - return state.DoS(0, false, REJECT_INSUFFICIENTFEE, - "insufficient priority"); - } - - // Continuously rate-limit free (really, very-low-fee) transactions. - // This mitigates 'penny-flooding' -- sending thousands of free - // transactions just to be annoying or make others' transactions take - // longer to confirm. - if (fLimitFree && nModifiedFees < minRelayTxFee.GetFee(nSize)) { - static CCriticalSection csFreeLimiter; - static double dFreeCount; - static int64_t nLastTime; - int64_t nNow = GetTime(); - - LOCK(csFreeLimiter); - - // Use an exponentially decaying ~10-minute window: - dFreeCount *= pow(1.0 - 1.0 / 600.0, double(nNow - nLastTime)); - nLastTime = nNow; - // -limitfreerelay unit is thousand-bytes-per-minute - // At default rate it would take over a month to fill 1GB - - // NOTE: Use the actual size here, and not the fee size since this - // is counting real size for the rate limiter. - if (dFreeCount + nSize >= - gArgs.GetArg("-limitfreerelay", DEFAULT_LIMITFREERELAY) * 10 * - 1000) { - return state.DoS(0, false, REJECT_INSUFFICIENTFEE, - "rate limited free transaction"); - } - - LogPrint(BCLog::MEMPOOL, "Rate limit dFreeCount: %g => %g\n", - dFreeCount, dFreeCount + nSize); - dFreeCount += nSize; - } - if (nAbsurdFee != Amount::zero() && nFees > nAbsurdFee) { return state.Invalid(false, REJECT_HIGHFEE, "absurdly-high-fee", strprintf("%d > %d", nFees, nAbsurdFee)); @@ -864,15 +832,16 @@ /** * (try to) add transaction to memory pool with a specified acceptance time. */ -static bool AcceptToMemoryPoolWithTime( - const Config &config, CTxMemPool &pool, CValidationState &state, - const CTransactionRef &tx, bool fLimitFree, bool *pfMissingInputs, - int64_t nAcceptTime, bool fOverrideMempoolLimit, const Amount nAbsurdFee, - bool test_accept) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { +static bool +AcceptToMemoryPoolWithTime(const Config &config, CTxMemPool &pool, + CValidationState &state, const CTransactionRef &tx, + bool *pfMissingInputs, int64_t nAcceptTime, + bool fOverrideMempoolLimit, const Amount nAbsurdFee, + bool test_accept) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { AssertLockHeld(cs_main); std::vector coins_to_uncache; bool res = AcceptToMemoryPoolWorker( - config, pool, state, tx, fLimitFree, pfMissingInputs, nAcceptTime, + config, pool, state, tx, pfMissingInputs, nAcceptTime, fOverrideMempoolLimit, nAbsurdFee, coins_to_uncache, test_accept); if (!res) { for (const COutPoint &outpoint : coins_to_uncache) { @@ -890,12 +859,11 @@ bool AcceptToMemoryPool(const Config &config, CTxMemPool &pool, CValidationState &state, const CTransactionRef &tx, - bool fLimitFree, bool *pfMissingInputs, - bool fOverrideMempoolLimit, const Amount nAbsurdFee, - bool test_accept) { - return AcceptToMemoryPoolWithTime( - config, pool, state, tx, fLimitFree, pfMissingInputs, GetTime(), - fOverrideMempoolLimit, nAbsurdFee, test_accept); + bool *pfMissingInputs, bool fOverrideMempoolLimit, + const Amount nAbsurdFee, bool test_accept) { + return AcceptToMemoryPoolWithTime(config, pool, state, tx, pfMissingInputs, + GetTime(), fOverrideMempoolLimit, + nAbsurdFee, test_accept); } /** @@ -5687,9 +5655,8 @@ if (nTime + nExpiryTimeout > nNow) { LOCK(cs_main); AcceptToMemoryPoolWithTime( - config, pool, state, tx, true /* fLimitFree */, - nullptr /* pfMissingInputs */, nTime, - false /* fOverrideMempoolLimit */, + config, pool, state, tx, nullptr /* pfMissingInputs */, + nTime, false /* fOverrideMempoolLimit */, Amount::zero() /* nAbsurdFee */, false /* test_accept */); if (state.IsValid()) { ++count; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4813,9 +4813,8 @@ // because we think that this newly generated transaction's change is // unavailable as we're not yet aware that it is in the mempool. bool ret = ::AcceptToMemoryPool( - GetConfig(), g_mempool, state, tx, true /* fLimitFree */, - nullptr /* pfMissingInputs */, false /* fOverrideMempoolLimit */, - nAbsurdFee); + GetConfig(), g_mempool, state, tx, nullptr /* pfMissingInputs */, + false /* fOverrideMempoolLimit */, nAbsurdFee); fInMempool |= ret; return ret; } diff --git a/test/functional/abc-high_priority_transaction.py b/test/functional/abc-high_priority_transaction.py deleted file mode 100755 --- a/test/functional/abc-high_priority_transaction.py +++ /dev/null @@ -1,94 +0,0 @@ -#!/usr/bin/env python3 -# Copyright (c) 2017 The Bitcoin developers -# Distributed under the MIT software license, see the accompanying -# file COPYING or http://www.opensource.org/licenses/mit-license.php. - -# -# Test HighPriorityTransaction code -# - -from test_framework.blocktools import create_confirmed_utxos -from test_framework.messages import COIN -from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import assert_equal, satoshi_round - - -class HighPriorityTransactionTest(BitcoinTestFramework): - - def set_test_params(self): - self.num_nodes = 1 - self.extra_args = [["-blockprioritypercentage=0", "-limitfreerelay=2"]] - - def create_small_transactions(self, node, utxos, num, fee): - addr = node.getnewaddress() - txids = [] - for _ in range(num): - t = utxos.pop() - inputs = [{"txid": t["txid"], "vout": t["vout"]}] - outputs = {} - change = t['amount'] - fee - outputs[addr] = satoshi_round(change) - rawtx = node.createrawtransaction(inputs, outputs) - signresult = node.signrawtransactionwithwallet( - rawtx, None, "NONE|FORKID") - txid = node.sendrawtransaction(signresult["hex"], True) - txids.append(txid) - return txids - - def generate_high_priotransactions(self, node, count): - # create 150 simple one input one output hi prio txns - hiprio_utxo_count = 150 - age = 250 - # be sure to make this utxo aged enough - hiprio_utxos = create_confirmed_utxos(node, hiprio_utxo_count, age) - - # Create hiprio_utxo_count number of txns with 0 fee - txids = self.create_small_transactions( - node, hiprio_utxos, hiprio_utxo_count, 0) - return txids - - def run_test(self): - # this is the priority cut off as defined in AllowFreeThreshold() (see: src/txmempool.h) - # anything above that value is considered an high priority transaction - hiprio_threshold = COIN * 144 / 250 - self.relayfee = self.nodes[0].getnetworkinfo()['relayfee'] - - # first test step: 0 reserved prio space in block - txids = self.generate_high_priotransactions(self.nodes[0], 150) - mempool_size_pre = self.nodes[0].getmempoolinfo()['bytes'] - mempool = self.nodes[0].getrawmempool(True) - # assert that all the txns are in the mempool and that all of them are hi prio - for i in txids: - assert i in mempool - assert mempool[i]['currentpriority'] > hiprio_threshold - - # mine one block - self.nodes[0].generate(1) - - self.log.info( - "Assert that all high prio transactions haven't been mined") - assert_equal(self.nodes[0].getmempoolinfo()['bytes'], mempool_size_pre) - - # restart with default blockprioritypercentage - self.restart_node(0, ["-limitfreerelay=2"]) - - # second test step: default reserved prio space in block (100K). - # the mempool size is about 25K this means that all txns will be - # included in the soon to be mined block - txids = self.generate_high_priotransactions(self.nodes[0], 150) - mempool_size_pre = self.nodes[0].getmempoolinfo()['bytes'] - mempool = self.nodes[0].getrawmempool(True) - # assert that all the txns are in the mempool and that all of them are hiprio - for i in txids: - assert i in mempool - assert mempool[i]['currentpriority'] > hiprio_threshold - - # mine one block - self.nodes[0].generate(1) - - self.log.info("Assert that all high prio transactions have been mined") - assert self.nodes[0].getmempoolinfo()['bytes'] == 0 - - -if __name__ == '__main__': - HighPriorityTransactionTest().main() diff --git a/test/functional/abc-p2p-compactblocks.py b/test/functional/abc-p2p-compactblocks.py --- a/test/functional/abc-p2p-compactblocks.py +++ b/test/functional/abc-p2p-compactblocks.py @@ -91,10 +91,7 @@ self.tip = None self.blocks = {} self.excessive_block_size = 16 * ONE_MEGABYTE - # -norelaypriority is needed so that the invalidateblock at the end - # will insert free transactions back into the mempool. - self.extra_args = [['-norelaypriority', - '-whitelist=127.0.0.1', + self.extra_args = [['-whitelist=127.0.0.1', '-limitancestorcount=999999', '-limitancestorsize=999999', '-limitdescendantcount=999999', diff --git a/test/functional/mining_prioritisetransaction.py b/test/functional/mining_prioritisetransaction.py --- a/test/functional/mining_prioritisetransaction.py +++ b/test/functional/mining_prioritisetransaction.py @@ -134,7 +134,7 @@ tx2_id = self.nodes[0].decoderawtransaction(tx2_hex)["txid"] # This will raise an exception due to min relay fee not being met - assert_raises_rpc_error(-26, "insufficient priority (code 66)", + assert_raises_rpc_error(-26, "min relay fee not met (code 66)", self.nodes[0].sendrawtransaction, tx2_hex) assert tx2_id not in self.nodes[0].getrawmempool() diff --git a/test/functional/timing.json b/test/functional/timing.json --- a/test/functional/timing.json +++ b/test/functional/timing.json @@ -11,10 +11,6 @@ "name": "abc-get-invalid-block.py", "time": 2 }, - { - "name": "abc-high_priority_transaction.py", - "time": 5 - }, { "name": "abc-invalid-chains.py", "time": 1