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.

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.

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

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.

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.

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.

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);

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

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

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

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)

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.

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.

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.

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.

This PR is superseeded by #29432.