Have createNewBlock() return a BlockTemplate interface by Sjors · Pull Request #30440 · bitcoin/bitcoin

Sjors · 2024-07-12T13:33:59Z

An external program that uses the Mining interface may need quick access to some information in the block template, while it can wait a bit longer for the full raw transaction data.

This would be the case for a Stratum v2 Template Provider which needs to send a NewTemplate message message (which doesn't include transactions) as quickly as possible. It does not include the serialized block transactions.

DrahtBot · 2024-07-12T13:34:02Z

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	ryanofsky, itornaza, TheCharlatan, 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:

#30437 (multiprocess: add bitcoin-mine test program by ryanofsky)
#30409 (Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block by Sjors)
#29039 (versionbits refactoring by ajtowns)
#27433 (getblocktemplate improvements for segwit and sigops by Sjors)

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.

DrahtBot · 2024-07-12T13:59:38Z

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

Hints

Make sure to run all tests locally, according to the documentation.

The failure may 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-07-15T08:03:01Z

In #29432 (comment) @TheBlueMatt wrote:

Any IPC thing that requires serializing all the transactions for all work is going to be too slow. Bitcoin Core needs to be able to track the templates it gave out and keep them around (to keep the Transaction shared_ptrs in a block ready to go) until they time out.

This PR returns a BlockTemplate interface which causes the node to hold on to the template and its block.

The goal is to be able to quickly send NewTemplate and after that to copy all transactions to the external application so it can respond to RequestTransactionData and SubmitSolution.

I assume your concern is that this would make SubmitSolution too slow because the external application needs to pass the whole block back via IPC? Or are you even worried that the TP wouldn't have the full block ready by the time RequestTransactionData comes in?

I could make submitSolution a method on the BlockTemplate interface. This would allow the external application to hold on to the BlockTemplate interface all the way until a solution comes in. This does move the burden of template memory management to the node.

Would it need a way to unilaterally drop templates, or to tell the external application to pick which templates to drop?

Benchmarks would be useful here.

Sjors · 2024-07-15T08:56:22Z

Pushed a commit that adds submitSolution to the BlockTemplate interface and updated the description.

DrahtBot · 2024-07-15T10:42:20Z

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

Hints

Make sure to run all tests locally, according to the documentation.

The failure may 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-07-15T10:43:22Z

Also added getBlockHeader() which the Template Provider needs for the NewTemplate message and a few other places.

Sjors · 2024-07-15T12:16:43Z

Also added getCoinbaseTx() and getWitnessCommitmentIndex() which are needed for the NewTemplate message.

Since this PR now contains Stratum v2 specific stuff anyway, I pulled in #30441.

I incorporating all this into #29432, so hopefully I didn't miss anything.

TheBlueMatt · 2024-07-15T19:30:58Z

This PR returns a BlockTemplate interface which causes the node to hold on to the template and its block.
The goal is to be able to quickly send NewTemplate and after that to copy all transactions to the external application so it can respond to RequestTransactionData and SubmitSolution.
I assume your concern is that this would make SubmitSolution too slow because the external application needs to pass the whole block back via IPC?

Ah, okay, that was my only real concern. Sending transaction data with the nonce solution requires Bitcoin Core do a metric shitload of allocations and takes ~forever. My only concern was making sure Bitcoin Core held on to the CTransaction share_ptrs for the transaction data that needs to be put in the CBlock.

ryanofsky

Light code review ACK 408c11e9bced08c51a7645a2de2c39c18e4360d9. This looks good, but I didn't review the getCoinbaseMerklePath implementation closely to check that it is computing the right thing.

src/rpc/mining.cpp

src/node/interfaces.cpp

Sjors · 2024-07-16T09:37:45Z

Rebased on the changes to #30356 and extracted GetCoinBaseMerklePath (that'll have to be its own PR though).

ryanofsky

Code review ACK 1e8f33ec15bc468385b2807e42d60c841c407681

src/consensus/merkle.cpp

src/node/interfaces.cpp

Sjors · 2024-08-26T15:26:11Z

@ryanofsky I used the incantation suggested here: https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/README.md#clang-format-diffpy

I assumed this would only edit lines impacted by the commit. But I see it touched a few other places in rpc/mining.cpp. I removed unrelated changes from the commit.

ryanofsky

Code review ACK 23374a22f442939802c0296d6447e0db02d513da. Just reined in clang-tidy since last review so there are fewer changes

Add interface changes from bitcoin#30409 bitcoin#30440 This commit changes the C++ Mining interface without changing the corresponding Capn'Proto interface. The capnp interface is updated the next commit.

Add interface changes from bitcoin#30409 bitcoin#30440 This commit updates the Cap'n Proto Mining interface after updating the C++ Mining interface last commit.

Add interface changes from bitcoin#30409 bitcoin#30440 This commit changes the C++ Mining interface without changing the corresponding Capn'Proto interface. The capnp interface is updated the next commit.

Add interface changes from bitcoin#30409 bitcoin#30356 bitcoin#30440 bitcoin#30443 This commit updates the Cap'n Proto Mining interface after updating the C++ Mining interface last commit.

itornaza

Concept ACK 23374a22f442939802c0296d6447e0db02d513da

Do you consider updating for cmake? It would be helpful for me to build and run tests locally.

An external program that uses the Mining interface may need quick access to some information in the block template, while it can wait a bit longer for the full raw transaction data. This would be the case for a Stratum v2 Template Provider which needs to send a NewTemplate message (which doesn't include transactions) as quickly as possible.

Sjors · 2024-09-13T08:17:35Z

Rebased for cmake (no changes).

ryanofsky

Code review ACK a93c171. Since last review, just rebased with no changes or conflicts

itornaza

Code review ACK a93c171

Offering an option for the Template Provider to get block header data as soon as possible is a great addition.

Code changes look straight forward. Also, built the pr and successfully run all tests on it locally.

sedited

Re-ACK a93c171

achow101 · 2024-09-16T19:21:58Z

ACK a93c171

It seems like BlockTemplate is mostly just a wrapper for CBlockTemplate. Presumably there was some discussion about having mining things in a separate process which makes having an interface for it necessary for IPC.

Sjors · 2024-09-16T19:25:47Z

@achow101 see #30510

Add interface changes from bitcoin#30409 bitcoin#30440 This commit changes the C++ Mining interface without changing the corresponding Capn'Proto interface. The capnp interface is updated the next commit.

Add interface changes from bitcoin#30409 bitcoin#30440 This commit updates the Cap'n Proto Mining interface after updating the C++ Mining interface last commit.

Add interface changes from bitcoin#30409 bitcoin#30440 This commit changes the C++ Mining interface without changing the corresponding Capn'Proto interface. The capnp interface is updated the next commit.

Add interface changes from bitcoin#30409 bitcoin#30440 This commit updates the Cap'n Proto Mining interface after updating the C++ Mining interface last commit.

DrahtBot changed the title ~~Have createNewBlock() return a BlockTemplate interface~~ Have createNewBlock() return a BlockTemplate interface Jul 12, 2024

Sjors mentioned this pull request Jul 12, 2024

ipc: add bitcoin-mine test program #30437

Open

Sjors force-pushed the 2024/07/newblock-iface branch from b50f75d to 50afc30 Compare July 12, 2024 13:59

DrahtBot added the CI failed label Jul 12, 2024

Sjors force-pushed the 2024/07/newblock-iface branch from 50afc30 to b13eeda Compare July 12, 2024 14:10

Sjors mentioned this pull request Jul 12, 2024

[WIP] Add getCoinbaseMerklePath() to Mining interface #30441

Closed

DrahtBot removed the CI failed label Jul 12, 2024

This was referenced Jul 13, 2024

Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block #30409

Merged

Stratum v2 Template Provider (take 3) #29432

Closed

getblocktemplate improvements for segwit and sigops #27433

Closed

Sjors force-pushed the 2024/07/newblock-iface branch 3 times, most recently from e24c853 to b7b9ad8 Compare July 15, 2024 10:42

DrahtBot added the CI failed label Jul 15, 2024

Sjors force-pushed the 2024/07/newblock-iface branch from b7b9ad8 to 3f2031c Compare July 15, 2024 12:16

Sjors force-pushed the 2024/07/newblock-iface branch from 3f2031c to 408c11e Compare July 15, 2024 13:32

DrahtBot removed the CI failed label Jul 15, 2024

ryanofsky approved these changes Jul 15, 2024

View reviewed changes

src/rpc/mining.cpp Outdated Show resolved Hide resolved

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

DrahtBot mentioned this pull request Jul 15, 2024

Introduce waitFeesChanged() mining interface #30443

Closed

1 task

Sjors force-pushed the 2024/07/newblock-iface branch from 408c11e to 1e8f33e Compare July 16, 2024 09:36

ryanofsky approved these changes Jul 16, 2024

View reviewed changes

src/consensus/merkle.cpp Outdated Show resolved Hide resolved

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

Sjors force-pushed the 2024/07/newblock-iface branch from 33f5ab3 to 23374a2 Compare August 26, 2024 15:26

ryanofsky approved these changes Aug 26, 2024

View reviewed changes

itornaza approved these changes Sep 12, 2024

View reviewed changes

Sjors added 2 commits September 13, 2024 10:14

Drop unneeded nullptr check from CreateNewBlock()

a93c171

Sjors force-pushed the 2024/07/newblock-iface branch from 23374a2 to a93c171 Compare September 13, 2024 08:17

ryanofsky approved these changes Sep 13, 2024

View reviewed changes

DrahtBot requested a review from itornaza September 13, 2024 14:25

itornaza approved these changes Sep 13, 2024

View reviewed changes

sedited approved these changes Sep 16, 2024

View reviewed changes

achow101 merged commit 3f66642 into bitcoin:master Sep 16, 2024

Sjors mentioned this pull request Sep 24, 2024

Mining interface: getCoinbaseMerklePath() and submitSolution() #30955

Merged

ryanofsky mentioned this pull request Oct 10, 2024

Introduce Mining interface #30200

Merged

Conversation

Sjors commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

DrahtBot commented Jul 12, 2024

Uh oh!

Sjors commented Jul 15, 2024

Uh oh!

Sjors commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 15, 2024

Uh oh!

Sjors commented Jul 15, 2024

Uh oh!

Sjors commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Jul 15, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Sjors commented Jul 16, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Sjors commented Aug 26, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

itornaza left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Sep 13, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

itornaza left a comment

Choose a reason for hiding this comment

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Sep 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Sjors commented Jul 12, 2024 •

edited

Loading

DrahtBot commented Jul 12, 2024 •

edited

Loading

Sjors commented Jul 15, 2024 •

edited

Loading

Sjors commented Jul 15, 2024 •

edited

Loading

achow101 commented Sep 16, 2024 •

edited

Loading