Merge #684: Reduce relay min rate to 100 sat/vkB

bf2f1c621 Reduce min relay to 100 sat/vkB (Gregory Sanders)
d8dc3f2f2 modify p2p_feefilter test to catch rounding error (Gregory Sanders)
130d190e7 Disallow implicit conversion for CFeeRate constructor (Gregory Sanders)
36fceda4a feefilter: Compute the absolute fee rather than stored rate to match mempool acceptance logic (Gregory Sanders)

Pull request description:

  Includes an upstream bugfix for feefilter/mempool divergence when the rate is not `%1000=0`.

  Does not lower the minimum wallet default! This must be chosen manually for the time being(`settxfee`), at the risk of not propagating to the rest of the network if peers have not also updated. Fee estimation is also not yet updated to support lower feerates.

Tree-SHA512: 561b47d6f5fd898eec56db41189a6c514e54682b817996f2e188d791a9234102df52bd191e02dced468df30da5816b9acfde53154939180db9bf23bd20256a53

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<uint256>::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;

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");
     }

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 */

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();
 }
 

src/txmempool.cpp

@@ -855,7 +855,7 @@ void CTxMemPool::queryHashes(std::vector<uint256>& 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<TxMempoolInfo> CTxMemPool::infoAll() const

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;

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

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()));

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

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)]
         sync_mempools(self.nodes) # must be sure node 0 has received all txs
 

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")