Stratum v2 Template Provider (take 2)

[Sjors]

Based on on @Fi3's 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?

• CKey: add Serialize and Unserialize #29295
• Custom signet mining improvement: signet: fixing mining for OP_TRUE challenge #29032
• Make nested pull-requests easier: Support self-hosted Cirrus workers on forks #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

• use EllSwift RFC: Use EllSwift for ECDH stratum-mining/sv2-spec#66, implemented in Stratum v2 handshake using EllSwift Sjors/bitcoin#28
• certificate signing issue: Incorrect certificate validation stratum-mining/stratum#717, fix in Fix certificate generation and validation Sjors/bitcoin#29

Networking

• add -sv2bind and -sv2allowip
• persist static key (needs Make (Read/Write)BinaryFile work with char vector, use AutoFile #29229)
• optional -sv2cert
• create Sv2Transport subclass similar to V2Transport: Introduce Sv2Transport Sjors/bitcoin#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 SRI uses incorrect encoding of secp256k1 public keys stratum-mining/stratum#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 JDC panics after receiving RequestTransactionDataError message stratum-mining/stratum#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

• move / refactor GetMerklePathForCoinbase helper (see Future Work in original PR, and Fix NewTemplate.merkle_path in bitcoind stratum-mining/stratum#567)
• truncate long trace messages

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, Implement a clever way to create and manage future jobs  stratum-mining/stratum#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)

[DrahtBot]

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.

[Sjors]

Rebased and fixed encoding mistake I made during rebase:

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

Yes, the coinbase transaction has a witness.

[Sjors]

I moved sv2_noise.h and sv2_messages.h to common and template_provider.h to node.

[Sjors]

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.

[Sjors]

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.

[Sjors]

Ran clang-tidy locally to (hopefully) fix its remaining complaints. Included @Fi3's Sjors#25 (plus additional header and test change) which drops the CIPHER_CONFIRMATION state, as per stratum-mining/sv2-spec#60.

[Sjors]

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 ?

class Sv2CipherState
{
public:
    Sv2CipherState() = default;
    ...
    ssize_t WriteMsg(Span<std::byte> msg);

[DrahtBot]

🚧 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}

[DrahtBot]

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

[Sjors]

(I plan to rebase this after some other stuff is merged)

[Sjors]

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)

[Sjors]

Added two commits to stay in sync with SRI: stratum-mining/stratum#742

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

[Sjors]

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.

[Sjors]

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.

[Sjors]

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.

[Sjors]

This PR is superseeded by #29432.