Stratum v2 Noise Protocol #29346

Sjors commented at 4:14 PM on January 29, 2024: member

Parent PR #29432. Followed by #30315.

This is the first part of the Stratum v2 Template Provider change that can be a standalone pull request.

It introduces a CMake build flag -DWITH_SV2 that is OFF by default (but ON for CI).

The Noise Protocol Framework is defined here: https://noiseprotocol.org/noise.html

It's quite similar to BIP324. The main differences are explained here, including why Stratum v2 can't use BIP234 (yet): https://delvingbitcoin.org/t/stratum-v2-noise-protocol-bip324-nuggets/413

The implementation is based on revision 38, 2018-07-11 (most recent at the time of writing).

The Stratum v2 spec defines the specific choice of ciphers: https://github.com/stratum-mining/sv2-spec/blob/main/04-Protocol-Security.md

This protocol has been implemented in the Stratum Reference Implementation (SRI, using rust-bitcoin). https://github.com/stratum-mining/stratum

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.

DrahtBot commented at 4:14 PM on January 29, 2024: 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
ACK	stratospher
Concept ACK	Shourya742
Stale ACK	itornaza

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:

#31045 (ci: Add missing -DWERROR=ON to test-each-commit by maflcko)
#30595 (kernel: Introduce initial C header API by TheCharlatan)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#28792 (Embed default ASMap as binary dump header file by fjahr)

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

in src/common/sv2_noise.h:66 in a392a4c2f3 outdated

  71 | +    {
  72 | +        s << m_version
  73 | +          << m_valid_from
  74 | +          << m_valid_to;
  75 | +
  76 | +        s.write(MakeByteSpan(m_sig));

maflcko commented at 8:57 AM on January 31, 2024:

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.

Sjors commented at 1:28 PM on February 1, 2024:

Any suggested reading to wrap my head around this serialisation / bytes / span stuff?

maflcko commented at 1:55 PM on February 1, 2024:

A std::byte is serialized no different than an uint8_t. (https://en.cppreference.com/w/cpp/types/byte)

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.

maflcko commented at 1:58 PM on February 1, 2024:

Possible solutions can be:

Remove the Sv2SignatureNoiseMessage default constructor and/or check that the size of m_sig is properly initialized
Use a std::array instead
Serialize as std::vector instead

Sjors commented at 2:53 PM on February 1, 2024:

Since m_sig is always 64 bytes, I changed it to std::array.

maflcko commented at 3:14 PM on February 1, 2024:

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)

Sjors commented at 3:59 PM on February 1, 2024:

If I drop MakeByteSpan() the compiler complains:

./common/sv2_noise.h:68:17: error: no viable conversion from 'const std::array<unsigned char, SCHNORR_SIGNATURE_SIZE>' to 'Span<const value_type>' (aka 'Span<const std::byte>')
        s.write(m_sig);

sipa commented at 4:56 PM on February 1, 2024:

Yes, you'd need to make it an array of std::byte.

maflcko commented at 5:20 PM on February 1, 2024:

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.

Sjors commented at 7:00 PM on February 1, 2024:

I'll look into replace write with >>.

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?

maflcko commented at 8:00 AM on February 2, 2024:

I'll look into replace write with >>.

>> means reading. You may want to try <<, if you want to write an object.

Sjors commented at 10:20 AM on February 2, 2024:

<< m_sig works fine, so m_sig can live happily as a std::array<unsigned char

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.

It also changes the protocol name to Noise_NX_Secp256k1+EllSwift_ChaChaPoly_SHA256 (https://github.com/stratum-mining/sv2-spec/pull/66#discussion_r1483090409).

Sjors force-pushed on Feb 12, 2024

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)

CI run: https://github.com/bitcoin/bitcoin/runs/21654131515

Sjors force-pushed on Mar 1, 2024

Sjors force-pushed on Mar 18, 2024

Sjors force-pushed on Mar 22, 2024

DrahtBot added the label Needs rebase on Apr 2, 2024

Sjors commented at 7:49 AM on April 3, 2024: member

(I plan to rebase this after ASan CI is fixed)

Sjors force-pushed on Apr 5, 2024

Sjors commented at 2:54 PM on April 5, 2024: member

Rebased after #29419.

DrahtBot removed the label Needs rebase on Apr 5, 2024

DrahtBot added the label Needs rebase on Apr 30, 2024

Sjors force-pushed on May 1, 2024

DrahtBot removed the label Needs rebase on May 1, 2024

Sjors force-pushed on May 2, 2024

ryanofsky commented at 6:26 PM on May 13, 2024: contributor

re: #29346#issue-2105856059

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:

class MiningService {
public:
    virtual ~MiningService() {}

    // Returns a new block template based on the current state of the mempool and blockchain
    virtual std::unique_ptr<CBlockTemplate> GetNewBlockTemplate(const CScript& scriptPubKey) = 0;

    // Accepts a completed block from a miner and attempts to add it to the blockchain
    virtual bool SubmitNewBlock(const std::shared_ptr<const CBlock>& block) = 0;

    // Returns the current best block hash
    virtual uint256 GetCurrentBestBlock() = 0;

    // Triggers an update or checks the status of the mempool
    virtual void UpdateMempool() = 0;

    // Additional methods can be added here as needed
};

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

class MiningService {
public:
    virtual ~MiningService() {}

    // Register a callback for new block templates
    virtual std::unique_ptr<Handler> RequestBlockTemplates(const CScript& scriptPubKey, std::function<void(const CBlockTemplate&>callback)) = 0;

    // Accepts a completed block from a miner and attempts to add it to the blockchain
    virtual bool SubmitNewBlock(const std::shared_ptr<const CBlock>& block) = 0;
};

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:

NodeContext& node = EnsureAnyNodeContext(request.context);
ChainstateManager& chainman = EnsureChainman(node);
...
Chainstate& active_chainstate = chainman.ActiveChainstate();
...
const CTxMemPool& mempool = EnsureMemPool(node);
...
pblocktemplate = BlockAssembler{active_chainstate, &mempool}.CreateNewBlock(scriptDummy);

I would write something like:

MiningContext& miner = EnsureAnyMinerContext(request.context);
...
miner.GetNewBlockTemplate(scriptDummy)

And then interface implementation would take care of getting the chain manager and mempool (via the node interface?).

ryanofsky commented at 2:21 PM on May 14, 2024: contributor

Maybe I could start by having the getblocktemplate RPC use that new interface?

That's a good idea, nice way the interface could be used from the start.

And then interface implementation would take care of getting the chain manager and mempool (via the node interface?).

Right, you could look at the interfaces::Chain for an example of how to initialize the interface. Store the interface in NodeContext like:

https://github.com/bitcoin/bitcoin/blob/7fcf4e99791ca5be0b068ac03a81a50ece11dba3/src/node/context.h#L71

Initialize in the init.cpp file like:

https://github.com/bitcoin/bitcoin/blob/7fcf4e99791ca5be0b068ac03a81a50ece11dba3/src/init.cpp#L1113

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.

Then add an Ensure function like you mentioned in https://github.com/bitcoin/bitcoin/blob/master/src/rpc/server_util.h

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::Chain interface::Node interfaces::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.

Update 2024-06-12: more conceptual discussion on what work Bitcoin Core could / should (not) take over from the Job Declarator client role: https://github.com/stratum-mining/sv2-spec/discussions/85

Sjors force-pushed on May 30, 2024

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.

Debug: https://github.com/bitcoin/bitcoin/runs/25592506395

Sjors commented at 12:35 PM on May 30, 2024: member

@ryanofsky let's continue the interface design in #30200.

DrahtBot removed the label CI failed on May 30, 2024

DrahtBot added the label Needs rebase on Jun 12, 2024

Sjors force-pushed on Jun 18, 2024

Sjors commented at 1:22 PM on June 18, 2024: member

Trivial rebase after #29015.

DrahtBot removed the label Needs rebase on Jun 18, 2024

Sjors force-pushed on Jun 18, 2024

DrahtBot added the label CI failed on Jun 18, 2024

DrahtBot removed the label CI failed on Jun 18, 2024

Fi3 commented at 4:41 PM on June 19, 2024: none

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

See also the conceptual discussion in https://github.com/stratum-mining/sv2-spec/discussions/85

Sjors force-pushed on Jun 21, 2024

ryanofsky referenced this in commit 323b0acfcb on Jun 24, 2024

Sjors force-pushed on Jun 25, 2024

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.

Debug: https://github.com/bitcoin/bitcoin/runs/26795764911

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

After going through #29346 (comment), sv2-spec, https://github.com/stratum-mining/sv2-spec/discussions/85 time and time again, I put all the relevant information in sv2-draft-sketch.pdf for the Config A case, to make better sense of the trade offs involved.

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.

Fi3 commented at 9:20 AM on June 30, 2024: none

@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: contributor

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

Fi3 commented at 9:17 AM on July 1, 2024: none

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

Sjors force-pushed on Jul 2, 2024

winlover32 approved

in src/common/sv2_noise.h:26 in a2436955f5 outdated

  21 | + *  https://github.com/stratum-mining/sv2-spec/blob/main/04-Protocol-Security.md
  22 | + */
  23 | +
  24 | +/** Section 3: All Noise messages are less than or equal to 65535 bytes in length. */
  25 | +static constexpr size_t NOISE_MAX_CHUNK_SIZE = 65535;
  26 | +/** Sv2 spec 4.5.2 */

itornaza commented at 5:06 PM on July 4, 2024:

nit: Is that comment needed? SV2 spec 4.5.2 is not relevant to message length.

Sjors commented at 10:15 AM on July 11, 2024:

Looks like this was duplicated from the comment below. Dropped.

in src/common/sv2_noise.h:70 in a2436955f5 outdated

  65 | +          << m_sig;
  66 | +    }
  67 | +    template <typename Stream>
  68 | +    void Unserialize(Stream& s)
  69 | +    {
  70 | +        s >> m_version >> m_valid_from >> m_valid_to >> m_sig;

itornaza commented at 5:09 PM on July 4, 2024:

styling nit: Consider splitting into multiple lines for clarity and consistency with Serialize above.

in src/common/sv2_noise.h:99 in a2436955f5 outdated

  94 | +     * @returns whether decryption succeeded
  95 | +     */
  96 | +    [[nodiscard]] bool DecryptWithAd(Span<const std::byte> associated_data, Span<std::byte> ciphertext, Span<std::byte> plain);
  97 | +    /** Encrypt message
  98 | +     * @param[in] associated_data associated data
  99 | +     * @param[out] plain message

itornaza commented at 5:25 PM on July 4, 2024:

nit: Should the plain message be the @param[in] in the EncryptWithAdd method?

Sjors commented at 10:17 AM on July 11, 2024:

Fixed.

in src/common/sv2_noise.h:100 in a2436955f5 outdated

  95 | +     */
  96 | +    [[nodiscard]] bool DecryptWithAd(Span<const std::byte> associated_data, Span<std::byte> ciphertext, Span<std::byte> plain);
  97 | +    /** Encrypt message
  98 | +     * @param[in] associated_data associated data
  99 | +     * @param[out] plain message
 100 | +     * @param[in] ciphertext message with encrypted and authenticated chunks.

itornaza commented at 5:26 PM on July 4, 2024:

nit: Should the ciphertext message be the @param[out] in the EncryptWithAdd method?

in src/common/sv2_noise.h:237 in a2436955f5 outdated

 232 | +
 233 | +private:
 234 | +    /** Our static key (s) */
 235 | +    CKey m_static_key;
 236 | +    /** EllSwift encoded static key, for optimized ECDH */
 237 | +    EllSwiftPubKey m_our_static_ellswift_pk;

itornaza commented at 5:31 PM on July 4, 2024:

styling nit: Consider dropping the _our part from the variable's name to be more consistent with the other names and be closer to the Noise protocol naming convention s (implicitly our) and rs (explicitly remote). Same for the rest of the names below.

Sjors commented at 10:21 AM on July 11, 2024:

Done

in src/common/sv2_noise.cpp:237 in a2436955f5 outdated

 232 | +    Assume(Sv2Cipher::EncryptedMessageSize(plain.size()) == ciphertext.size());
 233 | +
 234 | +    // The handshake requires mix hashing the cipher text NOT the decrypted
 235 | +    // plaintext.
 236 | +    std::vector<std::byte> ciphertext_copy;
 237 | +    ciphertext_copy.assign(ciphertext.begin(), ciphertext.end()); // (ciphertext.size(), std::byte(0));

itornaza commented at 5:32 PM on July 4, 2024:

nit: Consider removing the commented out code. Same to the following line as well.

itornaza approved

itornaza commented at 5:40 PM on July 4, 2024: contributor

Concept ACK a2436955f5bde920e41e03a5db34d477bfeb04a6

As stated in my previous comments, the implementation of the Stratum v2 Noise Protocol is adding more flexibility to Bitcoin than overhead towards the integration of the Stratum protocol Template Provider and Job Declaration Client roles within bitcoind.

I include some really minor nits that came out during a quick code review against the protocol specs and leaving them here to your discretion.

DrahtBot added the label CI failed on Jul 9, 2024

DrahtBot commented at 7:07 AM on July 9, 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.

Debug: https://github.com/bitcoin/bitcoin/runs/26955016804

Sjors force-pushed on Jul 11, 2024

Sjors commented at 10:29 AM on July 11, 2024: member

Rebased after silent merge conflict with #29625 which moved xoroshiro128plusplus.h. Addressed comments by @itornaza.

DrahtBot removed the label CI failed on Jul 11, 2024

Sjors force-pushed on Jul 17, 2024

Sjors force-pushed on Jul 19, 2024

in src/common/sv2_noise.h:128 in f69fd623ad outdated

 123 | +     *                  as ciphertext. The result is defragmented.
 124 | +     */
 125 | +    [[nodiscard]] bool DecryptMessage(Span<std::byte> ciphertext, Span<std::byte> plain);
 126 | +
 127 | +private:
 128 | +    uint8_t m_key[HASHLEN];

itornaza commented at 4:30 PM on August 3, 2024:

Cpp Core Guidelines explicitly want us to avoid C-Arrays SL.Con.1 and prefer STL'sstd::array which does not degenerate to a pointer and knows its size. Would it be better for us here to use std::array<uint8_t, HASHLEN> m_key or it is not justified in our context?

Update: I gave it a try on my fork's branch pr29346-carray-to-stl 2d28478d0791689070bd23df5b5640e9dae6f786 if you want to cherry pick.

Brock124590 commented at 5:53 PM on August 3, 2024:

Okay

Sjors commented at 11:03 AM on August 8, 2024:

@itornaza thanks, I'll look into your patch. Another guiding principle would be to stay close to bip324.h in terms of coding style. I haven't compared with that yet.

Sjors commented at 1:30 PM on August 8, 2024:

Update: bip324.h also uses std::array

in src/common/sv2_noise.h:171 in f69fd623ad outdated

 166 | +    /* For testing */
 167 | +    void LogChainingKey();
 168 | +    std::string GetChainingKey();
 169 | +
 170 | +private:
 171 | +    uint8_t m_chaining_key[Sv2CipherState::HASHLEN];

itornaza commented at 4:46 PM on August 3, 2024:

Same here

Brock124590 approved

Sjors force-pushed on Aug 8, 2024

Sjors commented at 3:21 PM on August 8, 2024: member

Rebased and I took @itornaza's patch with some changes, see inline comments on https://github.com/bitcoin/bitcoin/commit/2d28478d0791689070bd23df5b5640e9dae6f786.

Sjors force-pushed on Aug 8, 2024

Sjors commented at 4:06 PM on August 8, 2024: member

I can't reproduce the CI failure, but hopefully this fixes it.

See https://github.com/bitcoin/bitcoin/commit/2d28478d0791689070bd23df5b5640e9dae6f786#r145179932

DrahtBot commented at 4:06 PM on August 8, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/28524380497

<details><summary>Hints</summary>

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

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot added the label CI failed on Aug 8, 2024

DrahtBot removed the label CI failed on Aug 8, 2024

itornaza approved

itornaza commented at 4:04 PM on August 9, 2024: contributor

Code review ACK e73740fde107f79eeee8e55c6bed156713abe8a5

This pr closely follows the intersection of the original Noise Protocol paper and the Protocol Security section of the Stratum V2 Specification to provide an implementation of the Noise Protocol on Bitcoin Core. In that way we can now step by step integrate the Stratum protocol Template Provider and Job Declaration Client roles into bitcoind.

All my concerns at the present time were addressed and explained in #29346 (comment), #29346 (comment) and in my patch's thread https://github.com/bitcoin/bitcoin/commit/2d28478d0791689070bd23df5b5640e9dae6f786 by @Sjors and @Fi3.

I also built the commit again, run all unit, functional and extended tests and all of them pass. I also run the unit tests with the help of lldb to closely follow the program's flow and monitor how the variables behave. I intend to do more testing and review of the sv2 series as I learn more and get familiar with it.

Sjors referenced this in commit 2d28478d07 on Aug 13, 2024

Sjors force-pushed on Aug 13, 2024

Sjors commented at 5:06 PM on August 13, 2024: member

I introduced using NoiseHash = std::array<uint8_t, HASHLEN>; for improved readability.

Although all tests passed, I noticed while testing #29432 that SRI hangs up after the handshake. Turns out I broke the HKDF2 implementation, which the test didn't catch because there's no hardcoded test vector. That's fixed now.

hebasto added the label Needs CMake port on Aug 16, 2024

Sjors force-pushed on Aug 29, 2024

Sjors commented at 10:57 AM on August 29, 2024: member

Rebased for CMake (only).

hebasto removed the label Needs CMake port on Aug 29, 2024

Sjors force-pushed on Aug 29, 2024

DrahtBot added the label Needs rebase on Sep 3, 2024

Sjors force-pushed on Sep 4, 2024

Sjors commented at 9:09 AM on September 4, 2024: member

Rebased after #26619.

DrahtBot removed the label Needs rebase on Sep 4, 2024

in src/common/sv2_noise.cpp:114 in d4fc6e6077 outdated

 109 | +
 110 | +    const size_t max_chunk_size = NOISE_MAX_CHUNK_SIZE - Poly1305::TAGLEN;
 111 | +    size_t num_chunks = plain.size() / max_chunk_size;
 112 | +    if (plain.size() % max_chunk_size != 0) {
 113 | +        num_chunks++;
 114 | +    }

Shourya742 commented at 7:07 AM on September 10, 2024:

    size_t num_chunks = (plain.size() + max_chunk_size - 1) / max_chunk_size;

Sjors commented at 2:18 PM on September 19, 2024:

Taken

Sjors force-pushed on Sep 10, 2024

in src/common/sv2_noise.h:105 in 38cbea54d1 outdated

 100 | +
 101 | +    /** Encrypt message
 102 | +     * @param[in] associated_data associated data
 103 | +     * @param[in] plain message
 104 | +     * @param[out] ciphertext message with encrypted and authenticated chunks.
 105 | +     * @returns whether decryption succeeded

Shourya742 commented at 5:52 AM on September 13, 2024:

     * [@returns](/bitcoin-bitcoin/contributor/returns/) whether encryption succeeded

Sjors force-pushed on Sep 13, 2024

Sjors commented at 10:28 AM on September 13, 2024: member

The first two commits of https://github.com/Sjors/bitcoin/pull/62 could be included here to put it behind a -DWITH_SV2=ON feature flag.

I'll include that in a future push once it had some review. It shouldn't be a blocker for this PR, because the classes introduced here are not used yet.

in src/common/sv2_noise.cpp:486 in c8d9d84e21 outdated

 481 | +{
 482 | +    size_t num_chunks = msg_len / (NOISE_MAX_CHUNK_SIZE - Poly1305::TAGLEN);
 483 | +    if (msg_len % (NOISE_MAX_CHUNK_SIZE - Poly1305::TAGLEN) != 0) {
 484 | +        num_chunks++;
 485 | +    }
 486 | +    return msg_len + (num_chunks * Poly1305::TAGLEN);

Shourya742 commented at 3:41 PM on September 14, 2024:

    size_t chunk_size = NOISE_MAX_CHUNK_SIZE - Poly1305::TAGLEN;
    size_t num_chunks = (msg_len + chunk_size - 1) / chunk_size;
    return msg_len + (num_chunks * Poly1305::TAGLEN);

Shourya742 changes_requested

Shourya742 commented at 3:56 PM on September 14, 2024: none

utACK. This looks good, just a few nits. It closely aligns with the Noise NX standards and our SRI noise implementation. I will test it soon and share detailed findings.

in src/common/sv2_noise.h:151 in c8d9d84e21 outdated

 146 | +    static constexpr NoiseHash PROTOCOL_NAME_HASH = {
 147 | +        46, 180, 120, 129, 32, 142, 158, 238, 31, 102, 159, 103, 198, 110, 231, 14,
 148 | +        169, 234, 136, 9, 13, 80, 63, 232, 48, 220, 75, 200, 62, 41, 191, 16};
 149 | +
 150 | +    // The double hash of protocol name "Noise_NX_EllSwiftXonly_ChaChaPoly_SHA256".
 151 | +    static constexpr NoiseHash PROTOCOL_NAME_DOUBLE_HASH = {146, 47, 163, 46, 79, 72, 124, 13, 89, 202, 163, 190, 215, 137, 156, 227,

itornaza commented at 5:47 PM on September 14, 2024:

If you consider updating for other reasons, could you format this in the same way as PROTOCOL_NAME_HASH?

itornaza approved

itornaza commented at 5:48 PM on September 14, 2024: contributor

re ACK c8d9d84e21ba091be6be83261b31684ac693d016

Run all tests locally once again and all of them pass including the extended.

DrahtBot requested review from Shourya742 on Sep 16, 2024

Sjors commented at 2:18 PM on September 19, 2024: member

Added a CMake build flag -DWITH_SV2 that's OFF by default (ON for CI). Also rebased and addressed comments.

Sjors force-pushed on Sep 19, 2024

Sjors force-pushed on Sep 20, 2024

Sjors commented at 4:36 PM on September 20, 2024: member

The macOS 14 native failure is spurious, see #30922.

in src/logging.h:74 in eccd4a959e outdated

  70 | @@ -71,6 +71,7 @@ namespace BCLog {
  71 |          TXRECONCILIATION = (CategoryMask{1} << 26),
  72 |          SCAN        = (CategoryMask{1} << 27),
  73 |          TXPACKAGES  = (CategoryMask{1} << 28),
  74 | +        SV2         = (CategoryMask{1} << 30),

stratospher commented at 4:40 AM on September 26, 2024:

b4a84ab: any reason for skipping 29?

Sjors commented at 9:34 AM on October 10, 2024:

Not sure, maybe 29 existed in an earlier rebase. It might just change it to 29.

DrahtBot added the label Needs rebase on Sep 30, 2024

Add sv2 log category for Stratum v2 b4a84abda9

build: libbitcoin_sv2 scaffold d08a2ebfd0

ci: always build WITH_SV2=ON 5f36962cfc

Add sv2 noise protocol

Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>

b588ff85ae

Sjors force-pushed on Oct 4, 2024

DrahtBot removed the label Needs rebase on Oct 4, 2024

Sjors referenced this in commit ff4dbc3de1 on Oct 4, 2024

Sjors referenced this in commit 54b2da1b9a on Oct 5, 2024

in src/sv2/noise.h:150 in b588ff85ae

 145 | +    // This is the first step required when setting up the chaining key.
 146 | +    static constexpr NoiseHash PROTOCOL_NAME_HASH = {
 147 | +        46, 180, 120, 129, 32, 142, 158, 238, 31, 102, 159, 103, 198, 110, 231, 14,
 148 | +        169, 234, 136, 9, 13, 80, 63, 232, 48, 220, 75, 200, 62, 41, 191, 16};
 149 | +
 150 | +    // The double hash of protocol name "Noise_NX_EllSwiftXonly_ChaChaPoly_SHA256".

stratospher commented at 6:31 AM on October 8, 2024:

b588ff8: shouldn't this be "Noise_NX_Secp256k1+EllSwift_ChaChaPoly_SHA256" (from spec)?

Sjors commented at 9:36 AM on October 10, 2024:

It should match the spec. I'm guessing the comment is wrong, because otherwise it wouldn't work against SRI, but will check.

Sjors commented at 4:19 PM on November 14, 2024:

The spec: https://github.com/stratum-mining/sv2-spec/blob/main/04-Protocol-Security.md#451-handshake-act-1-nx-handshake-part-1---e

Which says Noise_NX_Secp256k1+EllSwift_ChaChaPoly_SHA256. I adjusted the second comment.

I also briefly sanity checked the numerical values with some online tools.

in src/sv2/noise.cpp:429 in b588ff85ae

 424 | +        // We initiated the connection, so it's safe to unconditionally log this:
 425 | +        LogWarning("Invalid certificate: %s\n", HexStr(remote_cert_bytes));
 426 | +        return false;
 427 | +    }
 428 | +
 429 | +    LogTrace(BCLog::SV2, "Mix hash: %s\n", HexStr(m_symmetric_state.GetHashOutput()));

stratospher commented at 2:13 PM on October 8, 2024:

b588ff8: nit: could move the MixHash log up (after DecryptAndHash)and before Validate log for more clarity.

in src/sv2/noise.cpp:500 in b588ff85ae

 495 | +bool Sv2Cipher::EncryptMessage(Span<const std::byte> input, Span<std::byte> output)
 496 | +{
 497 | +    Assume(output.size() == Sv2Cipher::EncryptedMessageSize(input.size()));
 498 | +
 499 | +    if (m_initiator) {
 500 | +        if (!m_cs1.EncryptMessage(input, output)) return false;

stratospher commented at 3:03 PM on October 8, 2024:

b588ff8: (micro nit/feel free to ignore) could return m_cs1.EncryptMessage to keep it consistent with how it's done in DecryptMessage.

DrahtBot added the label Needs rebase on Oct 8, 2024

DrahtBot commented at 3:40 PM on October 8, 2024: contributor

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

in src/sv2/noise.h:226 in b588ff85ae

 221 | +    void ReadMsgEphemeralPK(Span<std::byte> msg);
 222 | +    /** During handshake step 2, put our ephmeral key, static key
 223 | +     * and certificate in the buffer: <- e, ee, s, es, SIGNATURE_NOISE_MESSAGE
 224 | +     */
 225 | +    void WriteMsgES(Span<std::byte> msg);
 226 | +    /** During handshake step 2, read the remote ephmeral key, static key

stratospher commented at 3:41 PM on October 8, 2024:

b588ff8: typo - s/ephmeral/ephemeral in a few places.

since the template provider behaves as the server and only performs the responder handshake flow, it might be useful to mention initiator handshake flow is just for tests.

Sjors commented at 9:39 AM on October 10, 2024:

It's only used in tests, but I think we should review the code in both directions under the assumption that they'll be used in production.

in CMakeLists.txt:241 in b588ff85ae

 237 | @@ -236,6 +238,7 @@ if(BUILD_FOR_FUZZING)
 238 |    set(ENABLE_EXTERNAL_SIGNER OFF)
 239 |    set(WITH_MINIUPNPC OFF)
 240 |    set(WITH_ZMQ OFF)
 241 | +  set(WITH_SV2 OFF)

stratospher commented at 5:14 PM on October 8, 2024:

d08a2ebf: sv2 is OFF when fuzzing - so we need to turn it ON here to fuzz locally. Also the sv2 fuzz tests aren't run on the CI.

Sjors commented at 4:25 PM on November 14, 2024:

Looks like I dropped this in a recent update. WITH_SV2 is now on by default and not turned off by the fuzzer CI. I think...

in src/test/fuzz/sv2_noise.cpp:89 in b588ff85ae

  84 | +
  85 | +    auto bob_certificate = Sv2SignatureNoiseMessage(version, valid_from, valid_to,
  86 | +                                                    XOnlyPubKey(bob_static_key.GetPubKey()), bob_authority_key);
  87 | +
  88 | +    bool valid_certificate = version == 0 &&
  89 | +                             (valid_from <= now) &&

stratospher commented at 5:37 AM on October 9, 2024:

b588ff8: (valid_from < now) && (valid_to > now)

Sjors commented at 4:26 PM on November 14, 2024:

It's an edge case, but valid_from == now and valid_to == now should be allowed.

stratospher commented at 6:39 AM on October 9, 2024: contributor

ACK b588ff8. went through the noise protocol spec. also sharing a pictorial represantion of the NX handshake if it's useful to other reviewers.

DrahtBot requested review from itornaza on Oct 9, 2024

fanquake commented at 9:59 AM on October 10, 2024: member

It's ready for review, but not for merge:

The parent PR may need more conceptual review

This should probably be a draft then. Re more conceptual review, I think it's been made (fairly?) clear by multiple contributors (and from the progress of the sidecar PRs from #29432) which approach is favoured, and it doesn't seem to be the one involving this PR, and implementing all the SV2 logic inside Core.

Sjors commented at 8:48 AM on October 16, 2024: member

I moved this to https://github.com/Sjors/bitcoin/pull/66 so we can focus on building an interface for external applications to use, and getting multiprocess in a release. I plan to open a tracking issue for that.

The followup PRs #30315 and #29432 will also be moved shortly. @stratospher I will apply your feedback on the moved PR later.

Sjors closed this on Oct 16, 2024

Sjors deleted the branch on Oct 16, 2024

bitcoin locked this on Nov 14, 2025