Stratum v2 connman

Sjors commented at 10:50 am on June 25, 2024: member

Based on #30315 and #30205, parent PR #29432.

This PR introduces Sv2Connman which is somewhat similar to CConnman. It uses the Sv2Transport introduced in #30315 to enable incoming connections from other Stratum v2 roles.

It’s main target user is the Template Provider role introduced in #29432.

There may be other Stratum v2 roles we want to support in the future.

A remaining issue is that the code in ThreadSv2Handler reuses a lot of code from CConnman::SocketHandlerConnected. I’d like to find a way to extract this common functionality and put it somewhere else.

TODO:

use mock sockets from #30205
make CConnman::SocketHandlerConnected (partially) reusable

DrahtBot commented at 10:50 am on June 25, 2024: 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:

#30205 (test: add mocked Sock that can read/write custom data and/or CNetMessages by vasild)
#26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)
#26697 (logging: use bitset for categories by LarryRuane)

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 force-pushed on Jun 25, 2024

DrahtBot added the label CI failed on Jun 25, 2024

DrahtBot commented at 11:46 am on June 25, 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/26648048745

DrahtBot removed the label CI failed on Jun 25, 2024

ryanofsky referenced this in commit 2f6dca4d1c on Jun 27, 2024

Sjors force-pushed on Jun 28, 2024

Sjors force-pushed on Jul 1, 2024

DrahtBot commented at 7:14 pm on July 1, 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/26898716492

DrahtBot added the label CI failed on Jul 1, 2024

Sjors force-pushed on Jul 2, 2024

DrahtBot removed the label CI failed on Jul 2, 2024

glozow added the label Mining on Jul 2, 2024

Sjors force-pushed on Jul 2, 2024

DrahtBot added the label CI failed on Jul 2, 2024

DrahtBot commented at 6:37 pm on July 2, 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/26955543098

Sjors force-pushed on Jul 2, 2024

Sjors renamed this:
~~Stratum v2 connectivity~~
Stratum v2 connman
on Jul 2, 2024

Sjors force-pushed on Jul 11, 2024

TheBlueMatt commented at 3:10 pm on July 11, 2024: contributor

This seems like a lot of additional code that is very Sv2-specific. Is there no way to DRY a lot of this code up with connman itself? @pinheadmz may also have some thoughts here as I believe he’s looking at replacing libevent with stuff that uses the common Core socket code anyway.

Sjors commented at 3:22 pm on July 11, 2024: member

DRY a lot of this code up with connman itself?

Yes, I think the key is to make the copy-pasted parts of CConnman::SocketHandlerConnected reusable.

DrahtBot removed the label CI failed on Jul 11, 2024

pinheadmz commented at 6:00 pm on July 12, 2024: member

I am working on a libevent-replacing HTTP server using netbase primitives and yeah, I have some functions that look a lot like the sv2connamn in this branch.

I think it would be cool if possible to abstract the mechanisms in ThreadSocketHandler to work on “abstract clients” and I’d be happy to review and collaborate on that.

Sjors force-pushed on Jul 15, 2024

Sjors commented at 11:41 am on July 15, 2024: member

Removed the accidentally included src/common/sv2_messages.cpp.

in src/common/sv2_connman.cpp:126 in 04e89ba89a outdated

121+                LOCK(m_clients_mutex);
122+                std::unique_ptr transport = std::make_unique<Sv2Transport>(m_static_key, m_certificate.value());
123+                size_t id{m_sv2_clients.size() + 1};
124+                std::unique_ptr client = std::make_unique<Sv2Client>(
125+                    Sv2Client{id, std::move(sock), std::move(transport)}
126+                );

vasild commented at 12:39 pm on July 16, 2024:

make_unique takes the arguments to pass to the constructor which it will invoke internally. Passing it a Sv2Client temporary invokes the copy-constructor. Avoid that.

0                auto client = std::make_unique<Sv2Client>(id, std::move(sock), std::move(transport));

Sjors commented at 6:54 am on July 17, 2024:

I should probably also disable the copy-constructor on Sv2Client?

Sjors force-pushed on Jul 17, 2024

Sjors commented at 7:21 am on July 17, 2024: member

Now using mock sockets #30205, thanks @vasild!

Add sv2 log category for Stratum v2 59eacd21cb

Add sv2 noise protocol

Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>

f69fd623ad

Add sv2 message CoinbaseOutputDataSize

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>

176795aa1f

Convert between Sv2NetMsg and CSerializedNetMsg

This allows us to subclass Transport.

c4f2ef9c74

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

938570a7ac

Add sv2 SETUP_CONNECTION messages

Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>

750f75a227

Add strings for Sv2MsgType fc81013ed0

test: put the generic parts from StaticContentsSock into a separate class

This allows reusing them in other mocked implementations.

Also move the implementation (method definitions) to
`test/util/net.cpp` to make the header `test/util/net.h` easier to follow.

88e7fbdff4

test: add a mocked Sock that allows inspecting what has been Send() to it

And also allows gradually providing the data to be returned by `Recv()`
and sending and receiving net messages (`CNetMessage`).

d1e3e3bdaa

Add Sv2Connman

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

3cf779b0c0

Sjors force-pushed on Jul 19, 2024

Sjors commented at 7:10 pm on July 19, 2024: member

I moved this PR to my own fork at https://github.com/Sjors/bitcoin/pull/50 to reduce CI load, which apparently was becoming too much: #30475 (comment)

Sjors closed this on Jul 19, 2024

maflcko commented at 9:22 am on July 22, 2024: member

reduce CI load

Some more machines were added a few weeks back to deal with increased overall activity, so I think the load should be handleable now, unless someone pushes to 10 different pull requests every 20 minutes in a loop.

Looks like you fixed it in the meantime, but if CI load comes up again, you may create an issue so that it can be fixed.

Stratum v2 connman #30332

Code Coverage

Reviews

Conflicts