Stratum v2 Noise Protocol #29346

pull Sjors wants to merge 4 commits into bitcoin:master from Sjors:2024/01/sv2_noise changing 16 files +1184 −5
  1. 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:

    1. The parent PR may need more conceptual review (and perhaps the entire spec, but that would really slow things down)
    2. 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.
  2. 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.

  3. DrahtBot added the label CI failed on Jan 29, 2024
  4. Sjors force-pushed on Jan 30, 2024
  5. winlover32 approved
  6. winlover32 approved
  7. 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:

    0./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>')
    1        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
  8. Sjors force-pushed on Feb 1, 2024
  9. Sjors force-pushed on Feb 1, 2024
  10. 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]

  11. Sjors force-pushed on Feb 2, 2024
  12. 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)

  13. DrahtBot removed the label CI failed on Feb 2, 2024
  14. Sjors force-pushed on Feb 5, 2024
  15. Sjors force-pushed on Feb 9, 2024
  16. 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).

  17. Sjors force-pushed on Feb 12, 2024
  18. 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.
  19. Sjors force-pushed on Feb 15, 2024
  20. Sjors marked this as ready for review on Feb 16, 2024
  21. 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

  22. Sjors force-pushed on Mar 1, 2024
  23. Sjors force-pushed on Mar 18, 2024
  24. Sjors force-pushed on Mar 22, 2024
  25. DrahtBot added the label Needs rebase on Apr 2, 2024
  26. Sjors commented at 7:49 am on April 3, 2024: member
    (I plan to rebase this after ASan CI is fixed)
  27. Sjors force-pushed on Apr 5, 2024
  28. Sjors commented at 2:54 pm on April 5, 2024: member
    Rebased after #29419.
  29. DrahtBot removed the label Needs rebase on Apr 5, 2024
  30. DrahtBot added the label Needs rebase on Apr 30, 2024
  31. Sjors force-pushed on May 1, 2024
  32. DrahtBot removed the label Needs rebase on May 1, 2024
  33. Sjors force-pushed on May 2, 2024
  34. 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.

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

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

     0class MiningService {
     1public:
     2    virtual ~MiningService() {}
     3
     4    // Returns a new block template based on the current state of the mempool and blockchain
     5    virtual 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
     8    virtual bool SubmitNewBlock(const std::shared_ptr<const CBlock>& block) = 0;
     9
    10    // Returns the current best block hash
    11    virtual uint256 GetCurrentBestBlock() = 0;
    12
    13    // Triggers an update or checks the status of the mempool
    14    virtual void UpdateMempool() = 0;
    15
    16    // 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:

    0class MiningService {
    1public:
    2    virtual ~MiningService() {}
    3
    4    // Register a callback for new block templates
    5    virtual std::unique_ptr<Handler> RequestBlockTemplates(const CScript& scriptPubKey, std::function<void(const CBlockTemplate&>callback)) = 0;
    6
    7    // Accepts a completed block from a miner and attempts to add it to the blockchain
    8    virtual bool SubmitNewBlock(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.

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

    0NodeContext& node = EnsureAnyNodeContext(request.context);
    1ChainstateManager& chainman = EnsureChainman(node);
    2...
    3Chainstate& active_chainstate = chainman.ActiveChainstate();
    4...
    5const CTxMemPool& mempool = EnsureMemPool(node);
    6...
    7pblocktemplate = BlockAssembler{active_chainstate, &mempool}.CreateNewBlock(scriptDummy);
    

    I would write something like:

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

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

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

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

  40. Sjors force-pushed on May 30, 2024
  41. DrahtBot added the label CI failed on May 30, 2024
  42. 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

  43. Sjors commented at 12:35 pm on May 30, 2024: member
    @ryanofsky let’s continue the interface design in #30200.
  44. DrahtBot removed the label CI failed on May 30, 2024
  45. DrahtBot added the label Needs rebase on Jun 12, 2024
  46. Sjors force-pushed on Jun 18, 2024
  47. Sjors commented at 1:22 pm on June 18, 2024: member
    Trivial rebase after #29015.
  48. DrahtBot removed the label Needs rebase on Jun 18, 2024
  49. Sjors force-pushed on Jun 18, 2024
  50. DrahtBot added the label CI failed on Jun 18, 2024
  51. DrahtBot removed the label CI failed on Jun 18, 2024
  52. 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.
  53. 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

  54. Sjors force-pushed on Jun 21, 2024
  55. ryanofsky referenced this in commit 323b0acfcb on Jun 24, 2024
  56. Sjors force-pushed on Jun 25, 2024
  57. Sjors force-pushed on Jun 28, 2024
  58. DrahtBot added the label CI failed on Jun 28, 2024
  59. 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

  60. Sjors force-pushed on Jun 28, 2024
  61. 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.

  62. DrahtBot removed the label CI failed on Jun 28, 2024
  63. 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:

    1. 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.
    2. 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.
    3. 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.

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

    1. Pool and JDS do not need to communicate (and if they do they do not use the job declaration protocol)
    2. 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.
  65. KristijanSajenko approved
  66. 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?
  67. 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.
  68. Sjors force-pushed on Jul 2, 2024
  69. winlover32 approved
  70. 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.
  71. 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.
  72. 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.
  73. 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?
  74. 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
  75. 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.
  76. itornaza approved
  77. 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.

  78. DrahtBot added the label CI failed on Jul 9, 2024
  79. 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

  80. Sjors force-pushed on Jul 11, 2024
  81. 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.
  82. DrahtBot removed the label CI failed on Jul 11, 2024
  83. Sjors force-pushed on Jul 17, 2024
  84. Sjors force-pushed on Jul 19, 2024
  85. 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
  86. 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
  87. Brock124590 approved
  88. Sjors force-pushed on Aug 8, 2024
  89. 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.
  90. Sjors force-pushed on Aug 8, 2024
  91. 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

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

    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.

  93. DrahtBot added the label CI failed on Aug 8, 2024
  94. DrahtBot removed the label CI failed on Aug 8, 2024
  95. itornaza approved
  96. 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.

  97. Sjors referenced this in commit 2d28478d07 on Aug 13, 2024
  98. Sjors force-pushed on Aug 13, 2024
  99. 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.

  100. hebasto added the label Needs CMake port on Aug 16, 2024
  101. Sjors force-pushed on Aug 29, 2024
  102. Sjors commented at 10:57 am on August 29, 2024: member
    Rebased for CMake (only).
  103. hebasto removed the label Needs CMake port on Aug 29, 2024
  104. Sjors force-pushed on Aug 29, 2024
  105. DrahtBot added the label Needs rebase on Sep 3, 2024
  106. Sjors force-pushed on Sep 4, 2024
  107. Sjors commented at 9:09 am on September 4, 2024: member
    Rebased after #26619.
  108. DrahtBot removed the label Needs rebase on Sep 4, 2024
  109. 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:
    0    size_t num_chunks = (plain.size() + max_chunk_size - 1) / max_chunk_size;
    

    Sjors commented at 2:18 pm on September 19, 2024:
    Taken
  110. Sjors force-pushed on Sep 10, 2024
  111. 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:
    0     * [@returns](/bitcoin-bitcoin/contributor/returns/) whether encryption succeeded
    
  112. Sjors force-pushed on Sep 13, 2024
  113. 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.

  114. 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:
    0    size_t chunk_size = NOISE_MAX_CHUNK_SIZE - Poly1305::TAGLEN;
    1    size_t num_chunks = (msg_len + chunk_size - 1) / chunk_size;
    2    return msg_len + (num_chunks * Poly1305::TAGLEN);
    
  115. Shourya742 changes_requested
  116. 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.
  117. 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?
  118. itornaza approved
  119. 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.

  120. DrahtBot requested review from Shourya742 on Sep 16, 2024
  121. 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.
  122. Sjors force-pushed on Sep 19, 2024
  123. Sjors force-pushed on Sep 19, 2024
  124. Sjors force-pushed on Sep 20, 2024
  125. Sjors commented at 4:36 pm on September 20, 2024: member
    The macOS 14 native failure is spurious, see #30922.
  126. 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.
  127. DrahtBot added the label Needs rebase on Sep 30, 2024
  128. Add sv2 log category for Stratum v2 b4a84abda9
  129. build: libbitcoin_sv2 scaffold d08a2ebfd0
  130. ci: always build WITH_SV2=ON 5f36962cfc
  131. Add sv2 noise protocol
    Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
    b588ff85ae
  132. Sjors force-pushed on Oct 4, 2024
  133. DrahtBot removed the label Needs rebase on Oct 4, 2024
  134. Sjors referenced this in commit ff4dbc3de1 on Oct 4, 2024
  135. Sjors referenced this in commit 54b2da1b9a on Oct 5, 2024
  136. 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.

  137. 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.
  138. 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.
  139. DrahtBot added the label Needs rebase on Oct 8, 2024
  140. DrahtBot commented at 3:40 pm on October 8, 2024: contributor

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

  141. 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.
  142. 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…
  143. 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.
  144. 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.
  145. DrahtBot requested review from itornaza on Oct 9, 2024
  146. 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.

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

  148. Sjors closed this on Oct 16, 2024

  149. Sjors deleted the branch on Oct 16, 2024

github-metadata-mirror

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-12-25 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me