Stratum v2 Template Provider (take 2) #28983

pull Sjors wants to merge 20 commits into bitcoin:master from Sjors:2023/11/sv2-poll changing 54 files +7118 −2034
  1. Sjors commented at 11:39 am on December 1, 2023: member

    Based on on @Fi3’s https://github.com/bitcoin/bitcoin/compare/master...Fi3:bitcoin:PatchTemplates which is based on @ccdle12’s #27854. I rebased it and re-wrote the commit history. See the original branch for the evolution of the spec.

    See docs/stratum-v2.md for a brief description of Stratum v2 and the role of Bitcoin Core in that system..

    What to test and review?

    • #29295
    • Custom signet mining improvement: #29032
    • Make nested pull-requests easier: #29274

    See the testing guide for various ways to test this PR. This branch is actively used by (testnet) pools, so it should be ready for high level review.

    I’ll make separate pull requests for parts that are ready for detailed review. Probably starting with ECDH and the Noise Protocol.

    Implementation notes

    Silent Payments also needs the ECDH module, so I cherry-picked the commit from #28122. It uses ECDH is a slightly different way, but perhaps there’s more overlap to be had.

    Contributing

    If you want to help out with any of the issues below, please open a PR to my fork. I will then squash your commits into my own where needed.

    Upstream issues

    Spec

    • modify spec to use ProvideMissingTransactions? (followup?)
    • pick a good default for default_coinbase_tx_additional_output_size (see getblocktemplate RPC)

    Noise

    Networking

    • add -sv2bind and -sv2allowip
    • persist static key (needs #29229)
    • optional -sv2cert
    • create Sv2Transport subclass similar to V2Transport: https://github.com/Sjors/bitcoin/pull/27
    • drop Sv2TemplateProvider::SendBuf, reuse p2p socket handling if possible
    • limit number of connected clients
    • maybe limit (number of) coinbase_output_max_additional_size
    • TMP / TODO comments at the top of sv2_messages.h
    • adjust base58 encoding of authority public key https://github.com/stratum-mining/stratum/issues/721

    Testing

    • expand sv2_template_provider_tests
    • add transport fuzzer
    • add template provider fuzzer

    Template generation and updating

    • hold on to templates a bit after a block is found https://github.com/stratum-mining/stratum/issues/709 (in case of race to prevent downstream crashes, though we still wouldn’t relay it without additional changes)
    • hold on to template transactions even if the mempool drops them (for some time)
    • group templates with the same coinbase_tx_additional_output_size
    • don’t generate templates when no client is connected

    Misc

    Potential followups

    • implement Noise protocol and mock client in Python, add functional tests (based on test/sv2_template_provider_tests.cpp)
    • use process separation, e.g. a bitcoin-tp binary, see multiprocess.md
    • make template updates push based, on top of Cluster Mempool, see docs/stratum-v2.md (for new blocks it’s already push based)
    • push empty template for the next block (downstream can ignore or use, https://github.com/stratum-mining/stratum/issues/715)
      • send prevhash for this template as soon as any new block arrives
    • push optimistic template for the next block
      • send prevhash if and only if our template won (i.e. we got a SubmitSolution message)
  2. DrahtBot commented at 11:39 am on December 1, 2023: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #29295 (CKey: add Serialize and Unserialize by Sjors)
    • #29256 (Improve new LogDebug/Trace/Info/Warning/Error Macros by ryanofsky)
    • #29229 (Make (Read/Write)BinaryFile work with char vector, use AutoFile by Sjors)
    • #29119 (refactor: Use std::span over Span by maflcko)
    • #29071 (refactor: Remove Span operator==, Use std::ranges::equal by maflcko)
    • #29015 (kernel: Streamline util library by ryanofsky)
    • #28955 (index: block filters sync, reduce disk read operations by caching last header by furszy)
    • #28875 (build: Pass sanitize flags to instrument libsecp256k1 code by hebasto)
    • #28417 (contrib/signet/miner updates by ajtowns)
    • #28241 (Silent payment index (for light wallets and consistency check) by Sjors)
    • #26697 (logging: use bitset for categories by LarryRuane)
    • #26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)
    • #24539 (Add a “tx output spender” index by sstone)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label CI failed on Dec 1, 2023
  4. Sjors force-pushed on Dec 8, 2023
  5. Sjors commented at 11:04 pm on December 8, 2023: member

    Rebased and fixed encoding mistake I made during rebase:

     0diff --git a/src/node/sv2_messages.h b/src/node/sv2_messages.h
 1index fd6974b998..d3b257c31c 100644
 2--- a/src/node/sv2_messages.h
 3+++ b/src/node/sv2_messages.h
 4@@ -533,11 +533,11 @@ struct Sv2SubmitSolutionMsg
 5         s >> m_template_id >> m_version >> m_header_timestamp >> m_header_nonce;
 6 
 7         // Ignore the 2 byte length as the rest of the stream is assumed to be
 8         // the m_coinbase_tx.
 9         s.ignore(2);
10-        s >> TX_NO_WITNESS(m_coinbase_tx);
11+        s >> TX_WITH_WITNESS(m_coinbase_tx);
12     }
13 };

    Yes, the coinbase transaction has a witness.

  6. glozow added the label Mining on Dec 10, 2023
  7. Sjors force-pushed on Dec 11, 2023
  8. Sjors commented at 4:12 pm on December 11, 2023: member
    I moved sv2_noise.h and sv2_messages.h to common and template_provider.h to node.
  9. Sjors force-pushed on Dec 11, 2023
  10. Sjors force-pushed on Dec 11, 2023
  11. Sjors commented at 9:34 pm on December 11, 2023: member

    I de-duplicated code in Send() by having it call SendBuf(). Later I’d like to see if we can reuse some code from Transport, since there are some similarities with v2 transport.

    I dropped SendTemplate and overhauled SendWork, ThreadSv2Handler, the COINBASE_OUTPUT_DATA_SIZE handler and BuildNewWorkSet to de-duplicate stuff, make it easier to understand and to avoid passing m_ variables.

    Hopefully I didn’t break anything, but I did test on my custom CPU mined signet.

  12. Sjors force-pushed on Dec 12, 2023
  13. Sjors commented at 3:28 pm on December 12, 2023: member
    Pleased clang-tidy. Fixed memory issue by disabling a broken test, added Assume to noise.cpp to check the message size. Added a (possibly overkill) destructor to Sv2Template.
  14. Sjors force-pushed on Dec 12, 2023
  15. Sjors force-pushed on Dec 12, 2023
  16. Sjors commented at 6:08 pm on December 12, 2023: member
    Ran clang-tidy locally to (hopefully) fix its remaining complaints. Included @Fi3’s https://github.com/Sjors/bitcoin/pull/25 (plus additional header and test change) which drops the CIPHER_CONFIRMATION state, as per https://github.com/stratum-mining/sv2-spec/issues/60.
  17. Sjors force-pushed on Dec 12, 2023
  18. Sjors force-pushed on Dec 12, 2023
  19. Sjors commented at 7:56 pm on December 12, 2023: member

    I guess I didn’t catch them all locally. Pushed a change to the last (linter commit). Also fixed the broken noise handshake test. Replaced usleep with UninterruptibleSleep to keep Windows happy. @hebasto any idea how to handle D:\a\bitcoin\bitcoin\src\common\sv2_noise.h(92,13): error C3646: 'WriteMsg': unknown override specifier ?

    0class Sv2CipherState
1{
2public:
3    Sv2CipherState() = default;
4    ...
5    ssize_t WriteMsg(Span<std::byte> msg);
  20. Sjors force-pushed on Dec 13, 2023
  21. Sjors commented at 9:19 am on December 13, 2023: member

    It’s not easy being green, but including compat.h should fix the issue:

    0// ssize_t is POSIX, and not present when using MSVC.

    The CI fuzzer caught an overflow, so I added a guard against absurd coinbase_output_max_additional_size values. I have not tried the fuzzer myself yet, nor does it have complete coverage, so there may still be other issues.

  22. Sjors force-pushed on Dec 13, 2023
  23. Sjors force-pushed on Dec 13, 2023
  24. DrahtBot removed the label CI failed on Dec 13, 2023
  25. hebasto commented at 3:28 pm on December 13, 2023: member

    @hebasto any idea how to handle D:\a\bitcoin\bitcoin\src\common\sv2_noise.h(92,13): error C3646: 'WriteMsg': unknown override specifier ?

    It’s not easy being green

    Happy to see CI is green :)

  26. Sjors force-pushed on Dec 14, 2023
  27. Sjors commented at 3:20 pm on December 14, 2023: member
    I renamed -stratumv2 to -sv2 and stratumv2port to -sv2port. Added -sv2interval (default 30 seconds) and -sv2feedelta (default 1000 sat) to control how often templates are updated.
  28. Sjors force-pushed on Dec 18, 2023
  29. Sjors commented at 10:51 am on December 18, 2023: member
    Integrated changes from https://github.com/Sjors/bitcoin/pull/26: further noise simplification, and chunking large messages. This requires https://github.com/Fi3/stratum-1/tree/UpdateNoise until it’s merged into the dev branch. I also renamed SendMsg in noise.h to WriteMsg. And rebased.
  30. Sjors force-pushed on Dec 19, 2023
  31. Sjors commented at 3:27 pm on December 19, 2023: member

    I moved chunking into noise.h, since the maximum message length of 65 KB is part of the protocol. I also dropped ProcessMsg handler in favor of handling handshake vs. regular transport more directly. Finally SendBuf is now called by EncryptAndSendMessage rather than by each message call site.

    Main changes:

    • 945838fca264c8469e24d629a4552741dbbd4dda -> f08a10a892fc467dbf6037e0c73227822e9f698a
    • fc0aa335dd3f3beba383a42f19f73822b4ad2d34 -> 028c286d9a38343d8240c62a67207681d024a326
    • f706dffba9cf7552b179607d8674ba40cdeecd62 -> 8424d9270c27dd0d453895d36de6e070f3f6cd65

    Note: I’m still investigating a ~potential~ bug for packages that require more than one chunk.

  32. DrahtBot added the label CI failed on Dec 19, 2023
  33. Sjors force-pushed on Dec 19, 2023
  34. Sjors commented at 5:25 pm on December 19, 2023: member

    @Fi3 found the issue, fixed in a7f08e74428c4e5da62365d686e5c51c5afc959d

    0--- a/src/common/sv2_noise.cpp
1+++ b/src/common/sv2_noise.cpp
2@@ -56,11 +56,11 @@ void Sv2CipherState::EncryptMessage(Span<std::byte> input, Span<std::byte> outpu
3     for (size_t i = 0; i < num_chunks; ++i) {
4         size_t chunk_start = i * max_chunk_size;
5         size_t chunk_end = std::min(chunk_start + max_chunk_size, input.size());
6         size_t chunk_size = chunk_end - chunk_start;
7         const auto encrypted_chunk_start = output.begin() + i * NOISE_MAX_CHUNK_SIZE;
8-        std::copy(input.begin(), input.begin() + chunk_size, encrypted_chunk_start);
9+        std::copy(input.begin() + chunk_start, input.begin() + chunk_start + chunk_size, encrypted_chunk_start);
  35. Sjors force-pushed on Dec 19, 2023
  36. Sjors force-pushed on Dec 19, 2023
  37. DrahtBot removed the label CI failed on Dec 19, 2023
  38. Sjors force-pushed on Dec 28, 2023
  39. Sjors commented at 1:10 pm on December 28, 2023: member
    Hopefully fixed the “test each commit” CI. Added documentation, moved testing instructions from the PR descriptor to it. The doc now also explains how to test this PR with a custom signet - which so far seems the most practical approach, despite its many moving parts.
  40. Sjors force-pushed on Dec 28, 2023
  41. Sjors force-pushed on Dec 29, 2023
  42. Sjors force-pushed on Jan 8, 2024
  43. Sjors commented at 9:21 pm on January 8, 2024: member

    Rebased and big refactor.

    Introducing Sv2Transport, a subclass of Transport very similar to V2Transport. For the most part this abstraction works quite well, and should make it easy to review for people familiar with the v2 transport (BIP324) implementation.

    There’a few pain points though, perhaps @sipa has thoughts on this?

    1. CNetMessage and Sv2NetMsg can perhaps be united to a single base class, but there are differences: Sv2NetMsg does not need std::string m_type. This is why I ended up not implementing the override for GetReceivedMessage()

    2. Similar to (1), SetMessageToSend expects CSerializedNetMsg, but we need to keep track of the the header separately (because it contains the message length). So I implemented SetMessageToSend(Sv2NetMsg& msg) instead.

    3. (very minor): the m_type part of GetBytesToSend is not needed

    4. Sv2TemplateProvider::ThreadSv2Handler() contains some copy-pasted stuff from CConnman::SocketSendData. Ideally I’d like to extract some common functionality from CConnman that deals with sockets (and maybe also memory use checking, though I think that’s less of an issue for sv2).

    Another implementation thing to consider, though I think that can wait until I start splitting this into separate PR’s, is that I’m trying to organise the code in such a way that putting the sv2 Template Provider in a separate process will be relatively straightforward to do as a followup. One of the goals of the sv2 project is for template providers to run as a public service. Multiple changes to this PR are needed to do that safely, but one thing that should help is (eventually) moving it out of the node process, so that worst case only the TP crashes.

    In particular I’ve been moving stuff so that Sv2Transport has no dependency on Node, by moving Transport, CSerializedNetMsg and CNetMessage out of net.cpp. Sv2TemplateProvider does, but very little (mostly miner.h).

    Biggest changes:

    • ceef59cbb029a36bbb9cb3414e8a4c2d4dcf0652 -> 77987c6292171151e574017079b1644dca0d2efd
    • 0764c5531a1d1b4020b97a22bf4449cf32365f37 -> 61cfd61e1472e9bc7019f6c637259ac6ad5821ef
    • 2f7e02d45473e69d5dd8634d9f30e2d5875f7f2f
  44. DrahtBot added the label CI failed on Jan 8, 2024
  45. Sjors force-pushed on Jan 9, 2024
  46. DrahtBot removed the label CI failed on Jan 9, 2024
  47. Sjors force-pushed on Jan 11, 2024
  48. DrahtBot added the label CI failed on Jan 11, 2024
  49. DrahtBot commented at 4:05 pm on January 11, 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/20389970727

  50. Sjors force-pushed on Jan 12, 2024
  51. Sjors commented at 3:38 pm on January 12, 2024: member
    I added support for generating a self-signed certificate. The authority key is printed in the log in base58 format, so it can easily be used on the SRI side.
  52. Sjors force-pushed on Jan 12, 2024
  53. Sjors commented at 4:49 pm on January 12, 2024: member
    I pulled #29032 into this branch to remove the need for branch-switching while testing on a custom signet.
  54. DrahtBot removed the label CI failed on Jan 12, 2024
  55. Sjors force-pushed on Jan 15, 2024
  56. Sjors force-pushed on Jan 15, 2024
  57. DrahtBot added the label CI failed on Jan 15, 2024
  58. DrahtBot commented at 11:55 am on January 15, 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/20490019718

  59. Sjors force-pushed on Jan 15, 2024
  60. conf: add EDCH module 5c84492747
  61. Add ECDH 
    Co-authored-by: Christopher Coverdale <chris.coverdale24@gmail.com>
    7c63b77ed3
  62. Add sv2 log category for Stratum v2 f559599eda
  63. Sjors force-pushed on Jan 15, 2024
  64. Sjors force-pushed on Jan 15, 2024
  65. Sjors commented at 6:49 pm on January 15, 2024: member

    Added a (ephemeral) client id to the log so it’s easier to figure out what’s going on when there’s multiple connections.

    Also cleaned up noise.h

  66. Sjors force-pushed on Jan 15, 2024
  67. DrahtBot removed the label CI failed on Jan 15, 2024
  68. Add sv2 noise protocol 
    Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
    5ac32b702c
  69. move-only: introduce common/transport.h 
    The proposed Stratum v2 integration introduces a new Transport subclass. This commit makes it easier in the future to move this integration to its own process.

net.cpp is currently part of libbitcoin_node. Avoid a dependency on the node by moving the Transport abstract class to common.
    01709bdfd6
  70. move-only: v1 and v2 transport to their own file 
    This helps reduce the size and scope net.h, as well as to make v1 and v2 transport usable outside libbitcoin_node. This is useful for utilities that need a one-shot p2p connection, e.g. to broadcast a transaction or fetch a specific block.

This commit is not necessary for Stratum v2 process separation.
    4f04b94efd
  71. Add sv2 messages 
    Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
    db48064c0a
  72. Introduce Sv2Transport 
    Implemented starting from a copy of V2Transport and the V2TransportTester, modifying it to fit Stratum v2 and Noise Protocol requirements.

Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
Co-Authored-By: Fi3
    e9f78c7fb8
  73. Chainparams: add default sv2 port 
    Co-authored-by: Christopher Coverdale <chris.coverdale24@gmail.com>
    6a6b51f62c
  74. Stratum v2 template provider scaffold 
    Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
    ec34e864d4
  75. Make (Read/Write)BinaryFile work with char vector 
    ReadBinaryFile and WriteBinaryFile current work with std::string. This commit adds support for std::vector<unsigned char>>.

It uses a template and leverages the fact that both std::string and std::vector<unsigned char>> have an insert() method that can take a char array.
    8b55be1c06
  76. Template provider sv2 connection support 
    The template provider will listen for a Job Declarator client.
It can establish a connection and detect various protocol errors.

Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
Co-Authored-By: Fi3
    b6af017dc9
  77. Persist static key for Template Provider d1b4da7a4f
  78. fuzz: sv2 template provider 
    Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
    1cf872d727
  79. Sv2: construct and submit block templates 
    Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
    436583b918
  80. Incrementally update sv2 block template 
    Co-Authored-By: Fi3
    3d4d2cfb0d
  81. signet: miner skips PSBT step for OP_TRUE 5b3ab3e80c
  82. doc: explain Stratum v2 design, testing and usage cfbdf2b1f6
  83. Sjors force-pushed on Jan 16, 2024
  84. DrahtBot added the label CI failed on Jan 21, 2024
  85. DrahtBot commented at 3:50 pm on January 21, 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/20524379427

  86. Increment nonce after encrypt / decrypt 25137482ca
  87. Check maximum nonce value 79c5135708
  88. DrahtBot added the label Needs rebase on Jan 26, 2024
  89. DrahtBot commented at 1:09 pm on January 26, 2024: contributor

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

  90. Sjors commented at 9:20 am on January 29, 2024: member
    (I plan to rebase this after some other stuff is merged)
  91. Sjors commented at 4:15 pm on January 29, 2024: member

    I opened a (draft) pull request for the Noise part of this: #29346

    (will rebase this PR on that later, but right now that would break compatibility)

  92. Sjors commented at 5:29 pm on February 2, 2024: member

    Added two commits to stay in sync with SRI: https://github.com/stratum-mining/stratum/pull/742

    Next week I’ll try to rebase this on the latest #29346, which has diverged a bit.

  93. Sjors commented at 10:23 am on February 5, 2024: member

    I rebased on top of #29346 and pushed the result to: https://github.com/Sjors/bitcoin/tree/2024/02/sv2-poll-ellswift

    Once EllSwift lands on the SRI side (or if I make a revert commit for it) I’ll update this PR.

  94. Sjors commented at 4:11 pm on February 5, 2024: member
    Added to 2024/02/sv2-poll-ellswift: we now hold on to templates for 10 seconds after a new block arrives, in a case a miner was still working on it. The resulting block still wouldn’t get broadcast.
  95. Sjors commented at 6:05 pm on February 8, 2024: member

    I added tests for RequestTransactionData to 2024/02/sv2-poll-ellswift, including an RBF test. It seems that the node does hold on to transaction data after an RBF. I don’t know if that’s because the block template has a p pointer, or if there’s some other mechanism at play.

    We’ll need some way to limit the amount of memory that this can take up. Perhaps by pruning old (lower fee) templates more aggressively.

  96. Sjors commented at 11:35 am on February 14, 2024: member
    This PR is superseeded by #29432.
  97. Sjors closed this on Feb 14, 2024


Sjors DrahtBot hebasto

Labels
Mining Needs rebase CI failed

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-06-29 07:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me