[WIP] add a stratum v2 template provider

ccdle12 commented at 8:54 am on June 11, 2023: contributor

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:

Stratum V2 specification: https://github.com/stratum-mining/sv2-spec
SRI (Stratum V2 Reference Implementation): https://github.com/stratum-mining/stratum
Stratum V2 overview: https://stratumprotocol.org/

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_main via BlockAssembler
Another 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. secp256k1 and BIP 340 schnorrsig
There 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.

sv2_messages: introduce sv2 messages used by a sv2 template provider

These messages are used in the Template Distribution subprotocol in
Stratum V2 and will be used by a template provider server.

ec26579437

test+fuzz: add sv2_messages unit tests and fuzz tests f2dddadf7c

configre+init: add sv2 template provider enable args and configuration ab79b36ed6

sv2_template: introduce sv2 template provider server

The Sv2TemplateProvider class will act as a Template Distribution server
for any stratum v2 connections.

f17d92e8d6

test+fuzz: add sv2_template_provider unit tests and fuzz tests e21140b429

context+init: add sv2_template_provider to node context and on init

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.

6cc815a474

functional: add sv2_port to functional test config 26d854ae79

sv2_messages: styling nits a25f0233b7

init: add graceful shutdown if sv2 start fails 2c33acc269

sv2_template: refactor to only bind listening port after exiting IBD ff02f58816

sv2_template: refactor and reduce lock contention on wait for best block change 21bb3946c4

DrahtBot commented at 8:54 am on June 11, 2023: contributor

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
Concept ACK	Fi3, sipa, ariard, dunxen, kristapsk, luke-jr

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:

#27896 (Remove the syscall sandbox by fanquake)
#26966 (index: blockfilter initial sync speedup, parallelize process by furszy)
#24539 (Add a “tx output spender” index by sstone)

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.

dergoegge commented at 9:53 am on June 12, 2023: member

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 getblocktemplate and submitblock would be the only needed RPCs. I’ve seen the discussion about separation (multi-process) in #23049 but wasn’t convinced by the arguments there (e.g. #23049 (comment)). I am worried about the maintenance burden of this and would prefer it being a separate thing.

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.

Fi3 commented at 11:01 am on June 12, 2023: none

One reason is that getblocktemplate require polling bitcoind where NewTemplate will be pushed by bitcoind. What you call stratumV3 would likely be an extension of Sv2 and will likley use the same binary data format and framing of Sv2.

dergoegge commented at 11:12 am on June 12, 2023: member

One reason is that getblocktemplate require polling bitcoind where NewTemplate will be pushed by bitcoind.

I’m imagining a separate process stratumv2d that implements the server from this PR but polls the block template through getblocktemplate instead of using the BlockAssembler directly (which is equivalent to a poll, minus the rpc roundtrip). stratumv2d could then still push template updates after polling from bitcoind.

What you call stratumV3 would likely be an extension of Sv2 and will likley use the same binary data format and framing of Sv2.

I was just trying to make a point about Bitcoin Core probably not wanting to support every mining protocol that will ever exist.

Fi3 commented at 11:35 am on June 12, 2023: none

I’m imagining a separate process stratumv2d that implements the server from this PR but polls the block template through getblocktemplate instead of using the BlockAssembler directly (which is equivalent to a poll, minus the rpc roundtrip). stratumv2d could then still push template updates after polling from bitcoind.

having it implemented into bitcoind is not a big change and will allow more flexibility when templates are pushed respect to just poll getblocktemplate

I was just trying to make a point about Bitcoin Core probably not wanting to support every mining protocol that will ever exist.

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

dergoegge commented at 1:08 pm on June 12, 2023: member

having it implemented into bitcoind is not a big change and will allow more flexibility in when templates are pushed respect to just poll getblocktemplate

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):

do things like pre-construct next-next-block templates in advance and push them out quicker

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.

the inefficiencies of getblocktemplate have been a real source of pain for pool operators for some time, I’m excited its going away.

Correct me if I’m wrong but the inefficiencies of getblocktemplate (for the most part) stem from the underlying mining algorithm’s runtime complexity. This is not something that stratum v2 magically solves.

Fi3 commented at 1:28 pm on June 12, 2023: none

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

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.

It make sense to me. But I would like to know also Matt opinion on that.

sipa commented at 2:40 pm on June 12, 2023: member

I’m Concept ACK on eventually replacing the current getblocktemplate with another interface, for the following reasons:

The poll-based nature introduces unnecessary latency over a hypothetical push-based one.
Being RPC based, getblocktemplate is bandwidth and CPU inefficient due to JSON and hex encoding (for large amounts of data being transferred on every call).
getblocktemplate provides too much information for clients that do not care about performing their own transaction selection (full transaction hex and dependency information), with no way to opt out.

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 getblocktemplate-based.

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:

Advantages:
- Less complexity in Bitcoin Core to review and maintain.
- Less tied to the specific protocol, so wouldn’t need Bitcoin Core changes when further protocol changes occur (as long as those don’t introduce additional data requirements not exposed by the interface), perhaps making it easier to evolve StratumV2 (and/or successors).
Disadvantages:
- Slight efficiency/latency loss due to needing an additional process/layer to do the translation.
- Additional deployment friction (I do think it’s important in general to make sure mining is as easy and efficient as possible, and having easily usable interfaces helps with that).
- In case a feature like outsourcing transaction selection is considered, I can imagine having one party performing the coinbase construction (the pool), and another performing transaction selection (the bitcoind talked to). In case this involves communicating e.g. the size/sigops of that coinbase to bitcoind for block construction, that may be harder to incorporate in a “neutral” protocol (it’d necessarily be bidirectional, while REST/ZMQ are inherently uni-directional I believe).

I haven’t reviewed StratumV2 itself, so I’m curious how people see these. Perhaps one can also consider different trade-offs in between “bitcoind speaks the mining protocol directly” and “bitcoind only speaks a neutral interface”:

I assume what is considered in this PR is already only a subset of StratumV2 (as I remember there are multiple subprotocols, and I imagine not all of them are relevant to Bitcoin Core). But seeing things like Noise being part of the protocol, I do wonder if it’s a possibility to just consider an unencrypted/unauthenticated subset, requiring a (perhaps much thinner) separate daemon to add that. Doing so may perhaps reduce the complexity to maintain in Bitcoin Core significantly.
Perhaps we see what is considered here as StratumV2 not so much as “the” mining protocol we expect everything to use, but just as the de facto binary interface protocol chosen because we need one anyway, and StratumV2 is already designed to cover the needs of such a protocol. If StratumV2 expansions/evolutions come along, or whatever other protocol, it may not be unreasonable to expect that to need a separate daemon that speaks StratumV2 with Bitcoin Core still.

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.

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 getblocktemplate. And in the long term, I do believe it’s reasonable to consider at least the possibility of doing block template construction asynchronously in Bitcoin Core, because it is the one piece of infrastructure that has all the information to do so, and moving to such an approach enables for more computationally heavy block building without burdening the critical path.

Fi3 commented at 3:17 pm on June 12, 2023: none

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.

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.

Advantages:
    Less complexity in Bitcoin Core to review and maintain.
    Less tied to the specific protocol, so wouldn't need Bitcoin Core changes when further protocol changes occur (as long as those don't introduce additional data requirements not exposed by the interface), perhaps making it easier to evolve StratumV2 (and/or successors).
Disadvantages:
    Slight efficiency/latency loss due to needing an additional process/layer to do the translation.
    Additional deployment friction (I do think it's important in general to make sure mining is as easy and efficient as possible, and having easily usable interfaces helps with that).
    In case a feature like outsourcing transaction selection is considered, I can imagine having one party performing the coinbase construction (the pool), and another performing transaction selection (the bitcoind talked to). In case this involves communicating e.g. the size/sigops of that coinbase to bitcoind for block construction, that may be harder to incorporate in a "neutral" protocol.

Less complexity on Bitcoin Core: not sure how less complex we can be, maybe a little bit, but this with the need to introduce a new interface with his own spec.
Slight efficiency/latency loss: I don’t think that this could ever be a real world issue but the point is right.
Additional deployment friction: this is one of the main reason for why we want to include noise in the TP and not doing it as a separate layer.
About the last point the pool/miner communicate how much space at maximum he need for the coinbase outputs and the TP create a template considering it.

I assume what is considered in this PR is already only a subset of StratumV2 (as I remember there are multiple subprotocols, and I imagine not all of them are relevant to Bitcoin Core). But seeing things like Noise being part of the protocol, I do wonder if it’s a possibility to just consider an unencrypted/unauthenticated subset, requiring a (perhaps much thinner) separate daemon to add that. Doing so may perhaps reduce the complexity to maintain in Bitcoin Core significantly.

Also this make sense to me but I still would like to hear the opinion from someone else that have insight into Sv2.

Perhaps we see what is considered here as StratumV2 not so much as “the” mining protocol we expect everything to use, but just as the de facto binary interface protocol chosen because we need one anyway, and StratumV2 is already designed to cover the needs of such a protocol. If StratumV2 expansions/evolutions come along, or whatever other protocol, it may not be unreasonable to expect that to need a separate daemon that speaks StratumV2 with Bitcoin Core still.

yep

TheBlueMatt commented at 4:15 pm on June 12, 2023: contributor

None of these deficiencies require the full StratumV2 approach to address;

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.

it is (much, much) lower bandwidth than getblocktemplate,
it explicitly moves decisions of when to switch to a new block to bitcoin core with a push model, rather than long polling which almost never gets used in practice. This allows for substantially better optimization of transaction selection and next-block switching across the ecosystem. This also implies we cannot build this das some kind of layer on the existing bitcoin core APIs, there needs to be something new.
it provides some cryptographic authentication, allowing the protocol to go over the open internet without fear - this allows the protocol to take the place of current systems like CK’s “solopool”, which allows miners to solo mine without the complexity of running a full node and pool stack, and can now be replicated by any full node. More generally, abstracting the block construction logic from a pool via a protocol which doesn’t require tight integration could allow pools or miners with regulatory risk to use a third party template provider.

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 :).

in configure.ac:347 in 21bb3946c4

343@@ -344,6 +344,17 @@ AC_ARG_ENABLE([lto],
344     [enable_lto=$enableval],
345     [enable_lto=no])
346 
347+dnl Enable running a StratumV2 Template Provider server.

TheBlueMatt commented at 4:28 pm on June 12, 2023:

Why bother making this optional? Can just have a config flag.

ccdle12 commented at 9:17 pm on June 12, 2023:

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.

TheBlueMatt commented at 10:35 pm on June 12, 2023:

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.

in src/sv2_template_provider.h:64 in 21bb3946c4

59+    uint16_t optional_features = 0;
60+
61+    /**
62+     * The default option for the additional space required for coinbase output.
63+     */
64+    unsigned int default_coinbase_tx_additional_output_size = 0;

TheBlueMatt commented at 4:30 pm on June 12, 2023:

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.

in src/sv2_template_provider.h:69 in 21bb3946c4

64+    unsigned int default_coinbase_tx_additional_output_size = 0;
65+
66+    /**
67+     * The default flag for all new work.
68+     */
69+    bool default_future_templates = true;

TheBlueMatt commented at 4:33 pm on June 12, 2023:

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?

ccdle12 commented at 9:18 pm on June 12, 2023:

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.

in src/sv2_template_provider.h:54 in 21bb3946c4

49+
50+    /**
51+     * The current protocol version of stratum v2 supported by the server. Not to be confused
52+     * with byte value of identitying the stratum v2 subprotocol.
53+     */
54+    uint16_t protocol_version = 2;

TheBlueMatt commented at 4:34 pm on June 12, 2023:

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.

ccdle12 commented at 9:18 pm on June 12, 2023:

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

luke-jr commented at 10:51 pm on July 27, 2023:

Code readability is more important than “less code”…

TheBlueMatt commented at 4:35 pm on June 12, 2023: contributor

Just a few random notes, I’m not qualified to review modern C++ anymore :joy:

ccdle12 commented at 5:06 pm on June 12, 2023: contributor

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 getblocktemplate and submitblock would be the only needed RPCs. I’ve seen the discussion about separation (multi-process) in #23049 but wasn’t convinced by the arguments there (e.g. #23049 (comment)). I am worried about the maintenance burden of this and would prefer it being a separate thing.

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.

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:

Encoding Template fields to bytes
Encode bytes to HexStr
Build a JSON representation with HexStr fields
Serialize JSON representation to utf8 binary and send over the wire

where as if sv2 provided templates:

Encode Template fields to bytes (sv2 encoding)
Send sv2 bytes over the wire

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

TheBlueMatt commented at 7:32 pm on June 12, 2023: contributor

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.

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.

glozow added the label Mining on Jun 13, 2023

ariard commented at 1:56 am on June 14, 2023: member

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.

Another 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

In the future, it might make sense to split the bitcoin-node process in a bunch of more granualr process (e.g bitcoin-validation, bitcoin-server with indexes, bitcoin-p2p) to avoid exposed DoS leakage between them or having redundance of some functionality like block-relay. What can be a good architecture/deployment fit for a “hobbyist” full-node might not work for a substantial mining pool or Lightning Service Provider.

dunxen commented at 6:09 pm on June 14, 2023: contributor

Concept ACK as an alternative to getblocktemplate. I’m still digging into the deeper bits of StratumV2 though. :)

Sjors commented at 8:04 pm on June 14, 2023: member

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

kristapsk commented at 11:07 am on June 20, 2023: contributor

Concept ACK.

may get my hands on an old S9. So hopefully I can test this when electricity prices go negative

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

luke-jr commented at 2:23 am on June 24, 2023: member

Concept ACK

Sjors commented at 2:53 pm on June 26, 2023: member

@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)

DrahtBot commented at 10:46 pm on June 27, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Jun 27, 2023

in src/init.cpp:643 in 21bb3946c4

637@@ -630,6 +638,10 @@ void SetupServerArgs(ArgsManager& argsman)
638 #if defined(USE_SYSCALL_SANDBOX)
639     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);
640 #endif // USE_SYSCALL_SANDBOX
641+       //
642+#if ENABLE_TEMPLATE_PROVIDER
643+    argsman.AddArg("-stratumv2=<port>", "Listen for stratumv2 connections on <port>. Bitcoind will act as a Template Provider. (default: 8442)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

Sjors commented at 1:11 pm on July 3, 2023:

This should also mention the testnet, regtest and signet ports.

luke-jr commented at 10:48 pm on July 27, 2023:

Probably should be called -stratumv2port, unless it supports specifying an IP to bind to (probably a good idea)

luke-jr commented at 10:49 pm on July 27, 2023:

We don’t typically refer to “Bitcoind” like this

ccdle12 commented at 7:53 am on October 16, 2023:

Thank you for the suggestion. I’ll update the argument name and also add an IP binding option.

ccdle12 commented at 7:55 am on October 16, 2023:

Ok thanks, I’ll remove this reference in the description.

Sjors commented at 1:57 pm on July 3, 2023: member

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.

ccdle12 commented at 9:56 am on July 12, 2023: contributor

Hi @Sjors , thanks and appreciate the review,

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.

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.

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.

You’re right, I’ll work on setting up some functional tests and also adding the debug log category.

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.

This makes more sense. I implemented this change removing the compile configuration flag and just used -stratumv2 and stratumv2port with some default ports for each env.

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.

in src/init.cpp:1892 in 21bb3946c4

1887+    assert(node.mempool);
1888+
1889+    node.sv2_template_provider = std::make_unique<Sv2TemplateProvider>(*node.chainman, *node.mempool);
1890+
1891+    try {
1892+        auto sv2_port{static_cast<uint16_t>(gArgs.GetIntArg("-stratumv2", 8442))};

luke-jr commented at 10:49 pm on July 27, 2023:

Should be added to the port number check earlier

in src/init.cpp:1895 in 21bb3946c4

1890+
1891+    try {
1892+        auto sv2_port{static_cast<uint16_t>(gArgs.GetIntArg("-stratumv2", 8442))};
1893+        node.sv2_template_provider->Start(Sv2TemplateProviderOptions { .port = sv2_port });
1894+    } catch (const std::runtime_error& e) {
1895+        LogPrintf("sv2: %s\n", e.what());

luke-jr commented at 10:50 pm on July 27, 2023:

Rather than prefixing with “sv2:”, suggest a new log category.

ccdle12 commented at 7:55 am on October 16, 2023:

Ok thanks, I’ll add a new log category.

luke-jr changes_requested

ccdle12 commented at 7:56 am on October 16, 2023: contributor

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.

jonatack commented at 6:22 pm on October 16, 2023: contributor

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 #bitcoin-core-dev IRC meetings to provide status updates and answer questions.

Sjors commented at 3:37 pm on November 24, 2023: member

I was able to mine a bit on testnet today using:

@Fi3’s experimental branch: https://github.com/Fi3/bitcoin/commits/PatchTemplates
an S9 running Braiins OS, but using Stratum v1 because apparently the firmware is behind the spec
Translator proxy (v1 -> v2) running locally, connected to my S9, and upstream to a testnet pool. The pool incidentally rugged me because I don’t have an account :-)
Job Declarator client running locally, connected to my node, and upstream to a testnet Job Declarator server

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 getblocktemplate RPC is unaffected, it should be fine to ship work in progress and test-only stratum v2 functionality.

Alternatively we completely figure out how to get everything right before merging anything.

Sjors commented at 2:51 pm on November 30, 2023: member

I have a rebased and reorganised branch: https://github.com/bitcoin/bitcoin/compare/master...Sjors:bitcoin:2023/11/sv2-poll

It’s based on @Fi3’s https://github.com/bitcoin/bitcoin/compare/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.

Sjors commented at 11:39 am on December 1, 2023: member

Happy to leave this open for a bit, but here’s my PR: #28983

Sjors commented at 4:09 pm on December 28, 2023: member

I believe I’ve processed all feedback on this PR into my PR, either as documentation, TODO or actually implemented.

maflcko commented at 8:10 am on December 29, 2023: member

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 https://github.com/bitcoin/bitcoin/pull/28983

maflcko closed this on Dec 29, 2023

[WIP] add a stratum v2 template provider #27854

Description

Context

Motivation

Build

Future work

Code Coverage

Reviews

Conflicts