[p2p] Introduce node rebroadcast module #21061

I'm importing this file so I can use the default key extractors in the indexed_rebroadcast_set multi_index: 1. https://github.com/bitcoin/bitcoin/pull/21061/files#diff-eccf6b88ae8b612dbdbdb92c110b66e4e7e3ffa665ccdc1acf08093aadc66b82R49 2. https://github.com/bitcoin/bitcoin/pull/21061/files#diff-eccf6b88ae8b612dbdbdb92c110b66e4e7e3ffa665ccdc1acf08093aadc66b82R55

The contents of the boost header file can be found here.

If we'd rather not import another boost header, I can avoid by writing custom key extractors.

I think it's fine to add a boost multi_index header dependency, it's the only submodule of boost that we really need and for which there is no replacement on the horizon. So just use what you need IMO, re-implementing anything from multi-index without a good plan would be a waste of time, I think.

Any reasons for 30 minutes? Less than 30 minutes might have issues. What if we use more than 30 minutes or user can define this in bitcoin.conf?

30 minutes was chosen as the minimum age to ensure that transactions would have ample time to propagate through mempools. I don't think it makes sense for this to be a configurable value, both because of what sipa highlighted here: #21061 (comment), as well as the fact that it would increase the complexity of reasoning about & maintenance burden. However, that said, I don't think of this value (30min) as set in stone- through the review process & running the patch, we should identify if this value is appropriate. Another thing to keep in mind is that any rebroadcast candidates will have to pass through filterInventoryKnown for an inv to be sent to the peer.

Prefer default initialization when a member will always be initialized to the same value.

    int m_count{1};

And remove m_count from the initializer list.

done

I think this is unused.

removed

It seems unnecessary to define a type here. GetRebroadcastTransactions() could just return a vector of std::pairs, and the one caller (UpdatedBlockTip()) can unpack that vector of pairs using :sparkle: s t r u c t u t e d :sparkle: b i n d i n g s :sparkle:. Here's what the caller would look like:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 7811e3ca40..e5a425b4c1 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1393,12 +1393,12 @@ void PeerManagerImpl::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlock
 
     // Rebroadcast selected mempool transactions
     if (gArgs.GetArg("-rebroadcast", DEFAULT_REBROADCAST_ENABLED)) {
-        std::vector<TxIds> rebroadcast_txs = m_txrebroadcast.GetRebroadcastTransactions();
+        auto rebroadcast_txs = m_txrebroadcast.GetRebroadcastTransactions();
 
         LOCK(cs_main);
 
-        for (auto ids : rebroadcast_txs) {
-            RelayTransaction(ids.m_txid, ids.m_wtxid, m_connman);
+        for (auto [txid, wtxid] : rebroadcast_txs) {
+            RelayTransaction(txid, wtxid, m_connman);
         }
     }

That client code works whether you return a vector of TxIds or a vector of std::pair<uint256, uint256>>s (which is as it should be :slightly_smiling_face: )

yeah, I considered this approach but I went for an explicitly defined structure because its easy to mix up txid & wtxid, and doing so would cause quiet failures. It feels harder for a call site to accidentally mixup with the named access.

That's fair. Marking this as resolved.

Perhaps make this a unique_ptr, and then pass in a boolean to the PeerManagerImpl ctor to indicate whether you want to enable tx rebroadcast and initialize the pointer.

Then, in UpdatedBlockTip:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 7811e3ca40..8bb5e8f87f 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1392,8 +1392,8 @@ void PeerManagerImpl::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlock
     }
 
     // Rebroadcast selected mempool transactions
-    if (gArgs.GetArg("-rebroadcast", DEFAULT_REBROADCAST_ENABLED)) {
-        std::vector<TxIds> rebroadcast_txs = m_txrebroadcast.GetRebroadcastTransactions();
+    if (m_txrebroadcast) {
+        std::vector<TxIds> rebroadcast_txs = m_txrebroadcast->GetRebroadcastTransactions();
 
         LOCK(cs_main);

parametrizing whether the TxRebroadcastHandler is initialized avoids having to read the global args every block, and should make it the code path more obviously isolated and testable.

great idea! done

Prefer default initialization in the declaration, rather than setting these in the ctor.

These aren't doing anything. They already get set in the Options constructor.

good point, removed

not needed!

removed

Separate library imports from local project imports:

#include <boost/test/unit_test.hpp>

#include <amount.h>

I separated them, but I have boost after internal. Is that ok? Btw, I don't see anything in the style guide about this, maybe we should add?

Yes, that's the right way to do it.

I don't think we have any guidance in the style guide, but general best practice seems to be to go from small to large: https://stackoverflow.com/a/2762596/933705.

It'd be really nice to avoid including these boost headers in a header that will transitively be included in a lot of translation units. Could you hide the indexed_rebroadcast_set inside the .cpp file to avoid this?

A quick and easy way to do this is to make indexed_rebroadcast_set a class that inherits from the boost::multi_index_container template class (defined in .cpp), forward declare it in .h and make m_attempt_tracker a unique pointer to indexed_rebroadcast_set. A few other things need to be shuffled around to make that work, but here's a rough implementation:

<details> <summary>Diff</summary>

diff --git a/src/test/txrebroadcast_tests.cpp b/src/test/txrebroadcast_tests.cpp
index 6fdd5deb8f..d10a962ab3 100644
--- a/src/test/txrebroadcast_tests.cpp
+++ b/src/test/txrebroadcast_tests.cpp
@@ -27,34 +27,13 @@ public:
 
     bool CheckRecordedAttempt(uint256 txhsh, int expected_count, std::chrono::microseconds expected_timestamp)
     {
-        const auto it = m_attempt_tracker.find(txhsh);
-        if (it == m_attempt_tracker.end()) return false;
-        if (it->m_count != expected_count) return false;
-
-        // Check the recorded timestamp is within 2 seconds of the param passed in
-        std::chrono::microseconds delta = expected_timestamp - it->m_last_attempt;
-        if (delta.count() > 2) return false;
-
-        return true;
-    };
+        return TxRebroadcastHandler::CheckRecordedAttempt(txhsh, expected_count, expected_timestamp);
+    }
 
     void UpdateAttempt(uint256 txhsh, int count)
     {
-        auto it = m_attempt_tracker.find(txhsh);
-        for (int i = 0; i < count; ++i) {
-            RecordAttempt(it);
-        }
-    };
-
-    void RecordAttempt(indexed_rebroadcast_set::index<index_by_wtxid>::type::iterator& entry_it)
-    {
-        auto UpdateRebroadcastEntry = [](RebroadcastEntry& rebroadcast_entry) {
-            rebroadcast_entry.m_last_attempt = GetTime<std::chrono::microseconds>() - 4h;
-            ++rebroadcast_entry.m_count;
-        };
-
-        m_attempt_tracker.modify(entry_it, UpdateRebroadcastEntry);
-    };
+        TxRebroadcastHandler::UpdateAttempt(txhsh, count);
+    }
 
     void UpdateCachedFeeRate(CFeeRate new_fee_rate)
     {
diff --git a/src/txrebroadcast.cpp b/src/txrebroadcast.cpp
index 3cb9abaf69..49050c0d39 100644
--- a/src/txrebroadcast.cpp
+++ b/src/txrebroadcast.cpp
@@ -10,6 +10,11 @@
 #include <txrebroadcast.h>
 #include <validation.h>
 
+#include <boost/multi_index/hashed_index.hpp>
+#include <boost/multi_index/member.hpp>
+#include <boost/multi_index/ordered_index.hpp>
+#include <boost/multi_index_container.hpp>
+
 /** We rebroadcast 3/4 of max block weight to reduce noise due to circumstances
  *  such as miners mining priority transactions. */
 static constexpr unsigned int MAX_REBROADCAST_WEIGHT = 3 * MAX_BLOCK_WEIGHT / 4;
@@ -30,6 +35,46 @@ static constexpr int MAX_ENTRIES = 500;
 /** The maximum age of an entry ~3 months */
 static constexpr std::chrono::hours MAX_ENTRY_AGE = std::chrono::hours(3 * 30 * 24);
 
+/** Used for multi_index tag  */
+struct index_by_last_attempt {};
+
+struct RebroadcastEntry {
+    RebroadcastEntry(std::chrono::microseconds now_time, uint256 wtxid)
+        : m_last_attempt(now_time),
+          m_wtxid(wtxid),
+          m_count(1) {}
+
+    std::chrono::microseconds m_last_attempt;
+    const uint256 m_wtxid;
+    int m_count;
+};
+
+class indexed_rebroadcast_set : public
+boost::multi_index_container<
+    RebroadcastEntry,
+    boost::multi_index::indexed_by<
+        // sorted by wtxid
+        boost::multi_index::hashed_unique<
+            boost::multi_index::tag<index_by_wtxid>,
+            boost::multi_index::member<RebroadcastEntry, const uint256, &RebroadcastEntry::m_wtxid>,
+            SaltedTxidHasher
+        >,
+        // sorted by last rebroadcast time
+        boost::multi_index::ordered_non_unique<
+            boost::multi_index::tag<index_by_last_attempt>,
+            boost::multi_index::member<RebroadcastEntry, std::chrono::microseconds, &RebroadcastEntry::m_last_attempt>
+        >
+    >
+> {};
+
+TxRebroadcastHandler::TxRebroadcastHandler(CTxMemPool& mempool, ChainstateManager& chainman)
+    : m_attempt_tracker{std::make_unique<indexed_rebroadcast_set>()}
+    , m_mempool{mempool}
+    , m_chainman{chainman}
+{}
+
+TxRebroadcastHandler::~TxRebroadcastHandler() = default;
+
 std::vector<TxIds> TxRebroadcastHandler::GetRebroadcastTransactions()
 {
     std::vector<TxIds> rebroadcast_txs;
@@ -58,12 +103,12 @@ std::vector<TxIds> TxRebroadcastHandler::GetRebroadcastTransactions()
 
         // Check if we have previously rebroadcasted, decide if we will this
         // round, and if so, record the attempt.
-        auto entry_it = m_attempt_tracker.find(wtxid);
+        auto entry_it = m_attempt_tracker->find(wtxid);
 
-        if (entry_it == m_attempt_tracker.end()) {
+        if (entry_it == m_attempt_tracker->end()) {
             // No existing entry, we will rebroadcast, so create a new one
             RebroadcastEntry entry(start_time, wtxid);
-            m_attempt_tracker.insert(entry);
+            m_attempt_tracker->insert(entry);
         } else if (entry_it->m_count >= MAX_REBROADCAST_COUNT) {
             // We have already rebroadcast this transaction the maximum number
             // of times permitted, so skip rebroadcasting.
@@ -76,7 +121,12 @@ std::vector<TxIds> TxRebroadcastHandler::GetRebroadcastTransactions()
         } else {
             // We have rebroadcasted this transaction before, but will try
             // again now.
-            RecordAttempt(entry_it);
+            auto UpdateRebroadcastEntry = [](RebroadcastEntry& rebroadcast_entry) {
+                rebroadcast_entry.m_last_attempt = GetTime<std::chrono::microseconds>();
+                ++rebroadcast_entry.m_count;
+            };
+
+            m_attempt_tracker->modify(entry_it, UpdateRebroadcastEntry);
         }
 
         // Add to set of rebroadcast candidates
@@ -97,6 +147,32 @@ std::vector<TxIds> TxRebroadcastHandler::GetRebroadcastTransactions()
     return rebroadcast_txs;
 };
 
+void TxRebroadcastHandler::UpdateAttempt(uint256 txhsh, int count)
+{
+    auto it = m_attempt_tracker->find(txhsh);
+    for (int i = 0; i < count; ++i) {
+        auto UpdateRebroadcastEntry = [](RebroadcastEntry& rebroadcast_entry) {
+            rebroadcast_entry.m_last_attempt = GetTime<std::chrono::microseconds>() - 4h;
+            ++rebroadcast_entry.m_count;
+        };
+
+        m_attempt_tracker->modify(it, UpdateRebroadcastEntry);
+    }
+};
+
+bool TxRebroadcastHandler::CheckRecordedAttempt(uint256 txhsh, int expected_count, std::chrono::microseconds expected_timestamp)
+{
+    const auto it = m_attempt_tracker->find(txhsh);
+    if (it == m_attempt_tracker->end()) return false;
+    if (it->m_count != expected_count) return false;
+
+    // Check the recorded timestamp is within 2 seconds of the param passed in
+    std::chrono::microseconds delta = expected_timestamp - it->m_last_attempt;
+    if (delta.count() > 2) return false;
+
+    return true;
+};
+
 void TxRebroadcastHandler::CacheMinRebroadcastFee()
 {
     // Update stamp of chain tip on cache run
@@ -109,33 +185,23 @@ void TxRebroadcastHandler::CacheMinRebroadcastFee()
     LogPrint(BCLog::BENCH, "Caching minimum fee for rebroadcast to %s, took %d µs to calculate.\n", m_cached_fee_rate.ToString(FeeEstimateMode::SAT_VB), delta_time.count());
 };
 
-void TxRebroadcastHandler::RecordAttempt(indexed_rebroadcast_set::index<index_by_wtxid>::type::iterator& entry_it)
-{
-    auto UpdateRebroadcastEntry = [](RebroadcastEntry& rebroadcast_entry) {
-        rebroadcast_entry.m_last_attempt = GetTime<std::chrono::microseconds>();
-        ++rebroadcast_entry.m_count;
-    };
-
-    m_attempt_tracker.modify(entry_it, UpdateRebroadcastEntry);
-};
-
 void TxRebroadcastHandler::TrimMaxRebroadcast()
 {
     // Delete any entries that are older than MAX_ENTRY_AGE
     std::chrono::microseconds min_age = GetTime<std::chrono::microseconds>() - MAX_ENTRY_AGE;
 
-    while (!m_attempt_tracker.empty()) {
-        auto it = m_attempt_tracker.get<index_by_last_attempt>().begin();
+    while (!m_attempt_tracker->empty()) {
+        auto it = m_attempt_tracker->get<index_by_last_attempt>().begin();
         if (it->m_last_attempt < min_age) {
-            m_attempt_tracker.get<index_by_last_attempt>().erase(it);
+            m_attempt_tracker->get<index_by_last_attempt>().erase(it);
         } else {
             break;
         }
     }
 
     // If there are still too many entries, delete the oldest ones
-    while (m_attempt_tracker.size() > MAX_ENTRIES) {
-        auto it = m_attempt_tracker.get<index_by_last_attempt>().begin();
-        m_attempt_tracker.get<index_by_last_attempt>().erase(it);
+    while (m_attempt_tracker->size() > MAX_ENTRIES) {
+        auto it = m_attempt_tracker->get<index_by_last_attempt>().begin();
+        m_attempt_tracker->get<index_by_last_attempt>().erase(it);
     }
 };
diff --git a/src/txrebroadcast.h b/src/txrebroadcast.h
index 8a176a08bc..ec3ea34ed7 100644
--- a/src/txrebroadcast.h
+++ b/src/txrebroadcast.h
@@ -12,11 +12,6 @@
 #include <util/time.h>
 #include <validation.h>
 
-#include <boost/multi_index/hashed_index.hpp>
-#include <boost/multi_index/member.hpp>
-#include <boost/multi_index/ordered_index.hpp>
-#include <boost/multi_index_container.hpp>
-
 struct TxIds {
     TxIds(uint256 txid, uint256 wtxid) : m_txid(txid), m_wtxid(wtxid) {}
 
@@ -24,43 +19,15 @@ struct TxIds {
     const uint256 m_wtxid;
 };
 
-struct RebroadcastEntry {
-    RebroadcastEntry(std::chrono::microseconds now_time, uint256 wtxid)
-        : m_last_attempt(now_time),
-          m_wtxid(wtxid),
-          m_count(1) {}
-
-    std::chrono::microseconds m_last_attempt;
-    const uint256 m_wtxid;
-    int m_count;
-};
-
-/** Used for multi_index tag  */
-struct index_by_last_attempt {};
-
-using indexed_rebroadcast_set = boost::multi_index_container<
-    RebroadcastEntry,
-    boost::multi_index::indexed_by<
-        // sorted by wtxid
-        boost::multi_index::hashed_unique<
-            boost::multi_index::tag<index_by_wtxid>,
-            boost::multi_index::member<RebroadcastEntry, const uint256, &RebroadcastEntry::m_wtxid>,
-            SaltedTxidHasher
-        >,
-        // sorted by last rebroadcast time
-        boost::multi_index::ordered_non_unique<
-            boost::multi_index::tag<index_by_last_attempt>,
-            boost::multi_index::member<RebroadcastEntry, std::chrono::microseconds, &RebroadcastEntry::m_last_attempt>
-        >
-    >
->;
+class indexed_rebroadcast_set;
 
 class TxRebroadcastHandler
 {
 public:
-    TxRebroadcastHandler(CTxMemPool& mempool, ChainstateManager& chainman)
-        : m_mempool(mempool),
-          m_chainman(chainman){};
+    TxRebroadcastHandler(CTxMemPool& mempool, ChainstateManager& chainman);
+    ~TxRebroadcastHandler();
+
+    TxRebroadcastHandler(const TxRebroadcastHandler& other) = delete;
 
     std::vector<TxIds> GetRebroadcastTransactions();
 
@@ -75,10 +42,13 @@ protected:
     CFeeRate m_cached_fee_rate;
 
     /** Keep track of previous rebroadcast attempts */
-    indexed_rebroadcast_set m_attempt_tracker;
+    std::unique_ptr<indexed_rebroadcast_set> m_attempt_tracker;
+
+    /** Test only */
+    void UpdateAttempt(uint256 txhsh, int count);
 
-    /** Update an existing RebroadcastEntry - increment count and update timestamp */
-    void RecordAttempt(indexed_rebroadcast_set::index<index_by_wtxid>::type::iterator& entry_it);
+    /** Test only */
+    bool CheckRecordedAttempt(uint256 txhsh, int expected_count, std::chrono::microseconds expected_timestamp);
 
 private:
     const CTxMemPool& m_mempool;

</details>

txrebroadcast.h is only included from net_processing and txrebroadcast.cpp itself, and net_processing already includes txmempool.h so there's not much saving to be had here I think

Not currently, but I think a long-term it'd be good to remove the boost header dependencies from modules that aren't using boost multi_index. That'd involve refactoring the txmempool interface so it's not exposing multi_index objects.

To be honest, I'm not entirely sure how much actual benefit this would have on build times and binary size. I think that boost multi_index is almost entirely templates, so if they're not being instantiated in a translation unit, then it's probably minimal. Still, I think it's a good goal in itself to minimize where third party libraries are exposed within our code structure.

I don't think that having the boost headers has much tangible impact, but I decided to implement mostly for code legibility. The boost logic was ~1/2 of txrebroadcast.h, obscuring the way the module is supposed to be interacted with. I think it'd be fine for now, but more annoying if the rebroadcast logic were to get any more complex, which I think is a possibility.

Could keep the old cached fee rate here, so that if you try end up doing UpdateTip / CacheMinRebroadcastFee / RebroadcatTxs you can do something sensible?

   new_tip = ::ChainActive().Tip();
   if (new_tip != m_tip_at_cache_time) {
       m_last_cached_fee_rate = m_cached_fee_rate;
   } else {
       m_last_cached_fee_rate = max; // ?
   }
   m_tip_at_cache_time = new_tip;

I added some logging to see how often this happens, and indeed it looks like it skips 1-3 times on most days, sometimes up to ~10 times. So, this mechanism seems good, I'll add it.

done

Why is this lock added in the first commit and then removed?

oops, removed

Entries from m_attempt_tracker seem to be only cleared after 3 months or when it's full. Shouldn't we also remove them if the tx is removed from the mempool because it has made it into a block (which would seem like the most common cause for removing)?

Also make sure that you remove transactions that are conflicted out by the block (ie a conflicting transaction is included in the block).

Other cases:

• tx was RBFed out of mempool
• parents were invalidated by conflicting tx in mempool and/or block

~I think it boils down to = if we aren't keeping the tx in mempool, don't keep it in the rebroadcast tracker.~ I totally misunderstood, I thought you wouldn't want a tx in rebroadcast tracker if you didn't have it in mempool, but actually you'd want to keep it for a bit.

Removing entries from m_attempt_tracker when tx is removed from mempool doesn't seem necessary given m_attempt_tracker isn't used as a source for txs that need to be rebroadcasted.

I also don't see an issue with keeping txs that have already been included in a block or RBFed out of mempool in the m_attempt_tracker for up to 90 days (as per current setting) or till it gets removed after 500 limit is reached. Removing these entries from m_attempt_tracker would add unnecessary complexity for a small benefit of a little saved memory as far as I understand. On that note, would perhaps removing the 90 days limit and just keeping the 500 limit still be fine?

Removing entries from m_attempt_tracker when tx is removed from mempool doesn't seem necessary given m_attempt_tracker isn't used as a source for txs that need to be rebroadcasted.

I agree that rebroadcast would still work - the problem is more that m_attempt_tracker might not be able to fulfill its intended purpose with a 500 limit if it was keeping track of too many unnecessary transactions (and at the same time expel those that it was meant to keep track off after capacity is reached).

I added commit 2ac22164f82a51d4eaece9acc42f346e8db679a5 to address this! I agree that when transactions are removed from the mempool for certain reasons, we are highly unlikely to see it again, so it makes sense for it to be removed from the rebroadcast attempt tracker. It's ok if it doesn't work perfectly for edge cases, but this behavior supports helps it work as intended. Thanks!

The mempool is persisted to disk, would it make sense to do the same with the rebroadcast tracker? With the index designed to keep a memory of months, I'd guess that several nodes would undergo several restarts during such a long period and, currently, lose all memory about past rebroadcasts.

seems reasonable, but this PR is getting pretty huge so I'd rather keep this for a follow-up. I've noted it down here #21061 (comment) so I can keep track of it, and am going to resolve this conversation.

We're trying to get rid of global calls like Params(). Consider adding a:

    const CChainParams& m_chainparams;

member to TxRebroadcastHandler and setting it in the ctor (like PeerManagerImpl does)

done

    bool m_check_block_validity;

done

The type is obvious from the template parameter, so may be a good place to use auto:

    auto start_time = GetTime<std::chrono::microseconds>();

(and in the other calls to GetTime)

ok ok, I think I auto-ed the GetTime call sites.

AUTO

Maybe better to use "us" instead of "µs" to keep logs in ascii.

aw man, I thought I was so cool

use std::make_unique() to make unique pointers.

done

This seems impossible to hit. You've just constructed a new std::unique_ptr<CBlockTemplate>.

yeah, looks like I copied this from CreateNewBlock and thought it was a "safety check" but I agree, it seems pointless. Removed.

I don't think this is needed. Witness is definitely enabled at the tip!

so you'd prefer if I hard coded this to fIncludeWitness = true? 🤔 IsWitnessEnabled doesn't seem like an expensive function.

Avoid calls to the global ::ChainActive().

done by making chainman a member

    auto start_time = GetTime<std::chrono::microseconds>();

Functions should be capitalized:

    CFeeRate MinTxFeeRate();

fixed

again, maybe avoid greek letters:

    LogPrint(BCLog::BENCH, "Caching minimum fee for rebroadcast to %s, took %d us to calculate.\n", m_cached_fee_rate.ToString(FeeEstimateMode::SAT_VB), delta_time.count());

Are these definitely needed? You're just going to throw the block template away at the end.

I think you're right, so I've removed these three lines. nice catch!

Avoid the global ::ChainActive()

fixed

I don't think this is needed. You're throwing away the block afterwards.

agreed, gone

join these lines?

Is cs_main definitely required here? addPackageTxs() isn't annotated to say that it's required.

I tried to audit addPackageTxs and I think you're right that it's not necessary, but it's hard to be certain because there is a lot going on in the function & its only ever called under the cs_main lock. Do you have any recommendations for how I could build confidence other than checking the thread of every call site of every invoked function / variable? Maybe I could just remove and run the node and give it a 👍 if it doesn't crash for a week? 😅

I removed the cs_main call from here, but think I actually have to lock it from the caller, see: #21061 (review)

min_package_fee_rate seems very cheap to calculate. Perhaps just make it a reference so it's always returned.

(even better would be:

std::tuple<int, int, CFeeRate> BlockAssembler::addPackageTxs()

but that's maybe beyond the scope of this PR)

agree that updating min_package_fee_rate is pretty cheap, there's no additional calculation, its just updating the stored value, but might do so up to ~once per mempool transaction.

but can you help me understand the context of why you're suggesting a reference? even if it's not a lot of calculation, seems unnecessary?

(agree that returning a tuple instead of in/out params would be nice. but leaving other params as is within this PR.)

avoid global ::ChainActive

fixed

I think we prefer to include all headers that are used in the translation unit, even if they're also included in the corresponding header file. You don't need to remove the "#include <util/time.h> in the [rebroadcast] Track rebroadcast attempts commit.

Perhaps add some of the description from the [rebroadcast] Track rebroadcast attempts commit log here. It's not at all obvious why we'd stop rebroadcasting after some time, and that commit log has a good explanation.

done, how's it look? 3f4b664 (#21061)

Maybe pass in the current time to avoid calling GetTime() for every transaction?

no longer using the RecordAttempt function here, but did apply the concept so we don't call GetTime on a loop, https://github.com/bitcoin/bitcoin/pull/21061/files?file-filters%5B%5D=.cpp&file-filters%5B%5D=.h&file-filters%5B%5D=.py&file-filters%5B%5D=.sh#diff-7dff50848db96bdb8edffc4d21daeca6d9050ec0e67d96072780ea5751e7df06R124

y so verbose?

static constexpr std::chrono::hours MAX_ENTRY_AGE = 24h * 30 * 3;

Import standard libraries first, then local modules (this is the opposite order from c++)

(this is recommended in PEP8: https://www.python.org/dev/peps/pep-0008/#imports)

braced initialization? 😛

ok, done for all the constants in this file

Why oldest and not random eviction, for example? Have you considered having a configurable value (similar to -maxorphantx)?

RE oldest vs random: since the attempt tracker updates the timestamp every time a transaction is selected as a candidate for rebroadcast, it makes sense to me that the least relevant would be ones that haven't been selected for the longest time. usually we use randomness in p2p data structures to ensure that a malicious peer cannot have disproportionate influence, which I don't think quite applies here. its possible that a peer could roll the attempt tracker, but they would also have to roll filterInventoryKnown in order to spend actual network bandwidth. I haven't thought this through in great depth, so let me know if I'm missing some reasons why we should consider randomizing.

RE configurable: as sipa mentioned in #21061 (comment), "generally the criterion for deciding whether a parameter should be configurable is whether you can also give advice under which cases users should be changing it, and how." I am also sensitive to introducing configurable params because it increases the maintenance burden of the project essentially forever. Do you see use cases of how you would recommend users to adjust the attempt tracker? The main reason I'd imagine for wanting a larger tracker is to reduce bandwidth, but in that case I think a "turn rebroadcast off" toggle would make more sense (and of this PR its off by default)

~I don't think we should rebroadcast after every block if we're in IBD?~ Nevermind there's already an IBD gate above

Same, maybe not worth doing until after IBD?

good point! I kick off the scheduler here, but an early exit in the function if IBD: https://github.com/bitcoin/bitcoin/pull/21061/files?file-filters%5B%5D=.cpp&file-filters%5B%5D=.py&file-filters%5B%5D=.sh#diff-7dff50848db96bdb8edffc4d21daeca6d9050ec0e67d96072780ea5751e7df06R152

Super small nit: Since this is new code - consider renaming as after_cnb_time to conform to the symbol naming conventions as described in the developer notes? :)

In 06890651027cc50bdf3897283386a973bbf55725 "[mining] Add recency condition on block creation to get rebroadcast set"

This comment could be clearer as m_skip_inclusion_until isn't really an age. Rather we exclude any transactions that entered the mempool after the time specified by m_skip_inclusion_until.

Used this suggested wording & updated to:

      * If m_skip_inclusion_until is set in the options, we will exclude any
      * transactions that entered the mempool after the time specified. This is
      * currently used for rebroadcast logic. */

In 8b4db051bac56cefa8c85fe20e5548ddfab0f070 "[rebroadcast] Apply a fee rate filter"

I'm having a hard time understanding how this comment explains the condition here. Could you explain in more detail?

you're right, this comment is really confusing. updated to:

    // If the cache has run since we received the last block, the fee rate
    // condition will not filter out any transactions, so skip this run.

And here's some more explanation:

Let's refer to the fee rate for a txn to be in the top MAX_REBROADCAST_WEIGHT as top-of-mempool fee rate. The only event that has the ability to decrease the top-of-mempool fee rate is a block being mined. Otherwise, as transactions enter the mempool, the top-of-mempool fee rate either stays the same or increases.

If the top-of-mempool fee rate is only increasing over time, the fee rate cache would not help filter any transactions out.

Our desired order is: cache | block | rebroadcast, with the delta between cache & block to be as small as possible.

The order we want to skip is: block | cache | rebroadcast, because then the fee rate filter will be meaningless, and would simply select the next highest transactions to rebroadcast (which we logically wouldn't expect to be mined yet).

Does this make sense? Does the update to the comment help clarify? I'm not sure the best way to capture this in the code, so open to suggestions.

In d579110263ad07386f1fbf796a3397a12340b6d5 "[test] Add unit test for rebroadcast attempt logic"

It seems like this for loop is not necessary as it is essentially doing m_count += count. Could instead be:

    auto UpdateRebroadcastEntry = [last_attempt_time, count](RebroadcastEntry& rebroadcast_entry) {
        rebroadcast_entry.m_last_attempt = last_attempt_time;
        rebroadcast_entry.m_count += count;
    };

    m_attempt_tracker->modify(it, UpdateRebroadcastEntry);

good point, updated!

In d579110263ad07386f1fbf796a3397a12340b6d5 "[test] Add unit test for rebroadcast attempt logic"

It seems a bit odd to me that this function is supposed to be a wrapper but it also implements a little bit of test logic in that it changes the recorded last attempt time. I think it would make more sense to have the main test logic calculate the timestamp change and make this function a pure wrapper (or perhaps remove the entire class).

ok! removed the entire class and just made the test functions public with a comment indicating they are test only

In 0f13d876977d6a83549fa40f68a73308ba10d95d "[test] Add unit test for the fee rate cache"

Seems like this change should be part of the previous commit.

fixed

In 0f13d876977d6a83549fa40f68a73308ba10d95d "[test] Add unit test for the fee rate cache"

Seems like this change should be part of the previous commit.

In 7005767691005f66e7e08ec2e575d436dae3be47 "[test] Functional tests for rebroadcast logic."

# Copyright (c) 2021 The Bitcoin Core developers

In 7005767691005f66e7e08ec2e575d436dae3be47 "[test] Functional tests for rebroadcast logic."

getrawtransaction can decode txs too.

        raw_tx = node.getrawtransaction(input_tx_hsh, True)

In 7005767691005f66e7e08ec2e575d436dae3be47 "[test] Functional tests for rebroadcast logic."

It would be nice to not require the wallet for a p2p test.

agreed, I've noted it down here: #21061 (comment) so I can keep track of it, and will address either in this PR or in a follow up. I'm going to resolve this conversation and will track it with that comment.

IIUC the purpose of this variable, we assume miners's mempools to be composed in average of 1/4 of irreplaceable transactions. I.e transactions even if we know better feerate candidates, we won't be able to replace them as their scores are unduly increased by miner's mempool policy.

Intuitively, this sounds a lot, do you have any real-world data backing up this assumption ? One could look on how many mined block transactions are under the local node MinTxFeeRate you've introduced to determine the size of the surprise set.

What's the trade-off of picking a value deviating from real-world data ? Well I think you may prevent or delay rebroadcast of transactions in the bottom quarter of block feerate. Due to this delay, broadcasting clients of those transactions may reach a fee-bumping timer and they trigger a RBF/CPFP in consequence. As such bleeding feerate beyond what would have been effective to get into block templates.

If the risk I'm describing is founded, do you think we should preserve transactions broadcaster's feerate instead of network nodes' bandwidth ? I'm leaning toward a yes as rebroadcast logic is at least opt-in, but broadcaster won't be aware of their peers's policies barring propagation of their good-enough feerate transaction.

Let's say an attacker partitions a victim's mempool from the rest of network mempools thanks to maliciously conflicting transactions. E.g send non-rbf signaling, differing transactions to victim and all its tx-relay connected peers.

Those malicious transactions are attached a feerate high-enough to completely occupy the m_attempt_tracker. Due to MIN_REATTEMPT_INTERVAL=4h and assuming 6 blocks per hour, it will require 6 * 4 * 500 = 12000 transactions to jam victim's rebroadcast attempt. Of course, MAX_REBROADCAST_COUNT should upper bound such behavior, but an attacker can easily renew m_attempt_tracker jamming by RBFing the pinning transactions.

Let's assume malicious transactions size is MIN_STANDARD_TX_NONWITNESS_SIZE, so 82 bytes. Let's assume top block feerate is 170 sat/vbyte. Shrugging at RBF replacement fee penalties, 12_000 * 82 * 170 = 167_280_000 sats. So a permanent m_attempt_tracker jamming would require 1,67 BTC available to the attacker, though not burnt if the mempool isolation is operated properly.

Do the attack model seems sound to you ? If so I think we should carefully document risks of m_attempt_tracker pinning and likely scale up parameters value.

what is the fundamental resource that is being spent in this attack? are you evaluating the possibility of asymmetric bandwidth usage? if that's the case then you also need to factor in rolling over filterInventoryKnown.

also I don't understand how the attacker avoids spending any funds- say A and A' are two versions of the same txn via RBF. A is sent to the victim & A' is sent to the rest of the network. according to the current patch, if A' gets mined into a block, A is removed from the attempt tracker.

update: wait, you refer to the bandwidth interactions with filterInventoryKnown in your comment #21061 (review), so then I must be missing what you are trying to describe in this attack.

second update: is an assumption behind this hypothetical attack that jamming m_attempt_tracker would prevent a node from rebroadcasting? just want to clarify that rebroadcasts would work without the tracker, its mainly a technique to mitigate bandwidth usage.

Thanks for the second clarifying update, effectively this is documented L124 in src/txrebroadcast.cpp, "No existing entry, we will rebroadcast, so create new one".

So IIUC, even if m_attempt_tracker is fulfilled with pinning transactions, it won't block node rebroadcast capability. I think pinning costs of m_attempt_tracker above roughly above but it can't be abused by an attacker to jam rebroadcasting, so we're good.

I would recommend to document this "pinning-doesn't-block-rebroadcast-so-not-a-new-tx-relay jamming-vector" around m_attempt_tracker declaration in src/txreconciliation.h, it's quite an important design aspect imo.

W.r.t to asymmetric bandwidth usage and risks of bleeding, I don't think m_attempt_tracker pinning is making it better or worst, I share the opinion we're ultimately protected by filterInventoryKnown.

I don't understand how the attacker avoids spending any funds- say A and A' are two versions of the same txn via RBF. A is sent to the victim & A' is sent to the rest of the network. according to the current patch, if A' gets mined into a block, A is removed from the attempt tracker.

I should have precise this point. Transaction A feerate to successfully pin m_attempt_tracker must be at 170 sat/vbyte. However transaction A' feerate just need to be above miners'mempools min feerates to block propagation of transaction A. If transaction A' feerate is 2 sat/vbyte, with same equation laid out above, effective pinning cost for an attacker is 12_000 * 82 * 2 = 1_968_000 sats and not 167_280_000 sats. Miners observe and eventually mine transactions with a far lower-feerate than the ones maliciously sent to non-miner nodes. This is what I meant by "not burnt if the mempool isolation is operated properly".

So IIUC, even if m_attempt_tracker is fulfilled with pinning transactions, it won't block node rebroadcast capability.

yeah, exactly.

I would recommend to document this "pinning-doesn't-block-rebroadcast-so-not-a-new-tx-relay jamming-vector" around m_attempt_tracker declaration in src/txreconciliation.h, it's quite an important design aspect imo.

I don't think it makes sense to document what data structures don't do.

I should have precise this point...

Ah I understand now, thanks for clarifying.

Assuming an entry's m_count reaches MAX_REBROADCAST_COUNT, it should never be again selected by GetRebroadcastTransactions. At that point, should we return an event to the wallet logic inviting to RBF the transaction ?

Otherwise, the entry should stale until deletion by TrimMaxRebroadcast. At that point should we return an event inviting to re-submit the same transaction ? Network mempools might have cleared up in between.

I don't know if this new rebroadcasting mechanism should be transparent or not to Bitcoin applications vetted with their own rebroadcasting logics.

At that point, should we return an event to the wallet logic inviting to RBF the transaction ?

that's a cool idea. I don't think it is applicable yet, but I've noted it down for future work where it would be more relevant.

why I don't think it is applicable yet: as of this patch, ResendWalletTransactions still calls SubmitMemoryPoolAndRelay with the relay bool set to true (aka patch doesn't change it). This gets used in BroadcastTransaction to force relaying the transaction to peers at the time of submission. in the future, the goal is to disable this (see "Merge Plan & Next Steps" in #21061 (comment)), at which time I believe this sort of mechanism would make sense.

What's the rational to pick up this value ?

I don't think Bitcoin applications care about confirmation of transactions broadcast 3 months ago. At the contrary, an attacker could use this really long expiration delay as a fingerprint vector.

Let's say in optimistic scenarios, a rebroadcast entry has a compelling feerate enough to be selected by GetRebroadcastTransactions after each MIN_REATTEMPT_INTERVAL, after 6 * 4, it's matured enough to be legitimately expired ? Of course, we can assume some margin of error and double to 48h. Or even select MEMPOOL_EXPIRY as a maximum age. But I don't understand the month-length delay...

I don't understand this comment, let me explain some of the mechanisms incase that helps. otherwise, please help me understand what you are trying to communicate.

I don't think Bitcoin applications care about confirmation of transactions broadcast 3 months ago.

1. the attempt tracker records the timestamp of when the mempool transaction was last selected for rebroadcast, not when it was initially broadcast / ATMPed (see https://github.com/bitcoin/bitcoin/pull/21061/files?file-filters%5B%5D=.cpp&file-filters%5B%5D=.h&file-filters%5B%5D=.py&file-filters%5B%5D=.sh#diff-7dff50848db96bdb8edffc4d21daeca6d9050ec0e67d96072780ea5751e7df06R110 and https://github.com/bitcoin/bitcoin/pull/21061/files?file-filters%5B%5D=.cpp&file-filters%5B%5D=.h&file-filters%5B%5D=.py&file-filters%5B%5D=.sh#diff-7dff50848db96bdb8edffc4d21daeca6d9050ec0e67d96072780ea5751e7df06R125).
2. MAX_ENTRY_AGE is only called from TrimMaxRebroadcast. It is essentially used to say "if a transaction hasn't been selected for rebroadcast in the past 3 months, remove it from the attempt tracker"

Let's say in optimistic scenarios, a rebroadcast entry has a compelling feerate enough to be selected by GetRebroadcastTransactions after each MIN_REATTEMPT_INTERVAL, after 6 * 4, it's matured enough to be legitimately expired ?

can you clarify what you mean by "legitimately expired"? if you mean that we should disable automatic rebroadcast so the txn can (probably) expire from the (majority of) network mempools, then yes I agree. which is why we put it on the tracker. but then, what is the question?

select MEMPOOL_EXPIRY as a maximum age.

if we stored transactions on the attempt tracker for the same duration as MEMPOOL_EXPIRY time, that would leave room for the issue that the attempt tracker is meant to address. for more explanation on that, please see https://github.com/amitiuttarwar/bitcoin-notes/blob/main/rebroadcast-history.md#faq section "What happens if a transaction can / will never be mined?". expiry would be one of the main ways to get the ping-ponging behavior for never-going-to-be-mined transactions (eg. due to a policy upgrade on the network).

IIRC, by "legitimately expired" I meant a transaction of which m_count has reached MAX_REBROADCAST_COUNT. As we already rebroadcasted this transaction the maximum number of times permitted, we skip its rebroadcasting forever, but without necessarily removing it from m_attempt_tracker ? I think such transaction has exhausted its rebroadcast "credit" and it's mature enough to be "legitimately" expired or pruned. Though this deletion criteria isn't currently considered in TrimMaxRebroadcast() ?

I think this deletion criteria would be more efficient to expire transactions from the majority of network mempools rather than MAX_ENTRY_AGE, especially with a 3 months delay.

if we stored transactions on the attempt tracker for the same duration as MEMPOOL_EXPIRY time, that would leave room for the issue that the attempt tracker is meant to address. for more explanation on that, please see https://github.com/amitiuttarwar/bitcoin-notes/blob/main/rebroadcast-history.md#faq section "What happens if a transaction can / will never be mined?". expiry would be one of the main ways to get the ping-ponging behavior for never-going-to-be-mined transactions (eg. due to a policy upgrade on the network)

IIUC, the problem we're trying to solve is "How to avoid never-going-to-be-mined transactions to forever ping-pong across the network ?".

To achieve this aim, I believe we should expire a rebroadcast entry as soon as it has exhausted its rebroadcast count, a hint that either feerate is definitively aloof or severer circumstances as described in documentation. But selecting a MAX_ENTRY_AGE superior to MEMPOOL_EXPIRY moves in the opposite direction.

If rebroadcast entry average rebroadcast interval is superior to 56 hours, 56 * MAX_REBROADCAST_COUNT== MEMPOOL_EXPIRY and node rebroadcast guarantees attempts beyond the mempool expiration limit ?

Or is the behavior I'm describing not possible ? I'm happy to illustrate with a test to assert our intuitions :)

update: Or is the purpose of keeping rebroadcast entry with m_count == MAX_REBROADCAST_COUNT and only expire them when last_attempt is older than MAX_ENTRY_AGE prevent new registration in m_attempt_tracker cache provoked by rebroadcast from other network mempools ? Thus you expect a transaction to expire from mempools after MEMPOOL_EXPIRY, then to avoid network laggards with differing policy to trigger new rebroadcasting wawes, you have MAX_ENTRY_AGE as an upper bound of MEMPOOL_EXPIRY ?

Okay I can see how it solves mempool ping-pong though reasoning could be better documented and still we should envisage to reduce MAX_ENTRY_AGE to minimize node fingerprinting.

it seems like the majority of this comment is based on a misunderstanding of how m_attempt_tracker works.

the update seems like its getting on the right track- the main purpose of tracking rebroadcast entries is to allow enforcing a maximum number of rebroadcast attempts.

IIUC, the problem we're trying to solve is "How to avoid never-going-to-be-mined transactions to forever ping-pong across the network ?".

yes, exactly.

but it still seems like there is remaining confusion. MAX_ENTRY_AGE indicates the amount of time has passed since tx A has been rebroadcast to peers. if its been 3 months since we last tried to rebroadcast tx A, we remove the attempts from the tracker. if there is no entry for tx A on the tracker, we don't have record of the fact that we tried to rebroadcast it, so if tx A is selected by the block assembler, we would rebroadcast it again.

I don't see how MAX_ENTRY_AGE would viably be used for node fingerprinting when you combine with the other factors such as filterInventoryKnown.

  txdb.h \
  txmempool.h \
  txorphanage.h \
  txrebroadcast.h \
  txrequest.h \
  undo.h \

is there a suggestion here?

Have you tried to run this branch on mainet with a wide-sized mempool ? I think there is a logger in CacheMinRebroadcastFee, do you have stats to share on how much time it take by attempts ?

Not sure if the block min feerate accuracy is worth the CPU time...

yup, here are some values are for running the whole function. Note that it does not distinguish CPU time vs time spent waiting to acquire the lock, as it currently acquires cs_main and mempool.cs, but the first might be unnecessary (see #21061 (review)):

the median value is 9090 μs, with 75% of the runs being under 11300 μs, and 99% of the run being under 11826 μs.

Not sure if the block min feerate accuracy is worth the CPU time...

its a pretty critical component to this approach, without it the node would most likely be attempting to rebroadcast MAX_REBROADCAST_WEIGHT of transactions on every block.

the median value is 9090 μs, with 75% of the runs being under 11300 μs, and 99% of the run being under 11826 μs.

Okay and with a fulfilled mempool of the default size 300 MiB ? If yes I agree that's not that much and I think we're relieved on this concern :) If it does causes issues on low-grade nodes in the future, due to non-linear scaling of the CPU time in function of mempool size, I guess we'll be able to make this value configurable, offering a trade-off between CPU and bandwidth to node operators.

its a pretty critical component to this approach, without it the node would most likely be attempting to rebroadcast MAX_REBROADCAST_WEIGHT of transactions on every block.

Do you mean without it or with a lower frequency rather than 1 min ? IIUC, the node would attempt to rebroadcast MAX_REBROADCAST_WEIGHT because you don't account for the fresh arrivals of new transactions. The feerate of those newcomers making most of your rebroadcast candidates irrelevant to block confirmation ? If refreshing the cache was costly, I guess we would run it once between expected block intervals when the odds of having discovered enough new transactions to have a consistent view of next block are high. But cost is pretty low, so I don't think we care further...

nit: maybe replace 1 by a named constant like CACHE_REBROADCAST_FEERATE_FREQUENCY ?

Okay and with a fulfilled mempool of the default size 300 MiB ? If yes I agree that's not that much and I think we're relieved on this concern :)

my mempool is bumped up to 1000MiB, so these are values for a significantly-larger-than-normal mempool :)

I guess we'll be able to make this value configurable

yup, rebroadcast currently defaults off, but the plan is to keep this toggle in the long run to address any bandwidth concerns.

maybe replace 1 by a named constant like CACHE_REBROADCAST_FEERATE_FREQUENCY

done

I think a deletion criteria which might applied before age is delaying if m_count == MAX_REBROADCAST_COUNT. A transaction might be the oldest one but still feerate compelling and was unlucky on its previous rebroadcast attempts ?

hm, then what do you propose as the method to ensure old attempts expire from the tracker?

keep in mind that the time is updated every time a transaction is selected to be rebroadcast.

that said, there still could be the case that the transaction is very high fee rate but hasn't been rebroadcast in a long time because it hit the MAX_REBROADCAST_COUNT. if this is the concern, we could address that by re-ordering the if / else logic in GetRebroadcastTransactions and update the timestamp if a transaction is being considered to rebroadcast, but filtered due to having hit the max. this way they would stay recent and we could continue with the same deletion logic.

Do we agree first on the problem solved by m_attempt_tracker ? See new comment : #21061 (review)

What we want to do in TrimMaxRebroadcast() is pending on this conversation imo.

ok, going to resolve this conversation here in favor of the other comment thread.

Not exactly to be included in a block as we overprice its weight with MAX_REBROADCAST_WEIGHT ?

we apply MAX_REBROADCAST_WEIGHT for calculating the set of candidates when it is time to rebroadcast, not for calculating the minimum fee rate

I don't understand how victim's node distance from miner mempools make it easier or harder to successfully conflict. Of course, you can assume that few miner mempools are running full-rbf and as such you can't leverage lack of rbf signaling as a tx-relay jamming vector but a) require victim to be strict neighbor of miner and b) attacker can exploit other relay policies differences, i.e non taproot spend allowed ?

I don't understand how victim's node distance from miner mempools make it easier or harder to successfully conflict

I'm not suggesting it is??

So what are you suggesting is how a victim node distance from a miner enters into attacker motivation to target it ? Otherwise, I would suggest just drop "distance from a miner" part, I'm not sure if it significant information here.

I'm unable to parse your question. Although I think it helps motivate the attack, I'll drop the end of the sentence.

updated to 2. An attacker targets the network by sending conflicting transactions to nodes.

Thanks, my question was simply "Why a node distance from a miner helps to motivate the attack ?"

I think you should document somewhere how filterInventoryKnown is mitigating bandwidth attacks as described by gmaxwell in your notes. Even if you can pin m_attempt_tracker permanently or for a while, most of reattempt should be ended up by being passed over and won't consume again INV bandwidth.

Though you may roll over a majority of network nodes's filterInventoryKnown with cheap-RBF replacement ? Dunno if anyone has already done the math, but it might be interesting to know them for this PR...

this comment seems to have a lot of overlap with the one you left here: #21061 (review). I don't understand exactly what you mean by "cheap-RBF replacement". I'm going to resolve this conversation, but we can continue the topic in that other thread.

See second half of other comment answer on why you can consider RBF cheap for an attacker. More generally there is also the fact that the bip125 replacement penalty is static (DEFAULT_INCREMENTAL_RELAY_FEE) is static and doesn't scale up in function of your mempool congestion.

Simplify fee-bumping algorithms a lot though at the price of bandwidth overcost for network nodes processing replacement...

W.r.t to filterInventoryKnown, a malicious roll over isn't new from introducing node rebroadcast logic, though from now this module make assumptions on it to prevent bandwidth bleeding.

I think for rational 2) to hold ("If a spy observes a bitcoin core node rebroadcasting a transaction, it would no longer know that the node has wallet enabled"), you need bandwidth increase from rebroadcast set to be low enough for not being a burden to a wide majority of node operators. Otherwise, a privacy attacker can still guess with high success that only node operators with an associated wallet will activate rebroadcasting.

Is the longer plan to activate it by default ? If so we should have really good real-world data and I think consensus of what's an acceptable bandwidth increase for the alleged benefits.

agree that we don't want rebroadcast to have high bandwidth requirements. the longer plan is to activate it by default after building confidence about resource usage. please see Merge Plan & Next Steps.

It seems a shame to add a disk read here (which gets called in net_processing's UpdatedBlockTip() callback). We could avoid this by updating the UpdatedBlockTip() signature function to also pass a const std::shared_ptr<const CBlock> &block in the same way that BlockConnected() does. It could also just pass the weight of the new block, but that seems strangely specific for the validation interface. What do you think?

yeah, I went through a similar thought process. In the initial pass, I opted for the ReadBlockFromDisk approach because it seemed simpler. But in the latest push, I updated the UpdateBlockTip function signature to also take in block. Unfortunately, it seems like callers cannot rely on block to be properly populated, so I still need to have the ReadBlockFromDisk as a fallback. The relevant code is in 33eb966d284bd954af16ed5905315e0bc91c3662 & 33eb966d284bd954af16ed5905315e0bc91c3662, let me know what you think.

This could go in the initializer list:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 9a7bc8efd3..79897845d3 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -337,7 +337,7 @@ private:
     ChainstateManager& m_chainman;
     CTxMemPool& m_mempool;
     TxRequestTracker m_txrequest GUARDED_BY(::cs_main);
-    std::unique_ptr<TxRebroadcastHandler> m_txrebroadcast;
+    const std::unique_ptr<TxRebroadcastHandler> m_txrebroadcast;
 
     /** The height of the best chain */
     std::atomic<int> m_best_height{-1};
@@ -1271,6 +1271,7 @@ PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& conn
       m_banman(banman),
       m_chainman(chainman),
       m_mempool(pool),
+      m_txrebroadcast{enable_rebroadcast ? std::make_unique<TxRebroadcastHandler>(m_mempool, m_chainman, m_chainparams) : nullptr},
       m_stale_tip_check_time(0),
       m_ignore_incoming_txs(ignore_incoming_txs)
 {
@@ -1289,9 +1290,7 @@ PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& conn
     // same probability that we have in the reject filter).
     m_recent_confirmed_transactions.reset(new CRollingBloomFilter(48000, 0.000001));
 
-    if (enable_rebroadcast) {
-        m_txrebroadcast = std::make_unique<TxRebroadcastHandler>(m_mempool, m_chainman, m_chainparams);
-
+    if (m_txrebroadcast) {
         // Run fee rate cache every minute
         scheduler.scheduleEvery([this] { m_txrebroadcast->CacheMinRebroadcastFee(); }, REBROADCAST_CACHE_FREQUENCY);
     }
[john] /home/john/Code/crypto/bitcoin

That allows you to make the m_txrebroadcast member const (which means that there can't possibly be any data races on setting/reading the pointer).

very nice, done

Don't put the value (minute) in this comment. If the constant is updated at some point in the future, this comment will become outdated. In fact, I think this comment can be removed entirely. The code is pretty self-documenting.

agree, removed.

It'd be really nice if BlockAssembler could be extracted so that rebroadcast doesn't have dependencies on the mining-specific code. That would allow you to remove m_chainman.ActiveChainstate(), m_chainparams and CScript() from this call.

yeah, that would be nice! I don't think it makes sense to increase the complexity of this PR, but I have noted it down as potential future work.

Maybe move this initialization down to below the call to CNB (just above the main for loop in this function where rebroadcast_txs is used), and then call reserve to the size of block_template->block.vtx so that we don't to multiple reallocations)

done

Could this be a const CBlockIndex& rather than pointer? ReadBlockFromDisk() unconditionally dereferences the pointer, so it can't be null.

done

Maybe add an assertion that m_attempt_tracker is held.

done

The two while loops can be consolidated:

diff --git a/src/txrebroadcast.cpp b/src/txrebroadcast.cpp
index 940735df1a..401621153c 100644
--- a/src/txrebroadcast.cpp
+++ b/src/txrebroadcast.cpp
@@ -195,18 +195,11 @@ void TxRebroadcastHandler::TrimMaxRebroadcast()
 
     while (!m_attempt_tracker->empty()) {
         auto it = m_attempt_tracker->get<index_by_last_attempt>().begin();
-        if (it->m_last_attempt < min_age) {
-            m_attempt_tracker->get<index_by_last_attempt>().erase(it);
-        } else {
+        if (it->m_last_attempt >= min_age && m_attempt_tracker->size() <= MAX_ENTRIES) {
+            // The transaction is not old enough to trim and the attempt tracker is not full
             break;
         }
     }
-
-    // If there are still too many entries, delete the oldest ones
-    while (m_attempt_tracker->size() > MAX_ENTRIES) {
-        auto it = m_attempt_tracker->get<index_by_last_attempt>().begin();
-        m_attempt_tracker->get<index_by_last_attempt>().erase(it);
-    }
 };

Perhaps that's easier to read since there's less repetition?

If you do that, then perhaps you could just remove this function entirely and put the loop inside GetRebroadcastTransactions() since it's only called in that one place.

good point on consolidating the two loops. the code you shared doesn't quite make sense (it no longer erases anything from the attempt tracker? 😛), but I got the gist and reworked the logic.

Perhaps remove this added newline?

It seems to me that m_attempt_tracker has two distinct purposes:

1. prevent us from rebroadcasting transactions too frequently (see entry_it->m_last_attempt > start_time - MIN_REATTEMPT_INTERVAL)
2. prevent us from rebroadcasting transactions too many times (see else if (entry_it->m_count >= MAX_REBROADCAST_COUNT))

I think the expiry strategies conflict for these two purposes. Imagine that we (unupgraded) have 550 txs that are no longer standard for the rest of the network

• for don't-rebroadcast-too-frequently, we want to expire least-recently-rebroadcast first, since if we expire more recently rebroadcast transactions, then we'll try to rebroadcast them again the next time we receive a block, and we'll end up rebroadcasting a very small set of txs very frequently.
• for don't-rebroadcast-too-many-times, if we expire the least-recently-rebroadcast first, then the next time we calculate a rebroadcast set, we'll include them again, re-add them to attempt_tracker, and expire the next oldest 50. We'll continue cycling all the now-nonstandard transactions through the attempt tracker over and over again.

I think it might be a good idea to separate these two purposes by adding a (rolling?) bloom filter for (2). Once a transaction has been rebroadcast MIN_REATTEMPT_INTERVAL times (or it's removed from the attempt tracker because the tracker is full) we toss it in the bloom filter and then never rebroadcast it again (at least until restart).

Quick comment noticed while writing about this review club session. IIRC it's preferable to pass uint256 by reference to const rather than by value, e.g. const uint256& txid{tx->GetHash()}; or const auto& txid = tx->GetHash(); (and here these aliases may as well be const anyway as IIUC they aren't intended to be mutated). It looks like there are a few other places.

yup, makes sense. thanks!

I've updated this site, but in all the other places that I found, was already using references. Let me know if I'm missing some?

In a3367b9b79086326f75d111d6b381088d88312f0 [p2p] Implement flag to disable rebroadcast

Should this be in net_processing.h instead of net.h? It seems more like an application-layer thing.

great point! fixed now, thanks.

Approach-related question about d72f030be9c9fa77e23132268e16708d7cbb192e [mining] Calculate the minimum fee rate for transaction inclusion

Apologies if this has already been asked. Why is a new BlockAssembler::MinTxFeeRate() function created instead of using CBlockPolicyEstimator::estimateSmartFee() with a confirmation target of 1 block or some implementation built using the fee estimator? I'm not too familiar with how it works, but it seems like a very similar idea - get an estimation of what fee would be needed to make it into the next block.

it's an interesting idea, but I don't think it makes sense to reuse. although they are answering conceptually similar questions, the way these two functions are going about answering is quite different. MinTxFeeRate is calculating very precisely based on the state of the mempool right now, where as the estimateSmartFee function is trying to predict a rate based on current conditions combined with various confidence thresholds.

(also note that a confTarget of 1 gets automatically turned into 2. I didn't dig into the function too deep to understand exactly why that's necessary, but I think indicates the divergent aims)

In 956819c69de99db84908df0fb3b25ba616f2362d [rebroadcast] Apply a fee rate filter:

What happens if our node has its own prioritized transactions, which could be returned by our own block assembler but miners wouldn't care about? I believe the block assembler uses modified feerate, so setting blockMinFeeRate here won't filter those out.

GREAT point! in the original PR (#16698), I had a commit to update the prioritisetransaction help documentation, but somehow got lost along the way to this current PR. In the latest push, I've cherry-picked that commit onto this PR.

and, to confirm, you're absolutely right. the block assembler uses modified feerate, so could select transactions to rebroadcast that the user has manually added priority.

In 4165cb0606e4be476651899b80525f63f6e4465a [rebroadcast] Stop tracking rebroadcast attempts for certain transactions

Would it be possible to write in a comment here "we want to avoid a situation where two peers that are unaware of new policy/consensus rules endlessly rebroadcast a transaction considered invalid by the majority of the network. Thus, the rebroadcast attempt tracker should remember transactions even after they have expired from the mempool." or something along those lines? I just remember it being confusing what "genuinely expire from majority of mempools" meant and why we don't remove from the tracker when a tx expires from mempool.

done, thanks! I've been having a hard time explaining, added a comment along these lines.

In 4165cb0606e4be476651899b80525f63f6e4465a [rebroadcast] Stop tracking rebroadcast attempts for certain transactions

Why not group MemPoolRemovalReason::BLOCK with the cases that we call RemoveFromAttemptTracker() for (while keeping the comment indicating that they don't go through this callback)?

sure, doesn't seem like it matters either way? but done regardless

 * - Delete tracked rebroadcast attempts for block transactions

In 0ae1420767a320cfeafd2acee0b54bc6a6eb9b27 [test] Add unit test for the fee rate cache

Why note just use + REBROADCAST_MIN_TX_AGE + 5 minutes here?

REBROADCAST_MIN_TX_AGE is defined in the .cpp, so isn't available to import in the test. I could move it to the header, but I was trying to minimize how much stuff is in the header only for tests. I could redefine the symbol as a constant in the test, but then we run into the same issue of having to update it if the code changes, so doesn't seem much better..

In 0ae1420767a320cfeafd2acee0b54bc6a6eb9b27 [test] Add unit test for the fee rate cache

Would be nice if the test also checked what happens with transactions that are prioritized.

In 87d6466380737c2d05932d4af3879950982e61c4 [test] Add unit test for rebroadcast attempt logic

I'm not sure what difference it would make, but why don't these require m_rebroadcast_mutex instead of doing the locking internally?

just so I can keep m_rebroadcast_mutex as a private member

    const bool enable_rebroadcast{args.GetArg("-rebroadcast", DEFAULT_REBROADCAST_ENABLED)};

done. note: had to change GetArg to GetBoolArg to work with braced initialization, which seems better anyways.

to avoid the recursive lock

            _RelayTransaction(ids.m_txid, ids.m_wtxid);

good call, rebased to get the internal function & updated

would be nice to select the exact permission needed

done

This gives a 1-in-10k fp rate, so once the bloom filter has had time to fill up, we should expect each node to incorrectly not rebroadcast 1-in-10k transactions. However, the nTweak parameter should ensure that this is affects different transactions on different nodes, so I think this should be fine.

I think you should be locking cs_main prior to calling this -- you want a consistent view between tip and current_fee_rate, but currently there's no reason you couldn't update the tip immediately after this line, thus associating the old tip with the new (much reduced) fee rate.

yeah makes sense, done