Stratum v2 Noise Protocol #29346

Sjors · 2024-01-29T16:14:50Z

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 · 2024-01-29T16:14:54Z

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.

src/common/sv2_noise.h

Sjors · 2024-02-01T18:53:41Z

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 · 2024-02-02T11:47:01Z

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)

Sjors · 2024-02-09T09:37:29Z

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 (stratum-mining/sv2-spec#66 (comment)).

Sjors · 2024-02-14T11:47:16Z

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 · 2024-02-16T19:29:56Z

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 · 2024-09-19T14:18:36Z

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

Sjors · 2024-09-20T16:36:21Z

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

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

TODO: move to bitcoin#29346

DrahtBot · 2024-10-08T15:40:09Z

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

stratospher

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

src/logging.h

stratospher · 2024-10-08T06:31:31Z

src/sv2/noise.h

+        46, 180, 120, 129, 32, 142, 158, 238, 31, 102, 159, 103, 198, 110, 231, 14,
+        169, 234, 136, 9, 13, 80, 63, 232, 48, 220, 75, 200, 62, 41, 191, 16};
+
+    // The double hash of protocol name "Noise_NX_EllSwiftXonly_ChaChaPoly_SHA256".


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

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

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.

stratospher · 2024-10-08T14:13:17Z

src/sv2/noise.cpp

+        return false;
+    }
+
+    LogTrace(BCLog::SV2, "Mix hash: %s\n", HexStr(m_symmetric_state.GetHashOutput()));


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

stratospher · 2024-10-08T15:03:34Z

src/sv2/noise.cpp

+    Assume(output.size() == Sv2Cipher::EncryptedMessageSize(input.size()));
+
+    if (m_initiator) {
+        if (!m_cs1.EncryptMessage(input, output)) return false;


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

stratospher · 2024-10-08T15:41:37Z

src/sv2/noise.h

+     * and certificate in the buffer: <- e, ee, s, es, SIGNATURE_NOISE_MESSAGE
+     */
+    void WriteMsgES(Span<std::byte> msg);
+    /** During handshake step 2, read the remote ephmeral key, static key


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.

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.

stratospher · 2024-10-08T17:14:08Z

CMakeLists.txt

@@ -236,6 +238,7 @@ if(BUILD_FOR_FUZZING)
  set(ENABLE_EXTERNAL_SIGNER OFF)
  set(WITH_MINIUPNPC OFF)
  set(WITH_ZMQ OFF)
+  set(WITH_SV2 OFF)


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

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

stratospher · 2024-10-09T05:37:21Z

src/test/fuzz/sv2_noise.cpp

+                                                    XOnlyPubKey(bob_static_key.GetPubKey()), bob_authority_key);
+
+    bool valid_certificate = version == 0 &&
+                             (valid_from <= now) &&


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

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

fanquake · 2024-10-10T09:59:55Z

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 · 2024-10-16T08:48:33Z

I moved this to Sjors#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 mentioned this pull request Jan 29, 2024

Stratum v2 Template Provider (take 2) #28983

Closed

30 tasks

DrahtBot added the CI failed label Jan 29, 2024

This was referenced Jan 29, 2024

kernel: Streamline util library #29015

Merged

logging: use bitset for categories #26697

Closed

Sjors mentioned this pull request Jan 30, 2024

CKey: add Serialize and Unserialize #29295

Closed

Sjors force-pushed the 2024/01/sv2_noise branch from 8a5cd72 to a392a4c Compare January 30, 2024 10:42

winlover32 approved these changes Jan 30, 2024

View reviewed changes

maflcko reviewed Jan 31, 2024

View reviewed changes

src/common/sv2_noise.h Outdated Show resolved Hide resolved

Sjors force-pushed the 2024/01/sv2_noise branch 2 times, most recently from 4bb5f77 to e612cb5 Compare February 1, 2024 18:52

Sjors force-pushed the 2024/01/sv2_noise branch from e612cb5 to 71380de Compare February 2, 2024 11:46

DrahtBot removed the CI failed label Feb 2, 2024

Sjors mentioned this pull request Feb 2, 2024

Initial corpus for sv2_noise_cipher_roundtrip bitcoin-core/qa-assets#169

Closed

Sjors force-pushed the 2024/01/sv2_noise branch from 71380de to f2ef03f Compare February 5, 2024 09:02

Sjors mentioned this pull request Feb 5, 2024

Add Sv2Noise fuzzer Sjors/bitcoin#31

Closed

6 tasks

plebhash mentioned this pull request Feb 7, 2024

Incorrect certificate validation stratum-mining/stratum#717

Closed

Sjors force-pushed the 2024/01/sv2_noise branch from f2ef03f to 1b91655 Compare February 9, 2024 09:34

This was referenced Feb 9, 2024

Broadcast own transactions only via short-lived Tor or I2P connections #29415

Open

log: deduplicate category names and improve logging.cpp #29419

Merged

Sjors force-pushed the 2024/01/sv2_noise branch from 1b91655 to ec207fe Compare February 12, 2024 10:51

Sjors mentioned this pull request Feb 14, 2024

Stratum v2 Template Provider (take 3) #29432

Closed

24 tasks

Sjors force-pushed the 2024/01/sv2_noise branch from ec207fe to af8e738 Compare February 15, 2024 10:12

Sjors marked this pull request as ready for review February 16, 2024 19:04

Sjors force-pushed the 2024/01/sv2_noise branch 2 times, most recently from c1cf1e9 to a2a5a04 Compare September 19, 2024 14:36

This was referenced Sep 19, 2024

Reintroduce external signer support for Windows #29868

Merged

Remove the legacy wallet and BDB dependency #28710

Merged

Sjors force-pushed the 2024/01/sv2_noise branch from a2a5a04 to f1db3bd Compare September 20, 2024 16:18

This was referenced Sep 20, 2024

Embed default ASMap as binary dump header file #28792

Open

net: Replace libnatpmp with built-in PCP+NATPMP implementation #30043

Merged

DrahtBot mentioned this pull request Sep 29, 2024

build: Switch to Qt 6 #30997

Merged

DrahtBot added the Needs rebase label Sep 30, 2024

Sjors and others added 4 commits October 4, 2024 12:09

Add sv2 log category for Stratum v2

b4a84ab

build: libbitcoin_sv2 scaffold

d08a2eb

ci: always build WITH_SV2=ON

5f36962

Add sv2 noise protocol

b588ff8

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

Sjors force-pushed the 2024/01/sv2_noise branch from f1db3bd to b588ff8 Compare October 4, 2024 11:02

DrahtBot removed the Needs rebase label Oct 4, 2024

Sjors added a commit to Sjors/bitcoin that referenced this pull request Oct 4, 2024

ci: set WITH_SV2=ON in more places

ff4dbc3

TODO: move to bitcoin#29346

Sjors added a commit to Sjors/bitcoin that referenced this pull request Oct 5, 2024

ci: set WITH_SV2=ON in more places

54b2da1

TODO: move to bitcoin#29346

DrahtBot mentioned this pull request Oct 7, 2024

ci: Add missing -DWERROR=ON to test-each-commit #31045

Merged

DrahtBot added the Needs rebase label Oct 8, 2024

stratospher reviewed Oct 9, 2024

View reviewed changes

DrahtBot requested a review from itornaza October 9, 2024 06:39

Sjors mentioned this pull request Oct 16, 2024

Sv2 noise Sjors/bitcoin#66

Closed

Sjors closed this Oct 16, 2024

Sjors deleted the 2024/01/sv2_noise branch October 16, 2024 08:48

Sjors mentioned this pull request Oct 16, 2024

Stratum v2 via IPC Mining Interface tracking issue #31098

Open

33 tasks

Stratum v2 Noise Protocol #29346

Stratum v2 Noise Protocol #29346

Uh oh!

Conversation

Sjors commented Jan 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

Sjors commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Feb 9, 2024

Uh oh!

Sjors commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Sep 20, 2024

Uh oh!

DrahtBot commented Oct 8, 2024

Uh oh!

stratospher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake commented Oct 10, 2024

Uh oh!

Sjors commented Oct 16, 2024

Uh oh!

Uh oh!

Sjors commented Jan 29, 2024 •

edited

Loading

DrahtBot commented Jan 29, 2024 •

edited

Loading

Sjors commented Feb 1, 2024 •

edited

Loading

Sjors commented Feb 2, 2024 •

edited

Loading

Sjors commented Feb 14, 2024 •

edited

Loading

Sjors commented Feb 16, 2024 •

edited

Loading

Sjors commented Sep 19, 2024 •

edited

Loading