Merge #19204: p2p: Reduce inv traffic during IBD

fa525e4d1cfda8c1924d2c69f43bd7ae3b98fb72 net: Avoid wasting inv traffic during IBD (MarcoFalke)
fa06d7e93489e61078cfb95ab767c001536a6e10 refactor: block import implies IsInitialBlockDownload (MarcoFalke)
faba65e696a88e5626e587f4e63fa15500cbe4d0 Add ChainstateManager::ActiveChainstate (MarcoFalke)
fabf3d64ff2bd14f762810316144bb9fd69c517c test: Add FeeFilterRounder test (MarcoFalke)

Pull request description:

  Tx-inv messages are ignored during IBD, so it would be nice if we told peers to not send them in the first place. Do that by sending two `feefilter` messages: One when the connection is made (and the node is in IBD), and another one when the node leaves IBD.

ACKs for top commit:
  jamesob:
    ACK fa525e4d1cfda8c1924d2c69f43bd7ae3b98fb72 ([`jamesob/ackr/19204.1.MarcoFalke.p2p_reduce_inv_traffic_d`](https://github.com/jamesob/bitcoin/tree/ackr/19204.1.MarcoFalke.p2p_reduce_inv_traffic_d))
  naumenkogs:
    utACK fa525e4
  gzhao408:
    ACK https://github.com/bitcoin/bitcoin/commit/fa525e4d1cfda8c1924d2c69f43bd7ae3b98fb72
  jonatack:
    re-ACK fa525e4 checked diff `git range-diff 19612ca fa8a66c fa525e4`, re-reviewed, ran tests, ran a custom p2p IBD behavior test at https://github.com/jonatack/bitcoin/commit/9321e0f223ea87d21f6fa42b61bcc8d40a0943de.
  hebasto:
    re-ACK fa525e4d1cfda8c1924d2c69f43bd7ae3b98fb72, only rebased since the [previous](https://github.com/bitcoin/bitcoin/pull/19204#pullrequestreview-429519667) review (verified with `git range-diff`).

Tree-SHA512: 2c22a5def9822396fca45d808b165b636f1143c4bdb2eaa5c7e977f1f18e8b10c86d4c180da488def38416cf3076a26de15014dfd4d86b2a7e5af88c74afb8eb

6 files changed, 60 insertions, 11 deletions

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index 472382c7d2..9dc3078487 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -232,6 +232,7 @@ BITCOIN_TESTS =\
   test/net_tests.cpp \
   test/netbase_tests.cpp \
   test/pmt_tests.cpp \
+  test/policy_fee_tests.cpp \
   test/policyestimator_tests.cpp \
   test/pow_tests.cpp \
   test/prevector_tests.cpp \
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index b5912e18a7..80e58a6dba 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2576,7 +2576,7 @@ void ProcessMessage(
                     LogPrint(BCLog::NET, "transaction (%s) inv sent in violation of protocol, disconnecting peer=%d\n", inv.hash.ToString(), pfrom.GetId());
                     pfrom.fDisconnect = true;
                     return;
-                } else if (!fAlreadyHave && !fImporting && !fReindex && !::ChainstateActive().IsInitialBlockDownload()) {
+                } else if (!fAlreadyHave && !chainman.ActiveChainstate().IsInitialBlockDownload()) {
                     RequestTx(State(pfrom.GetId()), inv.hash, current_time);
                 }
             }
@@ -4392,10 +4392,21 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
         ) {
             CAmount currentFilter = m_mempool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK();
             int64_t timeNow = GetTimeMicros();
+            static FeeFilterRounder g_filter_rounder{CFeeRate{DEFAULT_MIN_RELAY_TX_FEE}};
+            if (m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
+                // Received tx-inv messages are discarded when the active
+                // chainstate is in IBD, so tell the peer to not send them.
+                currentFilter = MAX_MONEY;
+            } else {
+                static const CAmount MAX_FILTER{g_filter_rounder.round(MAX_MONEY)};
+                if (pto->m_tx_relay->lastSentFeeFilter == MAX_FILTER) {
+                    // Send the current filter if we sent MAX_FILTER previously
+                    // and made it out of IBD.
+                    pto->m_tx_relay->nextSendTimeFeeFilter = timeNow - 1;
+                }
+            }
             if (timeNow > pto->m_tx_relay->nextSendTimeFeeFilter) {
-                static CFeeRate default_feerate(DEFAULT_MIN_RELAY_TX_FEE);
-                static FeeFilterRounder filterRounder(default_feerate);
-                CAmount filterToSend = filterRounder.round(currentFilter);
+                CAmount filterToSend = g_filter_rounder.round(currentFilter);
                 // We always have a fee filter of at least minRelayTxFee
                 filterToSend = std::max(filterToSend, ::minRelayTxFee.GetFeePerK());
                 if (filterToSend != pto->m_tx_relay->lastSentFeeFilter) {
diff --git a/src/test/policy_fee_tests.cpp b/src/test/policy_fee_tests.cpp
new file mode 100644
index 0000000000..6d8872b11e
--- /dev/null
+++ b/src/test/policy_fee_tests.cpp
@@ -0,0 +1,34 @@
+// Copyright (c) 2020 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <amount.h>
+#include <policy/fees.h>
+
+#include <test/util/setup_common.h>
+
+#include <boost/test/unit_test.hpp>
+
+BOOST_FIXTURE_TEST_SUITE(policy_fee_tests, BasicTestingSetup)
+
+BOOST_AUTO_TEST_CASE(FeeRounder)
+{
+    FeeFilterRounder fee_rounder{CFeeRate{1000}};
+
+    // check that 1000 rounds to 974 or 1071
+    std::set<CAmount> results;
+    while (results.size() < 2) {
+        results.emplace(fee_rounder.round(1000));
+    }
+    BOOST_CHECK_EQUAL(*results.begin(), 974);
+    BOOST_CHECK_EQUAL(*++results.begin(), 1071);
+
+    // check that negative amounts rounds to 0
+    BOOST_CHECK_EQUAL(fee_rounder.round(-0), 0);
+    BOOST_CHECK_EQUAL(fee_rounder.round(-1), 0);
+
+    // check that MAX_MONEY rounds to 9170997
+    BOOST_CHECK_EQUAL(fee_rounder.round(MAX_MONEY), 9170997);
+}
+
+BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/validation.cpp b/src/validation.cpp
index 8bb03fdb97..fcaa62c9b2 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -5251,10 +5251,10 @@ CChainState& ChainstateManager::InitializeChainstate(const uint256& snapshot_blo
     return *to_modify;
 }
 
-CChain& ChainstateManager::ActiveChain() const
+CChainState& ChainstateManager::ActiveChainstate() const
 {
     assert(m_active_chainstate);
-    return m_active_chainstate->m_chain;
+    return *m_active_chainstate;
 }
 
 bool ChainstateManager::IsSnapshotActive() const
diff --git a/src/validation.h b/src/validation.h
index e403bcb51a..58383ad923 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -799,7 +799,8 @@ public:
     std::vector<CChainState*> GetAll();
 
     //! The most-work chain.
-    CChain& ActiveChain() const;
+    CChainState& ActiveChainstate() const;
+    CChain& ActiveChain() const { return ActiveChainstate().m_chain; }
     int ActiveHeight() const { return ActiveChain().Height(); }
     CBlockIndex* ActiveTip() const { return ActiveChain().Tip(); }
 
@@ -879,13 +880,13 @@ public:
 /** DEPRECATED! Please use node.chainman instead. May only be used in validation.cpp internally */
 extern ChainstateManager g_chainman GUARDED_BY(::cs_main);
 
-/** @returns the most-work valid chainstate. */
+/** Please prefer the identical ChainstateManager::ActiveChainstate */
 CChainState& ChainstateActive();
 
-/** @returns the most-work chain. */
+/** Please prefer the identical ChainstateManager::ActiveChain */
 CChain& ChainActive();
 
-/** @returns the global block index map. */
+/** Please prefer the identical ChainstateManager::BlockIndex */
 BlockMap& BlockIndex();
 
 /** Global variable that points to the active block tree (protected by cs_main) */
diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py
index 58d8c4abe1..ca26152e7e 100755
--- a/test/functional/rpc_net.py
+++ b/test/functional/rpc_net.py
@@ -46,10 +46,12 @@ class NetTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 2
-        self.extra_args = [["-minrelaytxfee=0.00001000"],["-minrelaytxfee=0.00000500"]]
+        self.extra_args = [["-minrelaytxfee=0.00001000"], ["-minrelaytxfee=0.00000500"]]
         self.supports_cli = False
 
     def run_test(self):
+        self.log.info('Get out of IBD for the minfeefilter test')
+        self.nodes[0].generate(1)
         self.log.info('Connect nodes both way')
         connect_nodes(self.nodes[0], 1)
         connect_nodes(self.nodes[1], 0)