[WIP] add a stratum v2 template provider #27854
Conversation
These messages are used in the Template Distribution subprotocol in Stratum V2 and will be used by a template provider server.
The Sv2TemplateProvider class will act as a Template Distribution server for any stratum v2 connections.
Enables the sv2_template_provider if the --enable-template-proviver config is provided on compliation config and start the template provider on a dedicated thread.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
|
What are the reasons for not having this as a separate piece of software that calls the Bitcoin Core RPCs? From glancing at the code here it seems like If stratumv3 or "some other cool new mining protocol" comes out tomorrow, are we gonna add support for that too? In my opinion, Bitcoin Core's interfaces should accommodate projects like this but direct integration should be reserved for strictly necessary things. |
|
One reason is that |
I'm imagining a separate process
I was just trying to make a point about Bitcoin Core probably not wanting to support every mining protocol that will ever exist. |
having it implemented into bitcoind is not a big change and will allow more flexibility when templates are pushed respect to just poll
Sure, and I agree. What I want to say is that in the case of an improvement of Sv2 that needs new/different messages to be supported by bitcoind these messages will use the same data format and framing' proposed here. So there wont be the need to support a completely new protocol. ' please note that the framing implemented if this PR is not the one on the spec so when I say the one proposed here I mean the one that is specified in the Sv2 spec. The framing will be updated if there will be sufficient support for an concept ACK |
What are you thinking of in terms of flexibility that the current interfaces do not offer? I can't think of anything that would not work by using the current interfaces or exposing extra functionality through them. For example, looking at Matt's comment #23049 (comment):
This is not something we currently support and would (afaik) require significant changes to our mining/mempool logic (entirely separate of any stratum v2 changes). Once we have that functionality we should expose it through our interfaces (e.g. zmq, rpc, rest) such that applications like stratum v2 can make use of it.
Correct me if I'm wrong but the inefficiencies of |
|
Can not answer about Matt comment. (@TheBlueMatt maybe can clarify?) What I intended is for example create a new template as soon as txs in mempool allow creations of new template with a fee amount that is bigger than x than the one of last created template. Or create a future template based on the one that miner is mining (next-next-block).
It make sense to me. But I would like to know also Matt opinion on that. |
|
I'm Concept ACK on eventually replacing the current
None of these deficiencies require the full StratumV2 approach to address; e.g. longpolling RPC could partially address the polling concern, a binary REST or ZMQ interface instead of RPC could address encoding issues, and additional RPC options to reduce verbosity could address that concern. Still, I don't think there was much impetus to improve upon these in an ecosystem that is If (and that is an if) it is clear that the ecosystem is likely to adopt StratumV2, then I believe it makes sense to consider it as an interface too now. An alternative could still be a more "neutral" interface that is also push-based, binary, and efficient, and keep the StratumV2 logic as a separate daemon. That has advantages and disadvantages:
I haven't reviewed StratumV2 itself, so I'm curious how people see these. Perhaps one can also consider different trade-offs in between "
It's true that we don't current support this, and that Stratum V2 does not magically solve this, but I do believe that part of the reason we don't is exactly because it's not that trivial in an RPC-based setting, and hard to move away from that in an ecosystem that's reliant on |
Given that Sv2 logic for TP is already minimal. The option of not using an already existent interface (zmq,rpc) neiter using the already specified Sv2 interface but redesign from scratch a new one, do not make much sense to me.
Also this make sense to me but I still would like to hear the opinion from someone else that have insight into Sv2.
yep |
This protocol basically is a protocol designed exactly for this use-case. The fact that it comes under a "StratumV2" banner is somewhat unrelated, there's only some common framing. It's also not a substantial protocol, arguably simpler than getblocktemplate in full. This protocol exists basically to create an alternative to getblocktemplate which solves key problems in it, and can be used with or without any other Stratumv2 content.
This protocol is certainly not set in stone, if someone has ideas for other goals that should be addressed or other protocol tweaks that can still happen at this phase, though ideally not a complete rewrite :). |
| @@ -344,6 +344,17 @@ AC_ARG_ENABLE([lto], | |||
| [enable_lto=$enableval], | |||
| [enable_lto=no]) | |||
|
|
|||
| dnl Enable running a StratumV2 Template Provider server. | |||
There was a problem hiding this comment.
Why bother making this optional? Can just have a config flag.
There was a problem hiding this comment.
I reasoned since that this proposal wasn't introducing/editing arguably a core feature that it should be optional. Also setting it by default, I think clashes with how the functional tests currently run in Bitcoin since the sv2 server will respond to building a new template on best block change and will affect assertions on the state of the mempool in functional tests.
There was a problem hiding this comment.
We can turn it on with a non-default config knob, though, it doesn't have to be non-built. I'm not sure I understand why it should be compile-time optional - it doesn't add a new dependency, doesn't invasive touch other code, and is completely unused if the config knob isn't turned on.
| /** | ||
| * The default option for the additional space required for coinbase output. | ||
| */ | ||
| unsigned int default_coinbase_tx_additional_output_size = 0; |
There was a problem hiding this comment.
Shouldnt this be non-0? 1KB would match the current GBT logic. More generally, there doesn't seem to be a way to set this, somehow I'd thought it was in the exposed protocol, but if not it probably should be.
| /** | ||
| * The default flag for all new work. | ||
| */ | ||
| bool default_future_templates = true; |
There was a problem hiding this comment.
Why is this a config flag? The message future_template bit indicates that its a predicted template, and presumably we'll have a stream of both predicted and current templates on the wire, not something in the options?
There was a problem hiding this comment.
This is true, I treated this PR as draft. Currently in development and testing against the SRI, all templates are assumed to be future so this would be updated if this PR gains interest for a complete implementation.
| * The current protocol version of stratum v2 supported by the server. Not to be confused | ||
| * with byte value of identitying the stratum v2 subprotocol. | ||
| */ | ||
| uint16_t protocol_version = 2; |
There was a problem hiding this comment.
Is there a reason not to hard-code this and optional_features until we actually have a need for them? Just less code if we do.
There was a problem hiding this comment.
True, we can hard code it when these fields need to be serialized and add a comment above explaining why they are currently hard coded
There was a problem hiding this comment.
Code readability is more important than "less code"...
Thanks and appreciate the time for reviewing the PR. A lot of the points are valid especially concerns around maintenance burden and supporting numerous project integrations. In terms of RPC calls, this is of course possible. With stratum v2 some of the goals are around improving performance and reducing latency. JSON RPC can fall short in speed and efficiency. If the sv2 TP were to be run as a separate process that uses the existing RPC calls, then at a very high level we have the overhead:
where as if sv2 provided templates:
There are different approaches we could take, none of which I think has necessarily strong support for a particular direction but I guess the common goal would be, to be able to extract templates and submit work more efficiently with less overhead than the current RPCs available |
Its not just latency and overhead, however - one of the important goals of replacing GBT in Bitcoin Core broadly is to move towards everything being push, rather than polling. In general, block templates should come out of Bitcoin Core very aggressively - when a new block comes in and has been validated, before the mempool gets updated, we should be able to get a new block template out based on some prediction and conflict-removal, from there, we can update the mempool and then do a full new block generation, updating the template again. During steady-state, we should make decisions about when to update the block template based on new transactions (ignore 30-second-old transactions unless they have lots of fees, do a block template update immediately if a huge-fee transaction comes in, etc). This is important for long-term sustainability of Bitcoin Core as a block template constructor, and while GBT technically supports polling, I don't think any major production pool servers actually use it - it being optional means you can't make decisions like most of the above without assuming everyone uses GBT long-polling, which just isn't true. |
|
Over-turning my comment from #23049 (comment). After studying more Stratum V2 and the inner details of mining operations, and in light of more “reactive" block template construction in future in function of the processed mempool content, I think this more valuable to have Stratum V2 TP integrated in Bitcoin Core to enhance decentralization of the mining ecosystem. From a maintenance viewpoint, the changes are very “peripheral” from other subsystem like the validation engine or the p2p stack so concept ACK to have Stratum V2 TP in Core.
In the future, it might make sense to split the |
|
Concept ACK as an alternative to getblocktemplate. I’m still digging into the deeper bits of StratumV2 though. :) |
|
I've been playing around with Stratum v2 in recent months and may get my hands on an old S9. So hopefully I can test this when electricity prices go negative, soon(tm). |
|
Concept ACK.
I even have S9 and could be ok to pay that electricity bill for some tests, but currently it's already too hot in my flat (I use it as a space heater from time to time, when it's too cold). |
|
Concept ACK |
|
@kristapsk I removed two of the three hashboards and set the target consumption to 100W. In practice it's more like 130, but that's fine; not hot and not too noisy (mine came with BrainsOS+ installed) |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
There was a problem hiding this comment.
The PR description could use instructions on how to test this. Specifically I'd like to try it on mainnet against my vintage S9 with BraiinsOS+ (not sure which pools already support job negotiation/ announcement). This can of course point to existing documentation. Config D is close but it assumes regtest and running your own pool.
It will also be easier to test and review this pull request if you add some functional tests. Maybe also add a -debug=sv2 log category so it's easier to see what's happening.
There could be potential conflict with functional tests. [...] My guess is that it would be acceptable to disable the Sv2 TP when running functional tests [...]
I think it's better to fix / improve these tests. Depending on the exact issue that could be done in a separate PR.
Imo this does not need to go behind a --enable-template-provider flag. Having -stratumv2 off by default is enough. You're not adding a new dependency nor creating a new binary. Covering all possible configure flags in CI scales exponentially. Of course this ties into my previous point about tests.
I'm seeing CreateNewBlock() messages now even when running without -stratumv2. That should probably be changed.
When running with -stratumv2 it says Sv2 Template Provider listening on port: 0 so something is off with the default argument parsing there.
| @@ -630,6 +638,10 @@ void SetupServerArgs(ArgsManager& argsman) | |||
| #if defined(USE_SYSCALL_SANDBOX) | |||
| argsman.AddArg("-sandbox=<mode>", "Use the experimental syscall sandbox in the specified mode (-sandbox=log-and-abort or -sandbox=abort). Allow only expected syscalls to be used by bitcoind. Note that this is an experimental new feature that may cause bitcoind to exit or crash unexpectedly: use with caution. In the \"log-and-abort\" mode the invocation of an unexpected syscall results in a debug handler being invoked which will log the incident and terminate the program (without executing the unexpected syscall). In the \"abort\" mode the invocation of an unexpected syscall results in the entire process being killed immediately by the kernel without executing the unexpected syscall.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); | |||
| #endif // USE_SYSCALL_SANDBOX | |||
| // | |||
| #if ENABLE_TEMPLATE_PROVIDER | |||
| argsman.AddArg("-stratumv2=<port>", "Listen for stratumv2 connections on <port>. Bitcoind will act as a Template Provider. (default: 8442)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); | |||
There was a problem hiding this comment.
This should also mention the testnet, regtest and signet ports.
|
Hi @Sjors , thanks and appreciate the review,
For this, honestly I'm not sure which would be the best configuration, I'll need to ask a few people that have done mainnet testing/testing with devices. I'll update the PR description once I find out.
You're right, I'll work on setting up some functional tests and also adding the debug log category.
This makes more sense. I implemented this change removing the compile configuration flag and just used I think since there's a lot of changes in this one PR and noise to come, it would probably make sense to break this down into much smaller focused PRs? I think it would be much more beneficial for reviewers to consume and review smaller chunks of code. |
| @@ -630,6 +638,10 @@ void SetupServerArgs(ArgsManager& argsman) | |||
| #if defined(USE_SYSCALL_SANDBOX) | |||
| argsman.AddArg("-sandbox=<mode>", "Use the experimental syscall sandbox in the specified mode (-sandbox=log-and-abort or -sandbox=abort). Allow only expected syscalls to be used by bitcoind. Note that this is an experimental new feature that may cause bitcoind to exit or crash unexpectedly: use with caution. In the \"log-and-abort\" mode the invocation of an unexpected syscall results in a debug handler being invoked which will log the incident and terminate the program (without executing the unexpected syscall). In the \"abort\" mode the invocation of an unexpected syscall results in the entire process being killed immediately by the kernel without executing the unexpected syscall.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); | |||
| #endif // USE_SYSCALL_SANDBOX | |||
| // | |||
| #if ENABLE_TEMPLATE_PROVIDER | |||
| argsman.AddArg("-stratumv2=<port>", "Listen for stratumv2 connections on <port>. Bitcoind will act as a Template Provider. (default: 8442)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); | |||
There was a problem hiding this comment.
Probably should be called -stratumv2port, unless it supports specifying an IP to bind to (probably a good idea)
There was a problem hiding this comment.
Thank you for the suggestion. I'll update the argument name and also add an IP binding option.
| @@ -630,6 +638,10 @@ void SetupServerArgs(ArgsManager& argsman) | |||
| #if defined(USE_SYSCALL_SANDBOX) | |||
| argsman.AddArg("-sandbox=<mode>", "Use the experimental syscall sandbox in the specified mode (-sandbox=log-and-abort or -sandbox=abort). Allow only expected syscalls to be used by bitcoind. Note that this is an experimental new feature that may cause bitcoind to exit or crash unexpectedly: use with caution. In the \"log-and-abort\" mode the invocation of an unexpected syscall results in a debug handler being invoked which will log the incident and terminate the program (without executing the unexpected syscall). In the \"abort\" mode the invocation of an unexpected syscall results in the entire process being killed immediately by the kernel without executing the unexpected syscall.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); | |||
| #endif // USE_SYSCALL_SANDBOX | |||
| // | |||
| #if ENABLE_TEMPLATE_PROVIDER | |||
| argsman.AddArg("-stratumv2=<port>", "Listen for stratumv2 connections on <port>. Bitcoind will act as a Template Provider. (default: 8442)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); | |||
There was a problem hiding this comment.
We don't typically refer to "Bitcoind" like this
There was a problem hiding this comment.
Ok thanks, I'll remove this reference in the description.
| node.sv2_template_provider = std::make_unique<Sv2TemplateProvider>(*node.chainman, *node.mempool); | ||
|
|
||
| try { | ||
| auto sv2_port{static_cast<uint16_t>(gArgs.GetIntArg("-stratumv2", 8442))}; |
There was a problem hiding this comment.
Should be added to the port number check earlier
| auto sv2_port{static_cast<uint16_t>(gArgs.GetIntArg("-stratumv2", 8442))}; | ||
| node.sv2_template_provider->Start(Sv2TemplateProviderOptions { .port = sv2_port }); | ||
| } catch (const std::runtime_error& e) { | ||
| LogPrintf("sv2: %s\n", e.what()); |
There was a problem hiding this comment.
Rather than prefixing with "sv2:", suggest a new log category.
There was a problem hiding this comment.
Ok thanks, I'll add a new log category.
| * The current protocol version of stratum v2 supported by the server. Not to be confused | ||
| * with byte value of identitying the stratum v2 subprotocol. | ||
| */ | ||
| uint16_t protocol_version = 2; |
There was a problem hiding this comment.
Code readability is more important than "less code"...
|
Hi everyone, Apologies for the absence. I've been busy working on related branches to address some of the reviews here and to keep up with the changes in the stratumv2 spec. It seems like both the protocol and the template provider work/spec have stabilized. I believe the focus from this point onward will be on intensive testing and refactoring the code to align more closely with the Bitcoin project. I'll likely need to reassess the best way to present these changes, possibly in smaller, more focused PRs to make it easier for contributors to review. |
|
Hi @ccdle12. Stratum v2 is one of the proposed high-priority projects in #28642 and it received several votes. It may be helpful if you can comment there whether the next six months would be favorable timing for the Stratum v2 work, if you (or someone else) is willing to be an active lead, and if that person would be able to attend the weekly |
|
I was able to mine a bit on testnet today using:
I have not tried the censorship solo mining fallback yet (there's no easy way to trigger that without a test pool that censors). It would be great to see this PR updated with the latest stuff, and maybe also rebased. In addition it would be useful to have an up to date overview of what parts of the Stratum v2 spec are implemented in what part of the code here. I wonder if we can keep the first iteration of this project poll based, e.g. the above linked branch just creates a template every 30 seconds and the Job Declarator client occasionally comes by and asks for it. The main changes to focus on would then be a new communication protocol and some new messages. Then a later pull request could figure out how to make it all push based. Since the existing Alternatively we completely figure out how to get everything right before merging anything. |
|
I have a rebased and reorganised branch: master...Sjors:bitcoin:2023/11/sv2-poll It's based on @Fi3's master...Fi3:bitcoin:PatchTemplates which IIUC is the most recent thing people are testing with (see Discord chat). That branch starts where this PR ends and has several newer commits by @ccdle12 and @Fi3. @ccdle12 are you still working on this? If not, I could take over and open a new PR. Otherwise, you could check if my branch is useful and use it here. |
|
Happy to leave this open for a bit, but here's my PR: #28983 |
|
I believe I've processed all feedback on this PR into my PR, either as documentation, TODO or actually implemented. |
|
Closing for now, due to lack of addressing feedback and questions. Please leave a comment if this should be re-opened. In the meantime, it seems better to focus on #28983 |
Description
This is a WIP/draft pull request seeking concept/approach ACK. The goal is to initiate discussion and gather feedback on the proposed changes to see if there is sufficient support for an ACK before finalizing an implementation.
This PR initially proposes the addition of a new thread and server to enable Bitcoin Core to run as a Sv2 TP (Stratum V2 Template Provider). This approach is the most direct and naive approach in regards to integrating into the Bitcoin architecture. It builds on #23049 but no longer uses the Rust library and implements the server and message de/serialization in C++.
This PR branch has been used for testing with the SRI (Stratum V2 Reference Implementation), but it has been limited to regtest and some tests on testnet. This branch is considered far from complete.
Context
Stratum V2 is a proposed evolution of the stratum mining protocol, it has several benefits over its predecessor:
By default, it uses secure communication, which can protect against man-in-the-middle-attacks, e.g. hashrate hijacking
It uses a binary protocol instead of JSON, optimizing data transfer between all parties
It allows miners/downstream connections to propose their own transaction sets to mining pools
It can reduce occurrence of empty mined blocks
Links below for further reference:
Motivation
The motivation for this PR and potential future work is to allow Bitcoin Core to run as a Sv2 TP. This will enable downstream Sv2 connections to be able to connect and receive/submit valid work over the Sv2 protocol.
Below is an outline of a few pros/cons regarding the Sv2 TP in Bitcoin Core.
Pros:
Running Bitcoin Core with a config flag to enable the Sv2 TP could be arguably easier for adoption. A user would only need to be able to compile and run Bitcoin Core and its dependencies to be able to extract valid work and broadcast legitimate work over the Sv2 protocol
The Sv2 TP being as close as possible to the mempool and p2p network benefits downstream connections so that they can start new work quickly. The time between a node learning of a new best block and then sending new work downstream should be as minimal as possible
Implementing the Sv2 TP in C++ means it should be (hopefully) easier for a wider range of contributors/reviewers to be able to maintain and build. The Sv2 TP only uses a small subset of messages from the entire Sv2 Rust library. Therefore, we realized that re-implementing a subset of the messages in C++ and implementing a server carries less external dependency risk and less architectural complexity
No additional external dependencies are required
Cons:
The Sv2 TP in core will need to be kept up to date with changes in the SRI, coordination would be required to prevent any potential versioning issues
Adds another thread/sub-module that will need to access the mempool to build candidate blocks, so there would be another area of the code attempting to acquire
cs_mainviaBlockAssemblerAnother architectural approach other than a new thread and server for the Sv2 TP might be preferred such as the current work in the multiprocess project
Build
To run the Sv2 TP on this branch, Bitcoin needs to be configured with
./configure --enable-template-provider.Future work
If there is sufficient approach/concept ACK or support in general. Then the below points are outstanding future work and existing nits.
Future work:
A noise implementation from the noise protocol framework needs to be introduced for secure communication between the Sv2 TP and downstream connections. The noise spec for Sv2 has been reworked to accommodate resources currently available in core e.g.
secp256k1andBIP 340 schnorrsigThere are current working changes proposed to the Sv2 TP spec, to allow downstream connections to send shorttxid sets, allowing miners/downstream connections to present their own tx sets
Perhaps the Sv2 TP should be adapted to current work in the Bitcoin Core architecture. The new thread has been helpful for testing against the current SRI stack, it seems like the server implementation can adapt easily to different architectural needs
Existing/known nits:
The server implementation has no limit on the number of downstream connections, it needs to be added
There could be potential conflict with functional tests. The Sv2 TP responds to the best new block change by reaching into the mempool and building a new block for downstream. My guess is that it would be acceptable to disable the Sv2 TP when running functional tests that asserts mempool state and then enable the Sv2 TP when it needs to specifically run its functional tests
Getting the full merkle path is required in the NewTemplate message. I noticed that this was previously possible with the old implementation that was moved to merkle_test.cpp , the current implementation does an in-place calculation to just return the root. I was wondering if anyone has any suggestions or insight to a preferred solution? e.g. Move back the old functions from merkle_tests or write a new implementation?
Thank you to all who helped contribute to the process of building this branch including working on the SRI, using this branch for testing and refining the Sv2 specs.
Any feedback or reviews from nits to overall approach would be very much appreciated.