Based on #29346. Followed by https://github.com/Sjors/bitcoin/pull/50. Parent PR #29432.
Introduces Sv2Transport::Transport which is very similar to V2Transport.
This shoehorns Sv2NetMsg into a CSerializedNetMsg in SetMessageToSend, and into a CNetMessage in GetReceivedMessage.
See discussion in #30209.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process. A summary of reviews will appear here.
Reviewers, 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.
libbitcoin_net library? Either at the same level as libbitcoin_common or on top of it. I could see how other node implementations could benefit from that, but it’s also a way to limit libbitcoin_common in size. Afaik only libbitcoin_node needs networking (and zmq?), and a future libbitcoin_sv2, and maybe libbitcoin_rpc.
🚧 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.
It compiles and the tests run, but it’s not pretty.
The conversion between Sv2NetMsg and CSerializedNetMsg / CNetMessage is done in https://github.com/bitcoin/bitcoin/pull/30315/commits/7d937674de425d8e2525bac9e719ac93a775ea45, which is a bit of a hack at the moment.
I also haven’t put much thought into Sv2NetMsg ever since I took over #27854, so perhaps this design can be improved as well.
Sv2NetMsg did not contain an Sv2NetHeader, and just stored type and message. The Sv2 Transport would then construct the header at submitting or sending time instead. This means Sv2NetMsg would be more of a dumb container for what higher-level code cares about, while the protocol details would be more abstracted away in the Transport.
9+#include <span.h>
10+#include <streams.h>
11+#include <string>
12+#include <util/check.h>
13+
14+namespace node {
namespace is outdated, since this isn’t part of the node anymore.
242+
243+ // Payload
244+ Span<const std::byte> payload_plain = MakeByteSpan(msg.data());
245+ // TODO: truncate very long messages, about 100 bytes at the start and end
246+ // is probably enough for most debugging.
247+ LogPrintLevel(BCLog::SV2, BCLog::Level::Trace, "Payload: %s\n", HexStr(payload_plain));
MakeByteSpan(msg), not MakeByteSpan(msg.data()).
🚧 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.
libbitcoin_common. See https://github.com/Sjors/bitcoin/pull/47 for a more ambitious attempt to introduce a whole new libbitcoin_net, but that became real big and distracting real fast :-)
🚧 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.
CNetMessage, CSerializedNetMsg and Transport to common/transport.h. To keep the diff small it doesn’t move the implementation; that could be done in a separate PR. This avoids a circular dependency between bitcoin-node and bitcoin-sv2.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30422811066
Make sure to run all tests locally, according to the documentation.
The failure may happen due to a number of reasons, for example:
Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
This commit adds the simplest stratum v2 message. The remaining messages are introduced in later commits.
Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
This avoids a circular dependency between bitcoin-sv2 and bitcoin-node.
This allows us to subclass Transport.
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
🐙 This pull request conflicts with the target branch and needs rebase.
Labels
Needs rebase