Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Stratum v2 connman #50

Open
wants to merge 26 commits into
base: sv2-transport
Choose a base branch
from
Open

Stratum v2 connman #50

wants to merge 26 commits into from

Conversation

Sjors
Copy link
Owner

@Sjors Sjors commented Jul 19, 2024 •
edited
Loading

Based on #67, bitcoin#30205 and bitcoin#30988. Followed by #49.

Parent PR #68

This PR introduces Sv2Connman, a subclass of SockMan (bitcoin#30988). It uses the Sv2Transport introduced in bitcoin#30315 to enable incoming connections from other Stratum v2 roles.

It's main target user is the Template Provider role introduced in #49.

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

Note to self, to keep this rebased until bitcoin#30988 lands:

git rebase --rebase-merges HEAD~8

Earlier version(s): bitcoin#30332

Note that this code will most likely not be upstreamed to Bitcoin Core, but instead used to create a standalone c++ application that connects to a running node via an IPC interface. See bitcoin#31098.

This was referenced Jul 19, 2024
Open
Closed
@Sjors
Copy link
Owner Author

Sjors commented Jul 19, 2024

@pinheadmz wrote in the original PR:

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.

I plan to take a look at this.

This was referenced Jul 19, 2024
Closed
Open
Closed
@Sjors Sjors force-pushed the 2024/06/sv2_transport branch 2 times, most recently from bdc8542 to 199234a Compare August 13, 2024 17:11
@Sjors Sjors mentioned this pull request Aug 26, 2024
Open
@Sjors
Copy link
Owner Author

Sjors commented Aug 29, 2024

CMake rebase.

@Sjors Sjors mentioned this pull request Sep 5, 2024
Open
@Sjors Sjors mentioned this pull request Sep 11, 2024
Closed
@Sjors Sjors force-pushed the 2024/06/sv2_transport branch 3 times, most recently from 8220337 to 5fca2c5 Compare September 19, 2024 14:40
@Sjors
Copy link
Owner Author

Sjors commented Sep 20, 2024

Rebased to move everything into a bitcoin_sv2 library (use -DWITH_SV2=ON to compile).

@vasild
net: isolate P2P specifics from SocketHandlerConnected() and ThreadSo…
50cb524 
…cketHandler()

Move some parts of `CConnman::SocketHandlerConnected()` and
`CConnman::ThreadSocketHandler()` that are specific to the Bitcoin-P2P
protocol to dedicated methods:
`EventIOLoopCompletedForNode()` and `EventIOLoopCompletedForAllPeers()`.

This brings us one step closer to moving `SocketHandlerConnected()` and
`ThreadSocketHandler()` to the protocol agnostic `SockMan` (which would
call `EventIOLoopCompleted...()` from `CConnman`).
@vasild
net: isolate all remaining P2P specifics from SocketHandlerConnected()
96974ff 
Introduce 4 new methods for the interaction between `CConnman` and
`SockMan`:

* `EventReadyToSend()`:
  called when there is readiness to send and do the actual sending of data.

* `EventGotData()`, `EventGotEOF()`, `EventGotPermanentReadError()`:
  called when the corresponing recv events occur.

These methods contain logic that is specific to the Bitcoin-P2P protocol
and move it away from `CConnman::SocketHandlerConnected()` which will
become a protocol agnostic method of `SockMan`.

Also, move the counting of sent bytes to `CConnman::SocketSendData()` -
both callers of that method called `RecordBytesSent()` just after the
call, so move it from the callers to inside
`CConnman::SocketSendData()`.
@vasild
net: split CConnman::ConnectNode()
3bb0145 
Move the protocol agnostic parts of `CConnman::ConnectNode()` into
`SockMan::ConnectAndMakeNodeId()` and leave the Bitcoin-P2P specific
stuff in `CConnman::ConnectNode()`.

Move the protocol agnostic `CConnman::m_unused_i2p_sessions`, its mutex
and `MAX_UNUSED_I2P_SESSIONS_SIZE` to `SockMan`.

Move `GetBindAddress()` from `net.cpp` to `sockman.cpp`.
@vasild
net: tweak EventNewConnectionAccepted()
9d1b352 
Move `MaybeFlipIPv6toCJDNS()`, which is Bitcoin P2P specific from the
callers of `CConnman::EventNewConnectionAccepted()` to inside that
method.

Move the IsSelectable check, the `TCP_NODELAY` option set and the
generation of new node id out of `CConnman::EventNewConnectionAccepted()`
because those are protocol agnostic. Move those to a new method
`SockMan::NewSockAccepted()` which is called instead of
`CConnman::EventNewConnectionAccepted()`.
@vasild
net: move sockets from CNode to SockMan
c133a63 
Move `CNode::m_sock` and `CNode::m_i2p_sam_session` to `SockMan::m_connected`.
Also move all the code that handles sockets to `SockMan`.

`CNode::CloseSocketDisconnect()` becomes
`CConnman::MarkAsDisconnectAndCloseConnection()`.

`CConnman::SocketSendData()` is renamed to
`CConnman::SendMessagesAsBytes()` and its sockets-touching bits are moved to
`SockMan::SendBytes()`.

`CConnman::GenerateWaitSockets()` goes to
`SockMan::GenerateWaitSockets()`.

`CConnman::ThreadSocketHandler()` and
`CConnman::SocketHandler()` are combined into
`SockMan::ThreadSocketHandler()`.

`CConnman::SocketHandlerConnected()` goes to
`SockMan::SocketHandlerConnected()`.

`CConnman::SocketHandlerListening()` goes to
`SockMan::SocketHandlerListening()`.
@vasild
net: move-only: improve encapsulation of SockMan
70c2f13 
`SockMan` members

`AcceptConnection()`
`NewSockAccepted()`
`GetNewNodeId()`
`m_i2p_sam_session`
`m_listen private`

are now used only by `SockMan`, thus make them private.
@Sjors
Copy link
Owner Author

Sjors commented Oct 4, 2024 •
edited
Loading

I turned Sv2Connman into a subclass of SockMan using bitcoin#30988. This gets rid of most of the copy-paste stuff.

Two open questions:

  1. Does having m_disconnect_flag still make sense, given that I call CloseConnection everywhere?
  2. Should EventReadyToSend use the more flag?

MSAN is unhappy:

[15:40:15.757]   Uninitialized value was created by an allocation of 'storage' in the stack frame
[15:40:15.757]     #0 0x55dc46e92b31 in SockMan::AcceptConnection(Sock const&, CService&) ci/scratch/build-x86_64-pc-linux-gnu/src/./src/common/sockman.cpp:351:5
[15:40:15.757] 
[15:40:15.757] SUMMARY: MemorySanitizer: use-of-uninitialized-value ci/scratch/build-x86_64-pc-linux-gnu/src/./src/netaddress.cpp:812:5 in CService::SetSockAddr(sockaddr const*)

Maybe a4d8293 makes it happy?

@Sjors Sjors mentioned this pull request Oct 4, 2024
Closed
@Sjors Sjors force-pushed the 2024/06/sv2_connection branch 3 times, most recently from 476161f to 96f953f Compare October 4, 2024 18:13
@Sjors
Copy link
Owner Author

Sjors commented Oct 4, 2024 •
edited
Loading

MSAN is happy. The last push just adds the dependency on bitcoin_common, which on macOS isn't needed, but the Linux Guix build is more picky.

@Sjors Sjors mentioned this pull request Oct 16, 2024
Open
Sjors and others added 10 commits October 16, 2024 12:12
@Sjors @ccdle12
Add sv2 SETUP_CONNECTION messages
c211a16 
Co-Authored-By: Christopher Coverdale <[email protected]>
@Sjors
Add strings for Sv2MsgType
818e124
@vasild @Sjors
test: move the implementation of StaticContentsSock to .cpp
c085339 
Move the implementation (method definitions) from `test/util/net.h` to
`test/util/net.cpp` to make the header easier to follow.
@vasild @Sjors
test: put the generic parts from StaticContentsSock into a separate c…
ae25a75 
…lass

This allows reusing them in other mocked implementations.
@vasild @Sjors
test: add a mocked Sock that allows inspecting what has been Send() t…
e7733c0 
…o it

And also allows gradually providing the data to be returned by `Recv()`
and sending and receiving net messages (`CNetMessage`).
@Sjors
Merge remote-tracking branch 'vasild/sockman' into 2024/06/sv2_connec…
9ee662c 
…tion
@Sjors
Move i2p from node to common
558c9e0
@Sjors
init storage to make MSAN happy
28d0915
@Sjors @vasild
Add Sv2Connman
a1f543b 
Co-Authored-By: Vasil Dimov <[email protected]>
@Sjors
tmp: fix silent merge conflict with bitcoin#30937
e8d8aea
@Sjors Sjors changed the base branch from 2024/06/sv2_transport to sv2-transport October 16, 2024 11:59
This was referenced Oct 16, 2024
Open
Open
pinheadmz
pinheadmz reviewed Oct 30, 2024
View reviewed changes
src/test/sv2_connman_tests.cpp

m_connman = std::make_unique<Sv2Connman>(TP_SUBPROTOCOL, static_key, m_connman_authority_pubkey, certificate);

BOOST_REQUIRE(m_connman->Start(this, "127.0.0.1", 18447));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are you able to Start() the connman but never break out of ThreadSocketHandler() with interruptNet()? If I leave the thread running in my HTTP unit test I get an error when the SockMan instance is destroyed.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea :-)

cc @vasild?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The destructor of Sv2Connman calls the Interrupt() method which signals the interruptNet variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pinheadmz pinheadmz pinheadmz left review comments

@vasild vasild vasild left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Sjors @vasild @pinheadmz