Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stratum v2 Template Provider via IPC interface (multiprocess) #48

Open
wants to merge 18 commits into
base: 2024/07/sv2-tp-common
Choose a base branch
from

Conversation

Sjors
Copy link
Owner

@Sjors Sjors commented Jul 17, 2024

Based on #49 and bitcoin#30437. This is an alternative approach to bitcoin#29432. Rather than integrating the Template Provider directly into Bitcoin Core, we make it an entirely separate application that communicates via IPC, see design/multiprocess.md.

Usage

Compile as usual, multiprocess is enabled by default.

Alternatively get a recent binary, see (unofficial, by me) releases ending in "ICP".

Start node:

build/src/bitcoin-node -chain=testnet4 -ipcbind=unix

Start the Template Provider:

build/src/bitcoin-mine -chain=testnet4 -debug=sv2 -loglevel=sv2:trace

See general Stratum v2 usage instructions in bitcoin#29432.

Rationale, PRs this builds on

There are multiple ways to create such an application. It could be built from scratch in Rust, using bits and pieces of the Stratum Reference Implementation SRI. It could be a c++ application built from scratch, perhaps using some libraries from Bitcoin Core.

But this PR goes about in a slightly different way. Similar to Elements or Knots it's a set of patches on top of the full Bitcoin Core codebase. This allows me to leverage the build system, subclass Transport and use various other useful bits.

This lets me reuse almost all code from the integrated Template Provider bitcoin#29432. Both build direct on:

This PR relies on the interfaces changes proposed in:

And the build changes proposed in:

These interface changes are also used by bitcoin#29432, but there they run in the same process.

I modified the guix script to only produce bitcoin-node (for now) and bitcoin-mine binaries.

Goal

The goal is to drop bitcoin-node from the release here. Ideally users would install Bitcoin Core in the manner they're familiar with. They would then install bitcoin-mine separately and it should just work(tm), at most having to add a line to bitcoin.conf to turn IPC on.

This won't work until we ship a release with multiprocess and the mining interface enabled, ideally in bitcoind and bitcoin-qt rather than in seperate bitcoin-node and bitcoin-gui binaries. See bitcoin#30437 for progress towards that.

It might eventually be feasible to complete separate the codebase. A first step towards that would be to turn libbitcoin_net into a library (see #47). However for the time being I'd rather rebase on Bitcoin Core, which I'm familiar with anyway, than maintain an entire (guix) build system myself.

@Sjors
Copy link
Owner Author

Sjors commented Jul 17, 2024

@ryanofsky I slightly mangled e373642 because of bitcoin@4e1a434#r1681193702. I'll clean that up later.

@Sjors
Copy link
Owner Author

Sjors commented Jul 17, 2024

Incorporated the changes from bitcoin#30437 (comment) and cleaned up commit history.

@Sjors
Copy link
Owner Author

Sjors commented Jul 18, 2024

@ryanofsky do you have a commit somewhere that adds libmultiprocess to the guix build?

@Sjors Sjors force-pushed the sv2-ipc branch 2 times, most recently from c75792b to 91720b7 Compare July 18, 2024 09:26
@Sjors
Copy link
Owner Author

Sjors commented Jul 18, 2024

Getting a cryptic error: '__NVCC___WORKAROUND_GUARD' is not defined error for the multiprocess job: https://cirrus-ci.com/task/4940413969104896?logs=ci#L2225

Somehow related to the Template Provider depending on boost/multi_index via its use of the mempool.

I can't make sense of the personality(old_personality | ADDR_NO_RANDOMIZE) error thrown by MSAN and TSAN, but it's possible that's related to my self-hosted CI setup and/or the modification in 3319668.

@ryanofsky
Copy link

re: #48 (comment)

@ryanofsky do you have a commit somewhere that adds libmultiprocess to the guix build?

I don't, since I haven't really experimented with the guix build. Maybe simplest approach to start with would enable the multiprocess build option and build and package all the multiprocess executables including bitcoin-tp and bitcoin-node. I know in the longer run you want to have separate releases as described in bitcoin#30437 (comment).

re: #48 (comment)

Getting a cryptic error: '__NVCC___WORKAROUND_GUARD' is not defined error for the multiprocess job: https://cirrus-ci.com/task/4940413969104896?logs=ci#L2225

Somehow related to the Template Provider depending on boost/multi_index via its use of the mempool.

This is a -Wundef error caused by bitcoin#29876 that happens when a boost header is included from a source file that is compiled with -I /path/to/boost/headers rather than -isystem /path/to/boost/headers. The fix for this is probably:

-bitcoin_mine_CPPFLAGS = $(bitcoin_bin_cppflags)
+bitcoin_mine_CPPFLAGS = $(bitcoin_bin_cppflags) $(BOOST_CPPFLAGS)

to add the right -isystem option.

I can't make sense of the personality(old_personality | ADDR_NO_RANDOMIZE) error thrown by MSAN and TSAN, but it's possible that's related to my self-hosted CI setup and/or the modification in 3319668.

Not sure about this error. Link seems to be https://cirrus-ci.com/task/4759864851824640?logs=ci#L2486

@Sjors Sjors force-pushed the 2024/07/sv2-tp-common branch 2 times, most recently from 19cc9f1 to dabc32c Compare July 19, 2024 08:59
@Sjors Sjors force-pushed the sv2-ipc branch 3 times, most recently from 6c24b45 to a8bb152 Compare July 19, 2024 13:55
@Sjors
Copy link
Owner Author

Sjors commented Jul 19, 2024

I'm getting the MSAN / TSAN error inconsistently on my self-hosted CI. Does not seem related to this PR. Trying to reproduce in isolation in #51.

@Sjors
Copy link
Owner Author

Sjors commented Oct 4, 2024

Rebased after bitcoin#30510.

Sjors and others added 14 commits October 4, 2024 20:16
This update brings in the following changes:
chaincodelabs/libmultiprocess#111: doc: Add internal design section
chaincodelabs/libmultiprocess#113 Add missing include to util.h
This update brings in the following changes:

chaincodelabs/libmultiprocess#116  shutdown bugfix: destroy RPC system before running cleanup callbacks
Allows InitError and InitWarning to be used by other executables beside
bitcoind and bitcoin-node.
See src/bitcoin-mine.cpp for usage information.

Co-authored-by: Sjors Provoost <[email protected]>
The Template Provider binary:
* won't have a wallet
* probably won't have a gui (maybe after the QT revamp)
* doesn't use the functional test framework

Disable some CI stuff to speed it up.

This is not a complete list, because:
1. That would be too hard to rebase
2. Some things might be added later, e.g. tracepoints
@Sjors
Copy link
Owner Author

Sjors commented Oct 4, 2024

Still a few things left to clean up, but this now produces a working bitcoin-mine for x86_64-darwin.

I pushed a commit to bitcoin#30975, because some of the CI failures don't seem related to sv2.

achow101 added a commit to bitcoin/bitcoin that referenced this pull request Oct 10, 2024
…ution()

525e9dc Add submitSolution to BlockTemplate interface (Sjors Provoost)
47b4875 Add getCoinbaseMerklePath() to Mining interface (Sjors Provoost)
63d6ad7 Move BlockMerkleBranch back to merkle.{h,cpp} (Sjors Provoost)

Pull request description:

  The new `BlockTemplate` interface introduced in #30440 allows for a more efficient way for a miner to submit the block solution. Instead of having the send the full block, it only needs to provide the nonce, timestamp, version fields and coinbase transaction.

  This PR introduces `submitSolution()` for that. It's currently unused.

  #29432 and Sjors#48 use it to process the Stratum v2 message [SubmitSolution](https:/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#77-submitsolution-client---server). The method should be sufficiently generic to work with alternative mining protocols (none exist that I'm aware off).

  This PR also introduces `getCoinbaseMerklePath()`, which is needed in Stratum v2 to construct the `merkle_path` field of the `NewTemplate` message (see [spec](https:/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#72-newtemplate-server---client)). The coinbase merkle path is also used in Stratum "v1", see e.g. https://bitcoin.stackexchange.com/questions/109820/questions-on-merkle-root-hashing-for-stratum-pools

  This last function uses `BlockMerkleBranch` which was moved to the test code in #13191. The reason back then for moving it was that it was no longer used. This PR moves it back.

  This PR does not change behaviour since both methods are unused.

ACKs for top commit:
  achow101:
    ACK 525e9dc
  itornaza:
    Code review ACK 525e9dc
  tdb3:
    Code review and light test ACK 525e9dc
  ryanofsky:
    Code review ACK 525e9dc. Left minor suggestions but none are important, and looks like this could be merged as-is

Tree-SHA512: 2a6a8f5d409ff4926643193cb67702240c7c687615414371e53383d2c13c485807f65e21e8ed98515b5456eca3d9fca13cec04675814a4081467d88b849c5653
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants