MiniMiner changes for package linearization

glozow commented at 2:45 pm on October 31, 2023: member

This is part of #27463. It splits off the MiniMiner-specific changes from #26711 for ease of review, as suggested in #26711 (comment).

Allow using MiniMiner on transactions that aren’t in the mempool.
Make target_feerate param of BuildMockTemplate optional, meaning “don’t stop building the template until all the transactions have been selected.”
- Add clarification for how this is different from target_feerate=0 (https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1377019133)
Track the order in which transactions are included in the template to get the “linearization order” of the transactions.
Tests

Reviewers can take a look at #26711 to see how these functions are used to linearize the AncestorPackage there.

DrahtBot commented at 2:45 pm on October 31, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, kevkevinpal, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28391 (refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access by TheCharlatan)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj added the label Mempool on Oct 31, 2023

glozow force-pushed on Oct 31, 2023

glozow commented at 5:07 pm on October 31, 2023: member

last push changed new instances of uint256 to Txid

in src/test/miniminer_tests.cpp:325 in a5b6481b94 outdated

320+        const int32_t tx4_vsize{tx_dims.at(tx4->GetHash()).vsize};
321+        const int32_t tx5_vsize{tx_dims.at(tx5->GetHash()).vsize};
322+        const int32_t tx6_vsize{tx_dims.at(tx6->GetHash()).vsize};
323+        const int32_t tx7_vsize{tx_dims.at(tx7->GetHash()).vsize};
324+
325+        CAmount tx5_mod_fee{low_fee + CENT/100};

kevkevinpal commented at 2:09 am on November 2, 2023:

nit: I just noticed CENT/100 is used in this file for tx 5’s fee aswell, not sure if it makes sense to make a common variable to use for both

glozow commented at 10:22 am on November 3, 2023:

added a refactor commit to pull fee amounts into constants

in src/test/miniminer_tests.cpp:673 in a5b6481b94 outdated

668+        miniminer_info.emplace_back(         10000,             10000,               100,                   200, parent_high_feerate);
669+        miniminer_info.emplace_back(           200,               200,               100,                   100, grandparent_double_low_feerate);
670+        miniminer_info.emplace_back(          1000,              1200,               100,                   200, parent_med_feerate);
671+        miniminer_info.emplace_back(           100,               100,               100,                   100, grandparent_low_feerate);
672+        miniminer_info.emplace_back(           200,               300,               100,                   200, parent_double_low_feerate);
673+        miniminer_info.emplace_back(            10,             11510,               100,                   700, child);

kevkevinpal commented at 2:17 am on November 2, 2023:

nit: Should we make some of these values into a variable instead of hardcoding the number?

glozow commented at 10:22 am on November 3, 2023:

done

in src/node/mini_miner.cpp:13 in d525998b07 outdated

 3@@ -4,9 +4,7 @@
 4 
 5 #include <node/mini_miner.h>
 6 
 7-#include <consensus/amount.h>
 8-#include <policy/feerate.h>
 9-#include <primitives/transaction.h>
10+#include <txmempool.h>

TheCharlatan commented at 4:51 pm on November 2, 2023:

In commit d525998b07be066c97a36536b58f0b98d816be39

Since you are changing the headers here, can you make them closer to what IWYU is suggesting?

glozow commented at 10:23 am on November 3, 2023:

Done, and updated the expected boost includes for the linter. Unsure if it’s preferred to omit those…?

achow101 commented at 5:48 pm on November 2, 2023: member

ACK a5b6481b94580ef2cc2d9c45ac1c1959e79a82b2

kevkevinpal commented at 7:24 pm on November 2, 2023: contributor

ACK a5b6481b94580ef2cc2d9c45ac1c1959e79a82b2

added few small nits but feel free to ignore

in src/node/mini_miner.cpp:136 in 11edc89bd8 outdated

124@@ -125,6 +125,48 @@ MiniMiner::MiniMiner(const CTxMemPool& mempool, const std::vector<COutPoint>& ou
125     SanityCheck();
126 }
127 
128+MiniMiner::MiniMiner(const std::vector<MiniMinerMempoolEntry>& manual_entries,
129+                     const std::map<Txid, std::set<Txid>>& descendant_caches)

TheCharlatan commented at 9:31 pm on November 2, 2023:

Nit: The naming is confusing further down in the for loop, but I can’t think of anything that’s really better.

glozow commented at 10:23 am on November 3, 2023:

leaving it as is :sweat_smile:

murchandamus commented at 3:03 pm on November 3, 2023:

I also found manual_entries vs m_entries make me check back a couple times. My understanding is that the manual_entries are just fully specified rather than read from the mempool. Would maybe manually_submitted_entries fit the bill?

TheCharlatan commented at 10:07 pm on November 2, 2023: contributor

Concept ACK

lgtm, but can’t really comment on the breadth of test cases.

[lint] update expected boost includes 4aa98b79b2

[refactor] change MiniMinerMempoolEntry ctor to take values, update includes

No behavior change. All we are doing is copying out these values before
passing them into the ctor instead of within the ctor.

This makes it possible to use the MiniMiner algorithms to analyze
transactions that haven't been submitted to the mempool yet.

It also iwyu's the mini_miner includes.

e3b2e630b2

[MiniMiner] allow manual construction with non-mempool txns

This is primarily intended for linearizing a package of transactions
prior to submitting them to mempool. Note that, if this ctor is used,
bump fees will not be calculated because we haven't instructed MiniMiner
which outpoints for which we want bump fees to be calculated.

5a83f55c96

[MiniMiner] make target_feerate optional

Add an option to keep building the template regardless of feerate. We
can't just use target_feerate=0 because it's possible for transactions
to have negative modified feerates.

No behavior change for users that pass in a target_feerate.

fe6332c0ba

[test] add case for MiniMiner working with negative fee txns 004075963f

[MiniMiner] track inclusion order and add Linearize() function

Sometimes we are just interested in the order in which transactions
would be included in a block (we want to "linearize" the transactions).
Track and store this information.

This doesn't change any of the bump fee calculations.

f4b1b24a3b

[refactor] unify fee amounts in miniminer_tests

Name {low,med,high}_fee constants for reuse across file.

dfd6a3788c

glozow force-pushed on Nov 3, 2023

[test] MiniMiner::Linearize and manual construction d9cc99d04e

glozow force-pushed on Nov 3, 2023

DrahtBot added the label CI failed on Nov 3, 2023

TheCharlatan commented at 11:44 am on November 3, 2023: contributor

ACK d9cc99d04e813caed51c5f7b6ebdc4c39c8823c9

DrahtBot requested review from achow101 on Nov 3, 2023

DrahtBot requested review from kevkevinpal on Nov 3, 2023

DrahtBot removed the label CI failed on Nov 3, 2023

kevkevinpal commented at 2:16 pm on November 3, 2023: contributor

reACK d9cc99d

DrahtBot removed review request from kevkevinpal on Nov 3, 2023

in src/node/mini_miner.h:41 in e3b2e630b2 outdated

45-        vsize_with_ancestors(entry->GetSizeWithAncestors())
46+    explicit MiniMinerMempoolEntry(CAmount fee_self,
47+                                   CAmount fee_ancestor,
48+                                   int64_t vsize_self,
49+                                   int64_t vsize_ancestor,
50+                                   const CTransactionRef& tx_in):

murchandamus commented at 2:31 pm on November 3, 2023:

Would it be possible for these parameters to be in the same order as the assignments below?

achow101 commented at 2:39 pm on November 3, 2023: member

re-ACK d9cc99d04e813caed51c5f7b6ebdc4c39c8823c9

DrahtBot removed review request from achow101 on Nov 3, 2023

achow101 merged this on Nov 3, 2023

achow101 closed this on Nov 3, 2023

in src/node/mini_miner.cpp:157 in 5a83f55c96 outdated

152+        // Descendant cache should include at least the tx itself.
153+        if (!Assume(!desc_txids.empty())) {
154+            m_ready_to_calculate = false;
155+            return;
156+        }
157+        std::vector<MockEntryMap::iterator> cached_descendants;

murchandamus commented at 2:58 pm on November 3, 2023:

Is it important that these descendants are cached? Could they maybe just be descendants? The cached_descendants vs descendant_caches is a bit confusing.

in src/node/mini_miner.cpp:297 in fe6332c0ba outdated

292@@ -292,7 +293,11 @@ void MiniMiner::BuildMockTemplate(const CFeeRate& target_feerate)
293         DeleteAncestorPackage(ancestors);
294         SanityCheck();
295     }
296-    Assume(m_in_block.empty() || m_total_fees >= target_feerate.GetFee(m_total_vsize));
297+    if (!target_feerate.has_value()) {
298+        Assume(m_in_block.size() == num_txns);

murchandamus commented at 3:12 pm on November 3, 2023:

It took me until the commit [MiniMiner] track inclusion order and add Linearize() function until I understood what this PR was aiming to achieve. Perhaps you could add another sentence to the PR description about the goal of this PR.

in src/test/miniminer_tests.cpp:316 in d9cc99d04e

309@@ -309,6 +310,64 @@ BOOST_FIXTURE_TEST_CASE(miniminer_1p1c, TestChain100Setup)
310             }
311         }
312     }
313+
314+    // Check m_inclusion_order for equivalent mempool- and manually-constructed MiniMiners.
315+    // (We cannot check bump fees in manually-constructed MiniMiners because it doesn't know what
316+    // outpoints are requested).

murchandamus commented at 3:38 pm on November 3, 2023:

Do we not know any outpoints or do we just not know whether the ancestor transactions of the submitted set are confirmed?

glozow commented at 5:50 pm on November 3, 2023:

It’s just “I can’t give an answer if I don’t know what the question is.” When using this constructor, the user is not specifying which outpoints they want bump fees for.

in src/test/miniminer_tests.cpp:650 in d9cc99d04e

641@@ -539,4 +642,64 @@ BOOST_FIXTURE_TEST_CASE(calculate_cluster, TestChain100Setup)
642     }
643 }
644 
645+BOOST_FIXTURE_TEST_CASE(manual_ctor, TestChain100Setup)
646+{
647+    CTxMemPool& pool = *Assert(m_node.mempool);
648+    LOCK2(cs_main, pool.cs);
649+    {
650+        // 3 pairs of fee-bumping grandparent + parent, plus 1 low-feerate child.

murchandamus commented at 3:44 pm on November 3, 2023:

Isn’t it the parent that is fee-bumping?

0        // 3 pairs of grandparent + fee-bumping parent, plus 1 low-feerate child.

murchandamus commented at 3:49 pm on November 3, 2023: contributor

I guess now post-merge ACK d9cc99d04e813caed51c5f7b6ebdc4c39c8823c9

in src/test/miniminer_tests.cpp:696 in d9cc99d04e

691+
692+        // CPFP double low + med
693+        BOOST_CHECK_EQUAL(sequences.at(grandparent_double_low_feerate->GetHash()), 1);
694+        BOOST_CHECK_EQUAL(sequences.at(parent_med_feerate->GetHash()), 1);
695+
696+        // CPFP low + med

theStack commented at 5:18 pm on November 3, 2023:

0        // CPFP low + double low

theStack commented at 5:21 pm on November 3, 2023: contributor

post-merge code-review ACK d9cc99d04e813caed51c5f7b6ebdc4c39c8823c9

glozow deleted the branch on Nov 3, 2023

glozow commented at 5:52 pm on November 3, 2023: member

Thanks for all the reviews :)

in src/node/mini_miner.h:130 in 5a83f55c96 outdated

125+     * MiniMinerMempoolEntrys.
126+    */
127     MiniMiner(const CTxMemPool& mempool, const std::vector<COutPoint>& outpoints);
128 
129+    /** Constructor in which the MiniMinerMempoolEntry entries have been constructed manually,
130+     * presumably because these transactions are not in the mempool (yet). It is assumed that all

ismaelsadeeq commented at 9:57 am on November 6, 2023:

I think just stating that this is a constructor where MiniMinerMempoolEntry are constructed manually is enough as this can be used with mempool transactions, but with the intent of building a block template to get a mempool fee rate histogram

0    * It is assumed that all

ismaelsadeeq commented at 10:58 am on November 6, 2023: member

Strong ACK d9cc99d04e813caed51c5f7b6ebdc4c39c8823c9

The whole mempool transactions can also be used to create manual_entries and descendant_caches and use the new constructor to build a block template to get a fee rate histogram of the block template, with just a modification to BuildMockTemplate to enable getting the block template fee rate histogram. it will be more desirable to modify this than BlockAssembler.

Due to the use of GatherClusters, which has a 500 transaction DOS limit, Mini Miner cannot be used to create a block template using the entire mempool with the initial constructor.

glozow referenced this in commit d60ebea597 on Nov 9, 2023

in src/node/mini_miner.h:34 in d9cc99d04e

32     int64_t vsize_with_ancestors;
33+    const CAmount fee_individual;
34+    CAmount fee_with_ancestors;
35 
36 // This class must be constructed while holding mempool.cs. After construction, the object's
37 // methods can be called without holding that lock.

stickies-v commented at 11:05 am on November 9, 2023:

nit: I think this docstring should now be removed, I don’t think the constructor requires a mempool.cs lock anymore?

glozow commented at 10:48 am on November 10, 2023:

There are 2 constructors, one requires mempool (and its lock) and the other one doesn’t.

glozow commented at 10:49 am on November 10, 2023:

Oh I see this is not in the best place - perhaps this should be in the docstring for the with-mempool ctor.

MiniMiner changes for package linearization #28762

Code Coverage

Reviews

Conflicts