Stratum v2 Template Provider (take 3) #29432
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
87f0527 to
684890f
Compare
| { | ||
| bool started = m_tp->Start(Sv2TemplateProviderOptions { .port = 18447 }); | ||
| if (! started) return false; | ||
| // Avoid "Connection refused" on CI: |
There was a problem hiding this comment.
The template provider tests are quite brittle because they use a real socket.
There was a problem hiding this comment.
For the time being I just added handling for MSG_MORE (on e.g. macOS sequential messages are sent separately while on Linux they're combined). I also made the timeouts a bit longer.
Hopefully that does the trick. This can be revisited closer to the time when the Template Provider is ready for its own PR.
There was a problem hiding this comment.
Except the macOS native CI... https://github.com/Sjors/bitcoin/actions/runs/7905485495/job/21578223892?pr=34#step:7:6060
There was a problem hiding this comment.
Working on a fix in Sjors#34
There was a problem hiding this comment.
I should probably look into using StaticContentsSock
There was a problem hiding this comment.
@vasild any thoughts on how to make mock Socks that can be used to play messages in two directions?
There was a problem hiding this comment.
Yes! See the first two commits in #26812:
bee6bdf test: put the generic parts from StaticContentsSock into a separate class
f42e4f3 test: add a mocked Sock that allows inspecting what has been Send() to it
and then how to use that in the last commit of the same PR:
8b10990 test: add unit tests exercising full call chain of CConnman and PeerManager
With those it is possible to send/receive raw bytes to/from the (mocked) socket, or NetMsgs, e.g.:
pipes->recv.PushNetMsg(NetMsgType::GETBLOCKS, block_locator, hash_stop);| ss << TX_WITH_WITNESS(tx); | ||
| tx_size = ss.size(); | ||
| } | ||
|
|
There was a problem hiding this comment.
TSAN is tripping up somewhere around here. The last thing it logs is - Connect 2 transactions:. It doesn't get to - Verify ... txins:. I wonder if this is related to mock time, which I'm testing in Sjors#34
There was a problem hiding this comment.
@maflcko shouldn't TSan on CI output something useful about why it crashed? I currently only says "error 2": https://cirrus-ci.com/task/5124733717446656?logs=ci#L3531
When running this locally on Ubuntu with clang 16.0.6 I get a WARNING: ThreadSanitizer: data race and significantly more details (still a bit cryptic, but hopefully enough to figure out what's happening).
There was a problem hiding this comment.
Ah, I guess the unit tests don't capture the tsan output?
There was a problem hiding this comment.
But they should. At least back when I tested #27667
There was a problem hiding this comment.
Maybe good to re-check this when/after the cmake migration is done?
90735e6 to
1e06c04
Compare
|
Added |
|
Bumping macOS to 14 on the CI does not help (tried in Sjors#35). I also can't reproduce this failure on my own Intel macOS machines, not on 13.6.4 and not on 14.2.1. A This extra delay seems to do the trick for now: Sjors@c8d10af Another option to consider is using the functional test framework instead, since these are not really unit tests. However that involves implementing the sv2 noise protocol in Python and a bunch of other work to export transport functions to the functional test framework. If anyone feels up to that challenge, let me know... |
Move the implementation (method definitions) from `test/util/net.h` to `test/util/net.cpp` to make the header easier to follow.
…lass This allows reusing them in other mocked implementations.
…o it And also allows gradually providing the data to be returned by `Recv()` and sending and receiving net messages (`CNetMessage`).
Co-Authored-By: Vasil Dimov <vd@FreeBSD.org>
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
Co-authored-by: Christopher Coverdale <chris.coverdale24@gmail.com>
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
This doesn't solve the underlying problem.
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. Compared to #28983 it introduces EllSwift in the handshake and fixes various bugs. I used that opportunity to change the branch name, which makes testing against SRI slightly easier. There's no conceptual discussion on #28983 so it can be ignored by reviewers.
See docs/stratum-v2.md for a brief description of Stratum v2 and the role of Bitcoin Core in that system..
There's roughly four layers:
What to test and review?
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 split this PR into separate pull requests for easier review. Feedback on each of them is welcome, even if they're stacked on another one.
Even if we abandon the approach here in favor of a sidecar (see experimental below), it is useful to review the above changes. They are fully reusable as demonstrated in Sjors#48. And until we actually ship something else, the above is being used by people, including on (very limited) mainnet.
The pull requests below enable a sidecar approach:
These are needed with any approach:
Exploratory / experimental:
Related useful:
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.
Things left todo
Networking
-sv2allowip-sv2certTemplate generation and updating
coinbase_tx_additional_output_sizeMisc
Potential followups
test/sv2_template_provider_tests.cpp)