Merge #16507: feefilter: Compute the absolute fee rather than stored rate

eb7b78165966f2c79da71b993c4c4d793e37297f modify p2p_feefilter test to catch rounding error (Gregory Sanders)
6a51f7951716d6d6fc0f9b56028f3a0dd02b61c8 Disallow implicit conversion for CFeeRate constructor (Gregory Sanders)
8e59af55aaf1b196575084bce2448af02d97d745 feefilter: Compute the absolute fee rather than stored rate to match mempool acceptance logic (Gregory Sanders)

Pull request description:

  This means we will use the rounding-down behavior in `GetFee` to match both mempool acceptance and wallet logic, with minimal changes.

  Fixes https://github.com/bitcoin/bitcoin/issues/16499

  Replacement PR for https://github.com/bitcoin/bitcoin/pull/16500

ACKs for top commit:
  ajtowns:
    ACK eb7b78165966f2c79da71b993c4c4d793e37297f code review only
  naumenkogs:
    utACK eb7b78165966f2c79da71b993c4c4d793e37297f
  achow101:
    re ACK eb7b78165966f2c79da71b993c4c4d793e37297f
  promag:
    ACK eb7b78165966f2c79da71b993c4c4d793e37297f.

Tree-SHA512: 484a11c8f0e825f0c983b1f7e71cf6252b1bba6858194abfe4c088da3bae8a418ec539ef6c4181bf30940e277a95c08d493595d59dfcc6ddf77c65b05563dd7e

6 files changed, 34 insertions, 21 deletions

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index e345af604c..9aa0294c27 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<uint256>::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/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 @@ public:
     /** Fee rate of 0 satoshis per kB */
     CFeeRate() : nSatoshisPerK(0) { }
     template<typename I>
-    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<I>::value, "CFeeRate should be used without floats");
     }
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 835b8d63bd..e4c1fd4bc6 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -773,7 +773,7 @@ void CTxMemPool::queryHashes(std::vector<uint256>& 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<TxMempoolInfo> CTxMemPool::infoAll() const
diff --git a/src/txmempool.h b/src/txmempool.h
index f2fc1c8310..229a923a28 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;
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 96fa50d42e..0904c03669 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;
diff --git a/test/functional/p2p_feefilter.py b/test/functional/p2p_feefilter.py
index 7f901b1886..4f242bd94a 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