Alternative approach to #31003, suggested in #31109 (comment).
This PR introduces waitNext(). It waits for either the tip to update or for fees at the top of the mempool to rise sufficiently. It then returns a new template, with which the caller can rinse and repeat.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31283.
See the guideline for information on the review process. A summary of reviews will appear here.
Reviewers, this pull request conflicts with the following ones:
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.
993+ }
994+
995+ return block_template;
996+ }
997+
998+ const CScript m_script_pub_key;
script_pub_key around. Afaik it’s only used by tests and for solo CPU mining.
937@@ -938,9 +938,71 @@ class BlockTemplateImpl : public BlockTemplate
938 return chainman().ProcessNewBlock(block_ptr, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/nullptr);
939 }
940
941+ std::unique_ptr<BlockTemplate> waitNext(CAmount fee_threshold, MillisecondsDouble timeout) override
942+ {
943+ if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono
I dropped this line.
Additionally this PR does not make use of last_mempool_update, since it’s not very useful in a typical production setting where something in the mempool updates all the time.
I think the race condition mentioned there is not an issue here because we don’t pass in the current fees via argument, but instead derive them from the original template.
I did forget to update now at the end of the loop, which I fixed.
miner_tests to use the Mining interface. I plan to expand those tests to cover waitNext().
71-BlockAssembler MinerTestingSetup::AssemblerForTest(CTxMemPool& tx_mempool)
72-{
73- BlockAssembler::Options options;
74-
75- options.nBlockMaxWeight = MAX_BLOCK_WEIGHT;
76- options.blockMinFeeRate = blockMinFeeRate;
684 }
685- std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(*pblock);
686- BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlock(shared_pblock, true, true, nullptr));
687- pblock->hashPrevBlock = pblock->GetHash();
688+ std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(block);
689+ BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(shared_pblock, /*force_processing=*/true, /*min_pow_checked=*/true, nullptr));
miner.submitSolution() here.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32946689392
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
ProcessNewBlock and submitSolution via the Mining interface. The latter approach is much simpler, but both are used in production code, so should be covered.
waitNext().
In commit f9200f6c65b5a97a4842cc1ec34e688ec13bffcd
“Use createNewBlock via the interface instead of calling Chainman’s CreateNewBlock.”
Do you mean “Use createNewBlock via the interface instead of calling CreateNewBlock through the BlockAssembler directly”? There is no CreateNewBlock in the chainman.
987+ CAmount new_fees = 0;
988+ for (CAmount fee : block_template->m_block_template->vTxFees) {
989+ // Skip coinbase
990+ if (fee < 0) continue;
991+ new_fees += fee;
992+ if (new_fees >= current_fees + fee_threshold) return block_template;
fee_threshold of 0 combined with a timeout of 0 will immediately return a new template. This is useful for the Template Provider, because it needs to unconditionally send new templates to all connected clients if fees increased sufficiently for one its connected clients. (at least until cluster mempool makes the fee calculation trivially cheap)
Rebased after #31288 and #31122.
This PR is now based on #31325 since that has enough support. Keeping this draft until that’s merged (or dropped).
The test changes will probably cause a merge conflict with #31318, but it should be trivial. Ditto for the waitNext() implementation since that would no longer need to pump script_pub_key around.
I dropped the early return in waitNext() because wait_until should already take care of that. I also made the while loop use <= instead of < so this actually works: #31283 (review) Added a test for that.
958+ WAIT_LOCK(notifications().m_tip_block_mutex, lock);
959+ notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
960+ // Although C++17 allows safe comparison of optional<T> with
961+ // another T, we need to wait for m_tip_block to be set AND
962+ // for the value to be different than the current_tip value.
963+ tip_changed = notifications().m_tip_block && notifications().m_tip_block != m_block_template->block.hashPrevBlock;
If #31346 lands first, notifications().m_tip_block could be an Assert and the comment removed, see #31346 (comment)
Though I have to make sure there’s no race condition where you call createNewBlock() followed by waitNext() right at node startup, without first calling waitTipChanged(ZERO).
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/34571636339
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Alternate calls between Chainman's ProcessNewBlock and submitSolution. The Chainman code path is used in production, so it's important to keep test coverage for it.
Use createNewBlock via the interface instead of using the BlockAssembler directly. The latter is always called via the interface in production code.