diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 18dd2f010c..a42f817fff 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3694,10 +3694,10 @@ bool PeerLogicValidation::SendMessages(CNode* pto) if (fSendTrickle && pto->fSendMempool) { auto vtxinfo = mempool.infoAll(); pto->fSendMempool = false; - CAmount filterrate = 0; + CFeeRate filterrate; { LOCK(pto->cs_feeFilter); - filterrate = pto->minFeeFilter; + filterrate = CFeeRate(pto->minFeeFilter); } LOCK(pto->cs_filter); @@ -3706,9 +3706,9 @@ bool PeerLogicValidation::SendMessages(CNode* pto) const uint256& hash = txinfo.tx->GetHash(); CInv inv(MSG_TX, hash); pto->setInventoryTxToSend.erase(hash); - if (filterrate) { - if (txinfo.feeRate.GetFeePerK() < filterrate) - continue; + // Don't send transactions that peers will not put into their mempool + if (txinfo.fee < filterrate.GetFee(txinfo.vsize)) { + continue; } if (pto->pfilter) { if (!pto->pfilter->IsRelevantAndUpdate(*txinfo.tx)) continue; @@ -3731,10 +3731,10 @@ bool PeerLogicValidation::SendMessages(CNode* pto) for (std::set::iterator it = pto->setInventoryTxToSend.begin(); it != pto->setInventoryTxToSend.end(); it++) { vInvTx.push_back(it); } - CAmount filterrate = 0; + CFeeRate filterrate; { LOCK(pto->cs_feeFilter); - filterrate = pto->minFeeFilter; + filterrate = CFeeRate(pto->minFeeFilter); } // Topologically and fee-rate sort the inventory we send for privacy and priority reasons. // A heap is used so that not all items need sorting if only a few are being sent. @@ -3761,7 +3761,8 @@ bool PeerLogicValidation::SendMessages(CNode* pto) if (!txinfo.tx) { continue; } - if (filterrate && txinfo.feeRate.GetFeePerK() < filterrate) { + // Peer told you to not send transactions at that feerate? Don't bother sending it. + if (txinfo.fee < filterrate.GetFee(txinfo.vsize)) { continue; } if (pto->pfilter && !pto->pfilter->IsRelevantAndUpdate(*txinfo.tx)) continue; diff --git a/src/policy/feerate.h b/src/policy/feerate.h index 85d7d22b4f..d081f2ce8e 100644 --- a/src/policy/feerate.h +++ b/src/policy/feerate.h @@ -25,7 +25,7 @@ class CFeeRate /** Fee rate of 0 satoshis per kB */ CFeeRate() : nSatoshisPerK(0) { } template - CFeeRate(const I _nSatoshisPerK): nSatoshisPerK(_nSatoshisPerK) { + explicit CFeeRate(const I _nSatoshisPerK): nSatoshisPerK(_nSatoshisPerK) { // We've previously had bugs creep in from silent double->int conversion... static_assert(std::is_integral::value, "CFeeRate should be used without floats"); } diff --git a/src/policy/policy.h b/src/policy/policy.h index b01db3c271..8f8012f0ce 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -22,7 +22,7 @@ extern CAsset policyAsset; /** Default for -blockmaxweight, which controls the range of block weights the mining code will create **/ static const unsigned int DEFAULT_BLOCK_MAX_WEIGHT = MAX_BLOCK_WEIGHT - 4000; /** Default for -blockmintxfee, which sets the minimum feerate for a transaction in blocks created by mining code **/ -static const unsigned int DEFAULT_BLOCK_MIN_TX_FEE = 1000; +static const unsigned int DEFAULT_BLOCK_MIN_TX_FEE = 100; /** The maximum weight for transactions we're willing to relay/mine */ static const unsigned int MAX_STANDARD_TX_WEIGHT = 400000; /** The minimum non-witness size for transactions we're willing to relay/mine (1 segwit input + 1 P2WPKH output = 82 bytes) */ @@ -34,7 +34,7 @@ static const unsigned int MAX_STANDARD_TX_SIGOPS_COST = MAX_BLOCK_SIGOPS_COST/5; /** Default for -maxmempool, maximum megabytes of mempool memory usage */ static const unsigned int DEFAULT_MAX_MEMPOOL_SIZE = 300; /** Default for -incrementalrelayfee, which sets the minimum feerate increase for mempool limiting or BIP 125 replacement **/ -static const unsigned int DEFAULT_INCREMENTAL_RELAY_FEE = 1000; +static const unsigned int DEFAULT_INCREMENTAL_RELAY_FEE = 100; /** Default for -bytespersigop */ static const unsigned int DEFAULT_BYTES_PER_SIGOP = 20; /** The maximum number of witness stack items in a standard P2WSH script */ diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp index 29ff631d8d..abb954d7f7 100644 --- a/src/test/test_bitcoin.cpp +++ b/src/test/test_bitcoin.cpp @@ -60,6 +60,9 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::st // Set policy asset for correct fee output generation policyAsset = CAsset(); + // For unit tests, increase minrelay to "normal" 1000 sat/vkB + incrementalRelayFee = CFeeRate(1000); + noui_connect(); } diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 933e471eb7..93688ffd8d 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -855,7 +855,7 @@ void CTxMemPool::queryHashes(std::vector& vtxid) } static TxMempoolInfo GetInfo(CTxMemPool::indexed_transaction_set::const_iterator it) { - return TxMempoolInfo{it->GetSharedTx(), it->GetTime(), CFeeRate(it->GetFee(), it->GetTxSize()), it->GetModifiedFee() - it->GetFee()}; + return TxMempoolInfo{it->GetSharedTx(), it->GetTime(), it->GetFee(), it->GetTxSize(), it->GetModifiedFee() - it->GetFee()}; } std::vector CTxMemPool::infoAll() const diff --git a/src/txmempool.h b/src/txmempool.h index 7088c482b8..c5341be381 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -338,8 +338,11 @@ struct TxMempoolInfo /** Time the transaction entered the mempool. */ int64_t nTime; - /** Feerate of the transaction. */ - CFeeRate feeRate; + /** Fee of the transaction. */ + CAmount fee; + + /** Virtual size of the transaction. */ + size_t vsize; /** The fee delta. */ int64_t nFeeDelta; diff --git a/src/validation.h b/src/validation.h index 553b2a449a..1fae307d6b 100644 --- a/src/validation.h +++ b/src/validation.h @@ -55,7 +55,7 @@ static const bool DEFAULT_WHITELISTRELAY = true; /** Default for -whitelistforcerelay. */ static const bool DEFAULT_WHITELISTFORCERELAY = false; /** Default for -minrelaytxfee, minimum relay fee for transactions */ -static const unsigned int DEFAULT_MIN_RELAY_TX_FEE = 1000; +static const unsigned int DEFAULT_MIN_RELAY_TX_FEE = 100; //! -maxtxfee default static const CAmount DEFAULT_TRANSACTION_MAXFEE = COIN / 10; //! Discourage users to set fees higher than this amount (in satoshis) per kB diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 69dec5f75f..0be74d57d2 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2625,7 +2625,7 @@ static UniValue settxfee(const JSONRPCRequest& request) CAmount nAmount = AmountFromValue(request.params[0]); CFeeRate tx_fee_rate(nAmount, 1000); - if (tx_fee_rate == 0) { + if (tx_fee_rate == CFeeRate(0)) { // automatic selection } else if (tx_fee_rate < ::minRelayTxFee) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be less than min relay tx fee (%s)", ::minRelayTxFee.ToString())); diff --git a/test/bitcoin_functional/functional/test_framework/util.py b/test/bitcoin_functional/functional/test_framework/util.py index c6a5e9c530..303f501488 100644 --- a/test/bitcoin_functional/functional/test_framework/util.py +++ b/test/bitcoin_functional/functional/test_framework/util.py @@ -323,6 +323,7 @@ def initialize_datadir(dirname, n): f.write("scriptprefix=196\n") f.write("bech32_hrp=bcrt\n") f.write("con_dyna_deploy_start="+str(2**31)+"\n") # Never starts + f.write("minrelaytxfee=0.00001\n") os.makedirs(os.path.join(datadir, 'stderr'), exist_ok=True) os.makedirs(os.path.join(datadir, 'stdout'), exist_ok=True) return datadir diff --git a/test/functional/p2p_feefilter.py b/test/functional/p2p_feefilter.py index d589519e45..183dce8518 100755 --- a/test/functional/p2p_feefilter.py +++ b/test/functional/p2p_feefilter.py @@ -41,6 +41,12 @@ def clear_invs(self): class FeeFilterTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 2 + # We lower the various required feerates for this test + # to catch a corner-case where feefilter used to slightly undercut + # mempool and wallet feerate calculation based on GetFee + # rounding down 3 places, leading to stranded transactions. + # See issue #16499 + self.extra_args = [["-minrelaytxfee=0.00000100", "-mintxfee=0.00000100"]]*self.num_nodes def skip_test_if_missing_module(self): self.skip_if_no_wallet() @@ -54,22 +60,25 @@ def run_test(self): self.nodes[0].add_p2p_connection(TestP2PConn()) - # Test that invs are received for all txs at feerate of 20 sat/byte - node1.settxfee(Decimal("0.00020000")) + # Test that invs are received by test connection for all txs at + # feerate of .2 sat/byte + node1.settxfee(Decimal("0.00000200")) txids = [node1.sendtoaddress(node1.getnewaddress(), 1) for x in range(3)] assert(allInvsMatch(txids, self.nodes[0].p2p)) self.nodes[0].p2p.clear_invs() - # Set a filter of 15 sat/byte - self.nodes[0].p2p.send_and_ping(msg_feefilter(15000)) + # Set a filter of .15 sat/byte on test connection + self.nodes[0].p2p.send_and_ping(msg_feefilter(150)) - # Test that txs are still being received (paying 20 sat/byte) + # Test that txs are still being received by test connection (paying .15 sat/byte) + node1.settxfee(Decimal("0.00000150")) txids = [node1.sendtoaddress(node1.getnewaddress(), 1) for x in range(3)] assert(allInvsMatch(txids, self.nodes[0].p2p)) self.nodes[0].p2p.clear_invs() - # Change tx fee rate to 10 sat/byte and test they are no longer received - node1.settxfee(Decimal("0.00010000")) + # Change tx fee rate to .1 sat/byte and test they are no longer received + # by the test connection + node1.settxfee(Decimal("0.00000100")) [node1.sendtoaddress(node1.getnewaddress(), 1) for x in range(3)] sync_mempools(self.nodes) # must be sure node 0 has received all txs diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index e5f970900a..d2b9bd8925 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -331,6 +331,7 @@ def initialize_datadir(dirname, n, chain): f.write("con_csv_deploy_start=0\n") # Enhance tests if removing this line f.write("blindedaddresses=0\n") # Set to minimize broken tests in favor of custom f.write("con_dyna_deploy_start="+str(2**31)+"\n") # Never starts unless overridden + f.write("minrelaytxfee=0.00001\n") #f.write("pubkeyprefix=111\n") #f.write("scriptprefix=196\n") #f.write("bech32_hrp=bcrt\n")