It has also been (independently) implemented in BraiinsOS. This part is currently not open source, and it’s behind on some recent changes.
The classes in sv2_noise.h attempt to stay close to the paper, whereas the test and fuzzer code borrow heavily from BIP324.
It’s ready for review, but not for merge:
The parent PR may need more conceptual review (and perhaps the entire spec, but that would really slow things down)
We could decide to not support Noise encryption at all and require users to install separate software for that. Code review of this PR could help inform that decision.
You can see this code in action in the parent PR #29432.
See the guideline for information on the review process.
A summary of reviews will appear here.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#26697 (logging: use bitset for categories by LarryRuane)
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 added the label
CI failed
on Jan 29, 2024
Sjors force-pushed
on Jan 30, 2024
winlover32 approved
winlover32 approved
in
src/common/sv2_noise.h:66
in
a392a4c2f3outdated
You can’t serialize a non-fixed-size vector as a span. This will lead to errors when deserializing. For example, a default constructed empty vector, serialized, can not be serialized into a vector which has been resized to 64 bytes.
A span holds a pointer and a size. It points to memory outside of itself. For example, a string_view (special kind of span) creates a view on a pre-existing string (string literal or std::string).
When serializing a span in Bitcoin Core, the size is not written. Thus, the size of the pointed-to memory must be exactly equal every time at runtime. How you achieve that the memory is always the same length, or whether a span is the right choice, depends on the context.
You don’t need a Make*ByteSpan when the underlying data is already bytes. Also, you don’t need Span at all, when the object is an array. (arrays as well have their size not serialized)
The member function write is the low-level interface for stream-like classes to accept a span of bytes. Normally, it shouldn’t be used in common code, only internally in serialization.
If you want to serialize an object in Bitcoin Core, you can use the << operator to write and >> to read.
Not sure if it’s worth changing m_sig from std::array<unsigned char> to std::array<std::byte>. It’s only used by VerifySchnorr and SignSchnorr which want a Span<unsigned char>, so whether we cast it here or there doesn’t matter?
<< m_sig works fine, so m_sig can live happily as a std::array<unsigned char
Sjors force-pushed
on Feb 1, 2024
Sjors force-pushed
on Feb 1, 2024
Sjors
commented at 6:53 pm on February 1, 2024:
member
I moved the static constants into classes and renamed them a bit. Expanded the fuzzer to send multiple messages back and forth (like the BIP324 fuzzer).
I dropped the 1 hour wiggle room in certificate timestamps, because it adds complexity and I don’t expect this to cause any issues in practice. A self-signed certificate is generated when the TemplateProvider loads, which is unlikely the same second someone connects to it.
The fuzzer found a bug where I forgot to defragment messages larger than the chunk size. Since the Template Provider generally only sends large messages, this (probably) wasn’t caught in testing.
DecryptAndHash was broken in a way that my initial tests didn’t catch (not sure why), nor interoperability tests because this iniator-side function is only used in test code. But I managed to break something in the ensuing refactor, will clean it up later…
[it’s currently in a broken state, second ECDH handshake gives different result, maybe due to bad memory management]
Sjors force-pushed
on Feb 2, 2024
Sjors
commented at 11:47 am on February 2, 2024:
member
Alright, code is back in working state. I dropped a bunch of spurious Make*ByteSpan (mostly by switching remaining uses of uint8_t to std::byte).
Also switched to the new logging convention, mostly LogTrace(). Also expanded the fuzzer to mess with bytes during the handshake.
(fuzzing for a few hours now, will submit a draft corpus if it doesn’t find anything)
DrahtBot removed the label
CI failed
on Feb 2, 2024
Sjors force-pushed
on Feb 5, 2024
Sjors force-pushed
on Feb 9, 2024
Sjors
commented at 9:37 am on February 9, 2024:
member
This latest push improves support for mock time (which the parent PR needs). It’s still using the deprecated GetTime, but it’s not clear to me what to replace it with.
Sjors
commented at 11:47 am on February 14, 2024:
member
Updated and simplified the description to account for the new parent PR. I’m leaving this in draft status pending ~two merges~ one merge on the SRI side; that way the whole thing is easier to test.
Sjors force-pushed
on Feb 15, 2024
Sjors marked this as ready for review
on Feb 16, 2024
Sjors
commented at 7:29 pm on February 16, 2024:
member
Possibly relevant: the test introduced here, and not modified later, failed at least once in the parent PR: #29432 (comment)
We could decide to not support Noise encryption at all and require users to install separate software for that. Code review of this PR could help inform that decision.
I’m just starting to look at this PR and #29432, and having separate software to handle the encryption and networking does seem like a pretty appealing approach.
However, even if the goal is to directly add support for the stratum protocol to bitcoin core, I think it would be good to have a little more separation between the stratum and node code, so instead of accessing the mempool and blockassembler and chainman and g_best_block directly, the stratum code would call a more well-defined interface. This could be a new interface class in src/interfaces/ called something like MiningServer or BlockTemplateProvider with a few methods for polling the mempool and chainstate, generating block templates and submitting new blocks.
I think this would be good in longer term to have more decoupling and potential process and project separation. But it could also help review in shorter term, by giving reviewers like me who are more familiar with bitcoin core code than with the stratum protocol another entry point for understanding the code.
Sjors
commented at 6:46 pm on May 13, 2024:
member
the stratum code would call a more well-defined interface
I would like that too. Just not sure how to go about it.
ryanofsky
commented at 7:40 pm on May 13, 2024:
contributor
the stratum code would call a more well-defined interface
I would like that too. Just not sure how to go about it.
I think in #29432, you could add a src/interfaces/mining_service.h header similar to src/interfaces/chain.h that looks something like:
0classMiningService {
1public: 2virtual~MiningService() {}
3 4// Returns a new block template based on the current state of the mempool and blockchain
5virtual std::unique_ptr<CBlockTemplate> GetNewBlockTemplate(const CScript& scriptPubKey) =0;
6 7// Accepts a completed block from a miner and attempts to add it to the blockchain
8virtualboolSubmitNewBlock(const std::shared_ptr<const CBlock>& block) =0;
910// Returns the current best block hash
11virtual uint256 GetCurrentBestBlock() =0;
1213// Triggers an update or checks the status of the mempool
14virtualvoidUpdateMempool() =0;
1516// Additional methods can be added here as needed
17};
^^^ was suggested by chatgpt after I uploaded sv2_template_provider.cpp
Implementation of the interface could go in src/node/interfaces.cpp and I think only sv2_template_provider.cpp would need to call the interface unless there are other parts of the PR doing similar things.
Or another approach if we wanted to move polling out of sv2_template_provider.cpp into the node could be an interface more like:
0classMiningService {
1public:2virtual~MiningService() {}
34// Register a callback for new block templates
5virtual std::unique_ptr<Handler> RequestBlockTemplates(const CScript& scriptPubKey, std::function<void(const CBlockTemplate&>callback)) =0;
67// Accepts a completed block from a miner and attempts to add it to the blockchain
8virtualboolSubmitNewBlock(const std::shared_ptr<const CBlock>& block) =0;
9};
I don’t think defining an interface is necessary, but it could be a useful way of breaking up the code.
Sjors
commented at 7:11 am on May 14, 2024:
member
@ryanofsky thanks for the suggestion! Maybe I could start by having the getblocktemplate RPC use that new interface? So instead of having:
You could just copy the makeChain method there, which has the boilerplate used to instantiate the interface and give it access to the chainstate and mempool.
I’d maybe call the interface something like MiningService or TemplateProvider rather than MiningContext. I think of “Context” as being a passive container for state instead of an object you would call methods on. The top level interfaces like interfaces::Chaininterface::Nodeinterfaces::Wallet are actually stateless themselves, and only point to state stored in other places. (The idea is to let interface instances be created and destroyed freely, since with multiprocess support new processes can connect and create interface pointers that get destroyed on disconnection, and the state of the node shouldn’t be affected too much from connections and disconnections. Anyway, the mining interface could follow this pattern.)
I am also still curious about the idea to “to not support Noise encryption at all and require users to install separate software.” I’m not sure if this has already been discussed somewhere or if you have more thoughts on it. Looking at this PR, the noise protocol implementation does not seem too complicated, and seems like it would not be a big deal to review and maintain as part of bitcoin core, but I’m not sure yet about the rest of the mining protocol. I’m also not sure how much work it would be to create separate software, if there could be a middle ground for mining support like there is for gui support, which is distributed with bitcoin core, but in a separate executable and with a separate github repository.
Sjors
commented at 8:17 am on May 28, 2024:
member
if you have more thoughts on it
Whenever I talk to people interested in using Stratum v2 I find it difficult to explain all the moving parts they need to install and configure.
Install:
a separate Bitcoin Core branch (temporary, pending #29432 merge)
a job declaration client
a sv1-sv2 translator (temporary, pending sv2 firmware support)
Configure:
point job declaration client to Bitcoin Core, to the pools job declaration server and to the pool itself
point translator to job declaration client
point miner to translator
various public keys for noise encryption (optionally private key and certificate)
Ideally mining should be as easy as: install Bitcoin Core, paste a single pool URL into Bitcoin Core.
In my ideal scenario the miner would find Bitcoin Core via mDNS, but that’s a fresh can of worms that I haven’t thought through in great detail. So instead a user will have to point to Bitcoin Core in the miner interface (typically a web server running on the device) [0].
To get there we’d have to implement not just the Template Provider role (#29432), but also the Job Declarator role. Additionally we’d have to implement either a (simple) Mining Proxy or finish the Job Distribution Protocol spec, so that the miner can communicate directly with the pool. See the protocol overview: https://github.com/stratum-mining/sv2-spec/blob/main/03-Protocol-Overview.md
To keep #29432 manageable, and because I don’t know if we want to add that much mining functionality, I haven’t done this. Instead the user has to install and configure the SRI Job Declarator client role, which also acts as a Proxy role. Probably on the same machine as the node.
We talk to the SRI software using noise encryption, but we could also talk to it with a unix socket. That is, if SRI adds support for that. Regardless of the transport layer, we have to support the sv2 protocol messages.
Implementing Noise encryption (and the sv2 protocol messages) now gives us the flexibility later to take over the Job Declarator client and Proxy role, removing the need to install SRI. At least for simple setups, e.g. a single ASIC (so the proxy doesn’t have to worry about how to distribute work).
IIUC the Job Declarator client role was designed to keep the Template Provider role easier to implement. To take it over we’d have to implement a few more messages: AllocateMiningJobToken, DeclareMiningJob, IdentifyTransactions and SubmitSolution. The IdentifyTransactions part is similar to how compact blocks work, where we only send full transactions to the pool if they don’t already have them.
While taking over the Job Declarator client role seems like a fairly straight-forward followup, taking over the Mining Proxy role is perhaps less so. Designing and implementing a Job Distribution Protocol, which IIUC avoids the need to implement a Mining Proxy, may be a better path. The user would then configure their miner to point to Bitcoin Core for templates as well as to the pool to submit shares.
If in the end we decide to do neither, then perhaps implementing noise was overkill. But as you point out it’s not that complicated.
Any complexity can be hidden from user with node-in-a-box solutions like Umbrel, but it’s nice if they can just use Bitcoin Core. Having a separate binary maintained by us, like HWI, adds installation and configuration complexity. For HWI that’s unavoidable because we don’t want to include the USB driver kitchen sink.
I don’t think simplicity is merely nice for amateur home miners with a single S9 space heater; I suspect even small scale “professional” miners are not sys-admins. Running and configuring a seperate job declarator client process making sure it restarts after a crash is non-trivial. Much easier to just point the miner directly to a pool and not bother with custom template creation. So imo we need to lower the barrier as much as (safely) possible.
An additional argument for implementing noise encryption support, completely separate from the above setup complexity, is that it allows making the template provider publicly reachable. I’m still a bit skeptical about that use case, but see discussion here: https://github.com/stratum-mining/sv2-spec/issues/63#issuecomment-1866515958
[0] on a hypothetical stratum v2 aware miner that is, which doesn’t exist. Today you have to point the miner to a sv1 - sv2 translator which runs on your computer. This translator points to your job declarator client.
DrahtBot added the label
CI failed
on May 30, 2024
DrahtBot
commented at 11:09 am on May 30, 2024:
contributor
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.
Possibly this is 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.
Leave a comment here, if you need help tracking down a confusing failure.
@Sjors initially the TP was without noise, we added it mainly to have third party TPs (not pool and not miner). About easy of use, if we don’t want to implement an sv1 translator in core, the miner still have to deploy a translator proxy. Maybe is easier to just release a translator that is also a JDC (like what I did for demand) rather the implement the JDC in the TP and add custom message to do that.
Sjors
commented at 8:17 am on June 20, 2024:
member
I’m still hopeful sv1 translators won’t be needed at all soon(tm). That certainly seems too complicated to implement in Bitcoin Core.
For the same reason I don’t think combining JDC and Translator is a good long term solution, though in the short run it could make the SRI side of things easier to configure.
ryanofsky referenced this in commit
323b0acfcb
on Jun 24, 2024
Sjors force-pushed
on Jun 25, 2024
Add sv2 log category for Stratum v23f25575ecc
Sjors force-pushed
on Jun 28, 2024
DrahtBot added the label
CI failed
on Jun 28, 2024
DrahtBot
commented at 8:16 am on June 28, 2024:
contributor
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.
Possibly this is 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.
Leave a comment here, if you need help tracking down a confusing failure.
Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
4778bb7049
Sjors force-pushed
on Jun 28, 2024
Sjors
commented at 9:11 am on June 28, 2024:
member
The MSan, depends job failed the test which issues the certificate 1 second in the future. I increased the time difference so such failure is less likely. See diff.
The fuzzer could also pick 1 second in the past or future for the certificate validity test. But it uses SetMockTime, so presumably doesn’t have this problem.
DrahtBot removed the label
CI failed
on Jun 28, 2024
itornaza
commented at 4:27 pm on June 28, 2024:
none
With the assumption that I understood the protocol correctly, it seems that integrating the Noise Protocol into Bitcoin Core adds much more flexibility and options than overhead #29346 (comment). Thus, Noise Protocol integration allows for the following:
Implement the Template Provider (TP) role and the Template Distribution Protocol into Bitcoin and point to SRI software for the Job Declarator Client (JDC) and Mining Proxy.
Implement the Template Provider (TP), Job Declarator Client (JDC) roles, and Template Distribution and Job Distribution protocols into Bitcoin and point to SRI software for Mining Proxy. This option makes the life of amateur miners easier since they do not need to configure a third party JDC.
The Template Provider (TP) can be publicly reachable.
In the longer term, and if it is decided to aim for the incorporation of the Template Provider (TP), Job Declarator Client (JDC) and Mining Proxy into Bitcoin, the Noise Protocol can facilitate all the interim solutions until the rest of the modules are production ready.
@itornaza I agree with you about the implication of integrating noise into the TP. Just want to point out 2 minors issues in your sketch:
Pool and JDS do not need to communicate (and if they do they do not use the job declaration protocol)
JDS and TP do not communicate using the template distribution protocol. JDS <-> TP communication is not part of Sv2. For example the JDS on SRI just use bitcoind RPC API.
KristijanSajenko approved
itornaza
commented at 10:38 am on June 30, 2024:
none
@Fi3 thank you for clarifying it really helped a lot! Would you also think this is a good point to start reviewing the Noise Protocol implementation on this PR compared to it’s specification?
@itornaza If I remember correctly anything needed to implement sv2 noise is contained in the sv2 spec and the noise spec are a little bit dispersive. But I implemented it on SRI more then one year ago so take it with a grain of salt.
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-07-01 10:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
