MiniMiner changes for package linearization #28762

glozow · 2023-10-31T14:45:10Z

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 (validate package transactions with their in-package ancestor sets #26711 (comment))
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 · 2023-10-31T14:45:13Z

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.

glozow · 2023-10-31T17:07:31Z

last push changed new instances of uint256 to Txid

achow101 · 2023-11-02T17:48:30Z

ACK a5b6481b94580ef2cc2d9c45ac1c1959e79a82b2

kevkevinpal

ACK a5b6481b94580ef2cc2d9c45ac1c1959e79a82b2

added few small nits but feel free to ignore

src/test/miniminer_tests.cpp

sedited

Concept ACK

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

src/node/mini_miner.cpp

…ncludes 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.

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.

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.

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.

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

sedited · 2023-11-03T11:44:14Z

ACK d9cc99d

kevkevinpal · 2023-11-03T14:16:40Z

reACK d9cc99d

achow101 · 2023-11-03T14:39:18Z

re-ACK d9cc99d

murchandamus

I guess now post-merge ACK d9cc99d

murchandamus · 2023-11-03T14:31:19Z

src/node/mini_miner.h

+    explicit MiniMinerMempoolEntry(CAmount fee_self,
+                                   CAmount fee_ancestor,
+                                   int64_t vsize_self,
+                                   int64_t vsize_ancestor,
+                                   const CTransactionRef& tx_in):


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

murchandamus · 2023-11-03T14:58:33Z

src/node/mini_miner.cpp

+            m_ready_to_calculate = false;
+            return;
+        }
+        std::vector<MockEntryMap::iterator> cached_descendants;


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

src/node/mini_miner.cpp

murchandamus · 2023-11-03T15:12:43Z

src/node/mini_miner.cpp

    }
-    Assume(m_in_block.empty() || m_total_fees >= target_feerate.GetFee(m_total_vsize));
+    if (!target_feerate.has_value()) {
+        Assume(m_in_block.size() == num_txns);


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.

murchandamus · 2023-11-03T15:38:39Z

src/test/miniminer_tests.cpp

+    // (We cannot check bump fees in manually-constructed MiniMiners because it doesn't know what
+    // outpoints are requested).


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

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.

murchandamus · 2023-11-03T15:44:27Z

src/test/miniminer_tests.cpp

+    CTxMemPool& pool = *Assert(m_node.mempool);
+    LOCK2(cs_main, pool.cs);
+    {
+        // 3 pairs of fee-bumping grandparent + parent, plus 1 low-feerate child.


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

Suggested change

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

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

theStack

post-merge code-review ACK d9cc99d

theStack · 2023-11-03T17:18:46Z

src/test/miniminer_tests.cpp

+        BOOST_CHECK_EQUAL(sequences.at(grandparent_double_low_feerate->GetHash()), 1);
+        BOOST_CHECK_EQUAL(sequences.at(parent_med_feerate->GetHash()), 1);
+
+        // CPFP low + med


Suggested change

// CPFP low + med

// CPFP low + double low

glozow · 2023-11-03T17:52:02Z

Thanks for all the reviews :)

ismaelsadeeq

Strong ACK d9cc99d

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.

ismaelsadeeq · 2023-11-06T09:57:41Z

src/node/mini_miner.h

    MiniMiner(const CTxMemPool& mempool, const std::vector<COutPoint>& outpoints);

+    /** Constructor in which the MiniMinerMempoolEntry entries have been constructed manually,
+     * presumably because these transactions are not in the mempool (yet). It is assumed that all


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

Suggested change

* presumably because these transactions are not in the mempool (yet). It is assumed that all

* It is assumed that all

…n followups b4b01d3 [refactor] updating miniminer comments to be more accurate (kevkevin) 83933ef [refactor] Miniminer var cached_descendants to descendants (kevkevin) 43423fd [refactor] Change MiniMinerMempoolEntry order (kevkevin) Pull request description: ### Motivation In bitcoin/bitcoin#28762 there were some post merge comments which are being addressed in this PR with the following commits ### [8d4c46f](kevkevinpal/bitcoin@8d4c46f) Reorganizing `MiniMinerMempoolEntry` to match the order we have elsewhere * bitcoin/bitcoin#28762 (comment) ### [7505ec2](kevkevinpal/bitcoin@7505ec2) Renaming `cached_descendants` to `descendants` for simpler variable naming * bitcoin/bitcoin#28762 (comment) ### [b21f2f2](kevkevinpal/bitcoin@b21f2f2) Code comment modifications to be more accurate to what is actually happening * bitcoin/bitcoin#28762 (comment) and * bitcoin/bitcoin#28762 (comment) and * bitcoin/bitcoin#28762 (comment) ACKs for top commit: murchandamus: reACK b4b01d3 theStack: LGTM ACK b4b01d3 Tree-SHA512: 54f044a578fb203d8a3c1aa0bcd1fc4bcdff0bc9b024351925a4caf0ccece7c7736b0694ad1168c3cbb447bdb58a91f4cac365f46114da29a889fbc8ea595b82

stickies-v · 2023-11-09T11:05:48Z

src/node/mini_miner.h

 // This class must be constructed while holding mempool.cs. After construction, the object's
 // methods can be called without holding that lock.


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

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

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

glozow mentioned this pull request Oct 31, 2023

validate package transactions with their in-package ancestor sets #26711

Closed

laanwj added the Mempool label Oct 31, 2023

DrahtBot mentioned this pull request Oct 31, 2023

refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access #28391

Merged

glozow force-pushed the 2023-10-26711-miniminer branch from 58ef23a to a5b6481 Compare October 31, 2023 17:06

glozow mentioned this pull request Nov 2, 2023

Package Relay Project Tracking #27463

Open

57 tasks

kevkevinpal reviewed Nov 2, 2023

View reviewed changes

src/test/miniminer_tests.cpp Outdated Show resolved Hide resolved

src/test/miniminer_tests.cpp Outdated Show resolved Hide resolved

sedited reviewed Nov 2, 2023

View reviewed changes

src/node/mini_miner.cpp Outdated Show resolved Hide resolved

src/node/mini_miner.cpp Outdated Show resolved Hide resolved

glozow added 7 commits November 3, 2023 10:17

[lint] update expected boost includes

4aa98b7

[MiniMiner] make target_feerate optional

fe6332c

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.

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

0040759

[refactor] unify fee amounts in miniminer_tests

dfd6a37

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

glozow force-pushed the 2023-10-26711-miniminer branch from a5b6481 to c83ab64 Compare November 3, 2023 10:21

[test] MiniMiner::Linearize and manual construction

d9cc99d

glozow force-pushed the 2023-10-26711-miniminer branch from c83ab64 to d9cc99d Compare November 3, 2023 10:39

DrahtBot added the CI failed label Nov 3, 2023

DrahtBot requested review from achow101 and kevkevinpal November 3, 2023 11:44

DrahtBot removed the CI failed label Nov 3, 2023

DrahtBot removed the request for review from kevkevinpal November 3, 2023 14:16

DrahtBot removed the request for review from achow101 November 3, 2023 14:39

achow101 merged commit d9007f5 into bitcoin:master Nov 3, 2023

murchandamus reviewed Nov 3, 2023

View reviewed changes

theStack reviewed Nov 3, 2023

View reviewed changes

glozow deleted the 2023-10-26711-miniminer branch November 3, 2023 17:51

ismaelsadeeq reviewed Nov 6, 2023

View reviewed changes

ismaelsadeeq mentioned this pull request Nov 6, 2023

Improving fee estimation accuracy #27995

Open

This was referenced Nov 7, 2023

refactor: Miniminer linearization followups kevkevinpal/bitcoin#5

Merged

refactor: Miniminer package linearization followups #28808

Merged

stickies-v reviewed Nov 9, 2023

View reviewed changes

bitcoin locked and limited conversation to collaborators Dec 3, 2024

		// (We cannot check bump fees in manually-constructed MiniMiners because it doesn't know what
		// outpoints are requested).

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

	* presumably because these transactions are not in the mempool (yet). It is assumed that all
	* It is assumed that all

		// This class must be constructed while holding mempool.cs. After construction, the object's
		// methods can be called without holding that lock.

MiniMiner changes for package linearization #28762

MiniMiner changes for package linearization #28762

Uh oh!

Conversation

glozow commented Oct 31, 2023

Uh oh!

DrahtBot commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

glozow commented Oct 31, 2023

Uh oh!

achow101 commented Nov 2, 2023

Uh oh!

kevkevinpal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sedited commented Nov 3, 2023

Uh oh!

kevkevinpal commented Nov 3, 2023

Uh oh!

achow101 commented Nov 3, 2023

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow commented Nov 3, 2023

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

DrahtBot commented Oct 31, 2023 •

edited

Loading