Add waitNext() to BlockTemplate interface #31283

Sjors · 2024-11-13T12:30:36Z

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.

On testnet3 and testnet4 the difficulty drops after 20 minutes, so the second ensures that a new template is returned in that case.

Alternative approach to #31003, suggested in #31109 (comment)

DrahtBot · 2024-11-13T12:30:39Z

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31283.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26966 (index: initial sync speedup, parallelize process by furszy)
#26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)

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.

Sjors · 2024-11-13T12:34:38Z

src/node/interfaces.cpp

+        return block_template;
+    }
+
+    const CScript m_script_pub_key;


Separate from this PR, I'm looking into whether we can avoid passing script_pub_key around. Afaik it's only used by tests and for solo CPU mining.

#31318 drops this argument. It's only slightly easier to merge that first, so I'll leave this PR open.

Sjors · 2024-11-13T12:37:51Z

src/node/interfaces.cpp

@@ -938,9 +938,71 @@ class BlockTemplateImpl : public BlockTemplate
        return chainman().ProcessNewBlock(block_ptr, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/nullptr);
    }

+    std::unique_ptr<BlockTemplate> waitNext(CAmount fee_threshold, MillisecondsDouble timeout) override
+    {
+        if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono


I still need to study #31003 (comment) in more detail to see if this code isn't making similar mistakes.

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.

Sjors · 2024-11-13T19:46:36Z

I pushed a commit refactoring miner_tests to use the Mining interface. I plan to expand those tests to cover waitNext().

Sjors · 2024-11-13T19:47:31Z

src/test/miner_tests.cpp

-    BlockAssembler::Options options;
-
-    options.nBlockMaxWeight = MAX_BLOCK_WEIGHT;
-    options.blockMinFeeRate = blockMinFeeRate;


a316ba5: I think dropping this doesn't matter?

src/test/miner_tests.cpp

DrahtBot · 2024-11-13T20:24:19Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/32946689392}

Hints

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.

Sjors · 2024-11-14T12:17:23Z

I made the test code alternate calls between Chainman's ProcessNewBlock and submitSolution via the Mining interface. The latter approach is much simpler, but both are used in production code, so should be covered.

Sjors · 2024-11-14T13:05:23Z

I added test coverage for waitNext().

TheCharlatan · 2024-11-14T13:18:28Z

In commit f9200f6

"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.

Sjors · 2024-11-14T13:31:07Z

Made some adjustments based on #31283 (comment)

Sjors · 2024-11-14T13:33:54Z

@TheCharlatan I adjusted the commit message

Sjors · 2024-12-18T03:44:57Z

Fixed tidy error.

Sjors · 2024-12-19T01:32:32Z

The changes here don't conflict with the merged #31196, but I rebased just in case.

Sjors · 2024-12-20T01:44:45Z

#31325 landed so this is ready for review.

Sjors · 2024-12-30T16:37:34Z

I extracted the first commit into its own PR #31581. Review on the second commit is still welcome here, since I don't expect it to change. Marking draft.

Sjors · 2025-01-06T17:25:18Z

Rebased on the latest #31581.

I added a commit that on testnet3 and testnet4 returns a new template after 20 minutes, to take advantage of the min difficulty rule.

Since miner_tests.cpp uses mainnet, I created testnet4_miner_tests.cpp to cover this behaviour in the unit tests. A functional test could be added later using the testnet4 blocks added by #31583. This could check if the difficulty is correct.

I also switched to using NodeClock::now() to be able to use mock time in tests.

DrahtBot · 2025-01-06T17:45:04Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/35210885796}

Hints

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.

0424968 test: use Mining interface in miner_tests (Sjors Provoost) Pull request description: Needed for both #31283 and #31564. By using the Mining interface in `miner_tests.cpp` we increase its coverage in unit tests. ACKs for top commit: achow101: ACK 0424968 ryanofsky: Code review ACK 0424968, just minor suggested changes (renames, comments, BOOST_REQUIREs) since last review and some more extra clarifications and checks added to the CreateNewBlock_validity test. The CreateNewBlock_validity changes seem clear and easy to understand now. vasild: ACK 0424968 tdb3: ACK 0424968 Tree-SHA512: 2761cb7555d759670e40d8f37b96a079f8e12a588ac43313b9e63c69afd478321515873a8896ea56784f0100dac4476b0c0e0ef8b5418f8aea24d9965cace4d4

Sjors · 2025-01-07T07:31:38Z

Rebased after #31581 landed, marking ready for review again.

If I need to retouch I'll fix the typo mentioned in #31581 (comment) and maybe add a helper to the test suggested here #31581 (comment).

This was referenced Nov 13, 2024

Add waitFeesChanged() to Mining interface #31003

Closed

Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants #31109

Open

Sjors commented Nov 13, 2024

View reviewed changes

DrahtBot mentioned this pull request Nov 13, 2024

refactor: mining interface 30955 followups #31197

Merged

Sjors commented Nov 13, 2024

View reviewed changes

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

DrahtBot added the CI failed label Nov 13, 2024

Sjors force-pushed the 2024/11/wait-next branch from a316ba5 to 6b9b3a1 Compare November 13, 2024 20:52

DrahtBot removed the CI failed label Nov 13, 2024

Sjors force-pushed the 2024/11/wait-next branch from 6b9b3a1 to 53f2845 Compare November 14, 2024 12:16

Sjors force-pushed the 2024/11/wait-next branch 3 times, most recently from 936f055 to 41f83a2 Compare November 14, 2024 13:04

Sjors marked this pull request as ready for review November 14, 2024 13:05

Sjors force-pushed the 2024/11/wait-next branch 2 times, most recently from bc05bc0 to c931a47 Compare November 14, 2024 13:16

Sjors force-pushed the 2024/11/wait-next branch from c931a47 to 171c116 Compare November 14, 2024 13:30

Sjors force-pushed the 2024/11/wait-next branch from 171c116 to e5a9b73 Compare November 14, 2024 13:33

This was referenced Nov 14, 2024

Stratum v2 via IPC Mining Interface tracking issue #31098

Open

Add destroy to BlockTemplate schema #31288

Merged

Stratum v2 Template Provider common functionality Sjors/bitcoin#49

Open

DrahtBot removed the Needs rebase label Dec 18, 2024

Sjors force-pushed the 2024/11/wait-next branch from 618398b to 7a505f5 Compare December 18, 2024 03:44

DrahtBot removed the CI failed label Dec 18, 2024

Sjors force-pushed the 2024/11/wait-next branch from 7a505f5 to 3fd2daf Compare December 19, 2024 01:32

Sjors force-pushed the 2024/11/wait-next branch from 3fd2daf to a19a5f9 Compare December 20, 2024 01:43

Sjors marked this pull request as ready for review December 20, 2024 01:44

Sjors mentioned this pull request Dec 24, 2024

rpc: checkblock, add checkBlock() to Mining interface Sjors/bitcoin#75

Closed

Sjors force-pushed the 2024/11/wait-next branch from a19a5f9 to 3827fd2 Compare December 24, 2024 12:17

This was referenced Dec 24, 2024

Add checkblock RPC and checkBlock() to Mining interface #31564

Draft

test: have miner_tests use Mining interface #31581

Merged

Sjors marked this pull request as draft December 30, 2024 16:38

Sjors force-pushed the 2024/11/wait-next branch from 3827fd2 to a43ea8c Compare January 6, 2025 17:25

Sjors force-pushed the 2024/11/wait-next branch from a43ea8c to da305ac Compare January 6, 2025 17:44

DrahtBot added the CI failed label Jan 6, 2025

DrahtBot removed the CI failed label Jan 6, 2025

Sjors added 2 commits January 7, 2025 08:29

Add waitNext() to BlockTemplate interface

956b449

On testnet create min diff template after 20 mins

8d54cfd

Sjors force-pushed the 2024/11/wait-next branch from da305ac to 8d54cfd Compare January 7, 2025 07:31

Sjors marked this pull request as ready for review January 7, 2025 07:31

This was referenced Jan 7, 2025

index: initial sync speedup, parallelize process #26966

Draft

test: add end-to-end tests for CConnman and PeerManager #26812

Open

Sjors mentioned this pull request Jan 8, 2025

mining: bugfix: Fix duplicate coinbase tx weight reservation #31384

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add waitNext() to BlockTemplate interface #31283

Add waitNext() to BlockTemplate interface #31283

Sjors commented Nov 13, 2024 •

edited

Loading

DrahtBot commented Nov 13, 2024 •

edited

Loading

Sjors Nov 13, 2024

Sjors Nov 18, 2024

Sjors Nov 13, 2024

Sjors Nov 14, 2024

Sjors commented Nov 13, 2024

Sjors Nov 13, 2024

DrahtBot commented Nov 13, 2024

Sjors commented Nov 14, 2024 •

edited

Loading

Sjors commented Nov 14, 2024

TheCharlatan commented Nov 14, 2024

Sjors commented Nov 14, 2024

Sjors commented Nov 14, 2024

Sjors commented Dec 18, 2024

Sjors commented Dec 19, 2024

Sjors commented Dec 20, 2024

Sjors commented Dec 30, 2024

Sjors commented Jan 6, 2025 •

edited

Loading

DrahtBot commented Jan 6, 2025

Sjors commented Jan 7, 2025 •

edited

Loading

Add waitNext() to BlockTemplate interface #31283

Are you sure you want to change the base?

Add waitNext() to BlockTemplate interface #31283

Conversation

Sjors commented Nov 13, 2024 • edited Loading

DrahtBot commented Nov 13, 2024 • edited Loading

Code Coverage & Benchmarks

Reviews

Conflicts

Sjors Nov 13, 2024

Choose a reason for hiding this comment

Sjors Nov 18, 2024

Choose a reason for hiding this comment

Sjors Nov 13, 2024

Choose a reason for hiding this comment

Sjors Nov 14, 2024

Choose a reason for hiding this comment

Sjors commented Nov 13, 2024

Sjors Nov 13, 2024

Choose a reason for hiding this comment

DrahtBot commented Nov 13, 2024

Sjors commented Nov 14, 2024 • edited Loading

Sjors commented Nov 14, 2024

TheCharlatan commented Nov 14, 2024

Sjors commented Nov 14, 2024

Sjors commented Nov 14, 2024

Sjors commented Dec 18, 2024

Sjors commented Dec 19, 2024

Sjors commented Dec 20, 2024

Sjors commented Dec 30, 2024

Sjors commented Jan 6, 2025 • edited Loading

DrahtBot commented Jan 6, 2025

Sjors commented Jan 7, 2025 • edited Loading

Sjors commented Nov 13, 2024 •

edited

Loading

DrahtBot commented Nov 13, 2024 •

edited

Loading

Sjors commented Nov 14, 2024 •

edited

Loading

Sjors commented Jan 6, 2025 •

edited

Loading

Sjors commented Jan 7, 2025 •

edited

Loading