From 8e59af55aaf1b196575084bce2448af02d97d745 Mon Sep 17 00:00:00 2001 From: Gregory Sanders Date: Wed, 31 Jul 2019 13:21:00 -0400 Subject: [PATCH 1/3] feefilter: Compute the absolute fee rather than stored rate to match mempool acceptance logic --- src/net_processing.cpp | 17 +++++++++-------- src/txmempool.cpp | 2 +- src/txmempool.h | 7 +++++-- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e345af604..9aa0294c2 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3847,10 +3847,10 @@ bool PeerLogicValidation::SendMessages(CNode* pto) if (fSendTrickle && pto->m_tx_relay->fSendMempool) { auto vtxinfo = mempool.infoAll(); pto->m_tx_relay->fSendMempool = false; - CAmount filterrate = 0; + CFeeRate filterrate; { LOCK(pto->m_tx_relay->cs_feeFilter); - filterrate = pto->m_tx_relay->minFeeFilter; + filterrate = CFeeRate(pto->m_tx_relay->minFeeFilter); } LOCK(pto->m_tx_relay->cs_filter); @@ -3859,9 +3859,9 @@ bool PeerLogicValidation::SendMessages(CNode* pto) const uint256& hash = txinfo.tx->GetHash(); CInv inv(MSG_TX, hash); pto->m_tx_relay->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->m_tx_relay->pfilter) { if (!pto->m_tx_relay->pfilter->IsRelevantAndUpdate(*txinfo.tx)) continue; @@ -3884,10 +3884,10 @@ bool PeerLogicValidation::SendMessages(CNode* pto) for (std::set::iterator it = pto->m_tx_relay->setInventoryTxToSend.begin(); it != pto->m_tx_relay->setInventoryTxToSend.end(); it++) { vInvTx.push_back(it); } - CAmount filterrate = 0; + CFeeRate filterrate; { LOCK(pto->m_tx_relay->cs_feeFilter); - filterrate = pto->m_tx_relay->minFeeFilter; + filterrate = CFeeRate(pto->m_tx_relay->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. @@ -3914,7 +3914,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->m_tx_relay->pfilter && !pto->m_tx_relay->pfilter->IsRelevantAndUpdate(*txinfo.tx)) continue; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 835b8d63b..e4c1fd4bc 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -773,7 +773,7 @@ void CTxMemPool::queryHashes(std::vector& vtxid) const } 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 f2fc1c831..229a923a2 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -334,8 +334,11 @@ struct TxMempoolInfo /** Time the transaction entered the mempool. */ std::chrono::seconds m_time; - /** 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; From 6a51f7951716d6d6fc0f9b56028f3a0dd02b61c8 Mon Sep 17 00:00:00 2001 From: Gregory Sanders Date: Tue, 20 Aug 2019 09:58:35 -0400 Subject: [PATCH 2/3] Disallow implicit conversion for CFeeRate constructor --- src/policy/feerate.h | 2 +- src/wallet/rpcwallet.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/policy/feerate.h b/src/policy/feerate.h index 85d7d22b4..d081f2ce8 100644 --- a/src/policy/feerate.h +++ b/src/policy/feerate.h @@ -25,7 +25,7 @@ public: /** 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/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 96fa50d42..0904c0366 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2327,7 +2327,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 < pwallet->chain().relayMinFee()) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be less than min relay tx fee (%s)", pwallet->chain().relayMinFee().ToString())); @@ -3386,7 +3386,7 @@ static UniValue bumpfee(const JSONRPCRequest& request) } } else if (options.exists("fee_rate")) { CFeeRate fee_rate(AmountFromValue(options["fee_rate"])); - if (fee_rate <= 0) { + if (fee_rate <= CFeeRate(0)) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid fee_rate %s (must be greater than 0)", fee_rate.ToString())); } coin_control.m_feerate = fee_rate; From eb7b78165966f2c79da71b993c4c4d793e37297f Mon Sep 17 00:00:00 2001 From: Gregory Sanders Date: Wed, 21 Aug 2019 16:36:42 -0400 Subject: [PATCH 3/3] modify p2p_feefilter test to catch rounding error --- test/functional/p2p_feefilter.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/test/functional/p2p_feefilter.py b/test/functional/p2p_feefilter.py index 7f901b188..4f242bd94 100755 --- a/test/functional/p2p_feefilter.py +++ b/test/functional/p2p_feefilter.py @@ -41,6 +41,12 @@ class TestP2PConn(P2PInterface): 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 @@ class FeeFilterTest(BitcoinTestFramework): 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)] self.sync_mempools() # must be sure node 0 has received all txs