Use Sock in CNode #23604

vasild commented at 12:35 PM on November 26, 2021: member

This is a piece of #21878, chopped off to ease review.

Change CNode to use a pointer to Sock instead of a bare SOCKET. This will help mocking / testing / fuzzing more code.

DrahtBot added the label P2P on Nov 26, 2021

DrahtBot commented at 5:18 AM on November 27, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24247 (p2p: Split network logging into two categories by anshu-khare-design)
#24185 (refactor: only use explicit reinterpret/const casts, not implicit by PastaPastaPasta)
#24170 (p2p, rpc: Manual block-relay-only connections with addnode by mzumsande)
#23900 (rpc: p2p_v2 rpc argument for addnode by dhruv)
#23545 (scripted-diff: Use clang-tidy syntax for C++ named arguments by MarcoFalke)
#22778 (net processing: Reduce resource usage for inbound block-relay-only connections by jnewbery)
#21160 (net/net processing: Move tx inventory into net_processing by jnewbery)
#17167 (Allow whitelisting outgoing connections by luke-jr)

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.

theStack approved

theStack commented at 6:12 PM on November 28, 2021: member

Concept and light code-review ACK ba8cc5b4e5c38e163d01b38f8a73da7c10ce0999

The first commit seems unrelated to the other two, I guess this will be useful in a later chopped off PR?

vasild force-pushed on Nov 29, 2021

vasild commented at 8:31 AM on November 29, 2021: member

ba8cc5b4e5...846e9952cf: use FuzzedSock in ConsumeNode()

Invalidates (light) ACK from @theStack

The first commit seems unrelated to the other two...

Oh, good catch! The idea is to use FuzzedSock in ConsumeNode() which creates a CNode object. Both are defined in src/test/fuzz/util.h, but for this FuzzedSock must appear before ConsumeNode(). It was like this in a previous incarnation of #21878 but in the latest push, for some reason, I changed it to use nullptr 🤦

DrahtBot added the label Needs rebase on Dec 1, 2021

vasild force-pushed on Dec 1, 2021

vasild commented at 3:40 PM on December 1, 2021: member

846e9952cf...78cda19bb8: rebase due to conflicts

Previously invalidated (light) ACK from @theStack

DrahtBot removed the label Needs rebase on Dec 1, 2021

theStack approved

theStack commented at 3:39 PM on December 2, 2021: member

Light re-ACK 78cda19bb859be69a8bdb006c210b8d47d56acc9

(as per git range-diff ba8cc5b4...78cda19b)

DrahtBot added the label Needs rebase on Dec 6, 2021

vasild force-pushed on Dec 7, 2021

vasild commented at 9:53 AM on December 7, 2021: member

78cda19bb8...0ec56c651e: rebase due to conflicts

Invalidates (light) ACK from @theStack

DrahtBot removed the label Needs rebase on Dec 7, 2021

jonatack commented at 12:58 PM on December 7, 2021: member

Concept ACK. CI failures are unrelated.

in src/test/net_tests.cpp:611 in 0ec56c651e outdated

 605 | @@ -607,7 +606,16 @@ BOOST_AUTO_TEST_CASE(ipv4_peer_with_ipv6_addrMe_test)
 606 |      in_addr ipv4AddrPeer;
 607 |      ipv4AddrPeer.s_addr = 0xa0b0c001;
 608 |      CAddress addr = CAddress(CService(ipv4AddrPeer, 7777), NODE_NETWORK);
 609 | -    std::unique_ptr<CNode> pnode = std::make_unique<CNode>(0, NODE_NETWORK, INVALID_SOCKET, addr, /*nKeyedNetGroupIn=*/0, /*nLocalHostNonceIn=*/0, CAddress{}, /*pszDest=*/std::string{}, ConnectionType::OUTBOUND_FULL_RELAY, /*inbound_onion=*/false);
 610 | +    std::unique_ptr<CNode> pnode = std::make_unique<CNode>(0,
 611 | +                                                           NODE_NETWORK,
 612 | +                                                           nullptr,

jonatack commented at 5:07 PM on December 7, 2021:

c702f77a in net_tests and denialofservice_tests named args might be nice for these nullptr params

                                                           /*sock=*/nullptr,

vasild commented at 12:16 PM on December 9, 2021:

Done (all over the place)

in src/test/fuzz/util.h:300 in 0ec56c651e outdated

 296 | @@ -259,7 +297,7 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
 297 |  {
 298 |      const NodeId node_id = node_id_in.value_or(fuzzed_data_provider.ConsumeIntegral<NodeId>());
 299 |      const ServiceFlags local_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
 300 | -    const SOCKET socket = INVALID_SOCKET;
 301 | +    auto sock = std::make_shared<FuzzedSock>(fuzzed_data_provider);

jonatack commented at 5:09 PM on December 7, 2021:

c702f77a1f5e5988a0c7cee7ef3f7cfc63a9ea1b feel free to ignore: not sure if there is a reason to not keep it const (and use braced initialization)

    const auto sock{std::make_shared<FuzzedSock>(fuzzed_data_provider)};

vasild commented at 12:19 PM on December 9, 2021:

Yes, it can be const, this is the std::shared_ptr itself, not the contained FuzzedSock.

Braced initialization is pointless with auto since there is nothing to check against. I find braced initialization less readable than = so I left it as it is.

jonatack commented at 5:18 PM on December 7, 2021: member

ACK 0ec56c651ee87d10e7c9b730dcdfffe26b95d88c

vasild force-pushed on Dec 9, 2021

vasild commented at 12:16 PM on December 9, 2021: member

0ec56c651e...d22a477542: address suggestions (only stylistic changes)

Invalidates ACK from @jonatack

Previously invalidated ACK from @theStack

jonatack commented at 1:56 PM on December 9, 2021: member

ACK d22a47754208f18d5bd674c28458bb82bbd1b9b9

stratospher commented at 7:05 PM on December 9, 2021: contributor

ACK d22a477

laanwj commented at 1:32 PM on December 13, 2021: member

Concept ACK. Just a question: using a shared pointer instead of a unique pointer muddles the concept of ownership a bit (e.g. it makes it harder to reason about the lifecycle). Do we need one?

vasild commented at 2:26 PM on December 13, 2021: member

using a shared pointer instead of a unique pointer muddles the concept of ownership a bit ...

Oh well, this is a bit subtle, maybe some comments are warranted 🤔. It is needed to resolve #21744. That issue contains some details on "why shared_ptr".

DrahtBot added the label Needs rebase on Dec 14, 2021

vasild force-pushed on Dec 15, 2021

vasild commented at 12:46 PM on December 15, 2021: member

d22a477542...1477f3aa73: rebase due to conflicts, elaborate the comment for CNode::m_sock

Invalidates ACKs from @jonatack, @stratospher

Previously invalidated ACK from @theStack

vasild commented at 12:49 PM on December 15, 2021: member

@laanwj, added this comment in the hopes to make it more clear:

`shared_ptr` (instead of `unique_ptr`) is used to avoid premature close of
the underlying file descriptor by one thread while another thread is
poll(2)-ing it for activity.
[@see](/bitcoin-bitcoin/contributor/see/) [#21744](/bitcoin-bitcoin/21744/) for details.

DrahtBot removed the label Needs rebase on Dec 15, 2021

jonatack commented at 3:19 PM on December 15, 2021: member

ACK 1477f3aa73c4bec6e04682be63071592a032d506

theStack approved

theStack commented at 4:13 PM on December 15, 2021: member

Light re-ACK 1477f3aa73c4bec6e04682be63071592a032d506

(as per git range-diff 78cda19b...1477f3aa)

stratospher commented at 5:00 PM on December 18, 2021: contributor

ACK 1477f3a.

w0xlt approved

w0xlt commented at 4:53 AM on December 27, 2021: contributor

tACK 1477f3a

Great explanation of why use shared_ptr instead of std::unique_ptr for m_sock

DrahtBot added the label Needs rebase on Jan 5, 2022

fuzz: move FuzzedSock earlier in src/test/fuzz/util.h

This way it can be used in `ConsumeNode()`.

c5dd72e146

vasild force-pushed on Jan 6, 2022

vasild commented at 10:07 AM on January 6, 2022: member

1477f3aa73...af7fc2864e: rebase due to conflicts

Invalidates ACKs from @jonatack, @theStack, @stratospher, @w0xlt

DrahtBot removed the label Needs rebase on Jan 6, 2022

in src/net.cpp:508 in af7fc2864e outdated

 504 | @@ -505,7 +505,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
 505 |      if (!addr_bind.IsValid()) {
 506 |          addr_bind = GetBindAddress(sock->Get());
 507 |      }
 508 | -    CNode* pnode = new CNode(id, nLocalServices, sock->Release(), addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", conn_type, /* inbound_onion */ false);
 509 | +    CNode* pnode = new CNode(id, nLocalServices, std::move(sock), addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", conn_type, /* inbound_onion */ false);

jonatack commented at 4:08 PM on January 6, 2022:

nit if you retouch

    CNode* pnode = new CNode(id, nLocalServices, std::move(sock), addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", conn_type, /*inbound_onion=*/false);

vasild commented at 5:42 AM on January 28, 2022:

Done (in a separate commit to avoid combining style changes and logical changes in one commit).

jonatack commented at 1:03 PM on January 28, 2022:

I find the line wrapping style changes more difficult to read than before and the trivial named argument fixup could be done while touching the line in net: use Sock in CNode but ok.

in src/net.cpp:1200 in af7fc2864e outdated

1196 | @@ -1197,7 +1197,7 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
1197 |      }
1198 |  
1199 |      const bool inbound_onion = std::find(m_onion_binds.begin(), m_onion_binds.end(), addr_bind) != m_onion_binds.end();
1200 | -    CNode* pnode = new CNode(id, nodeServices, sock->Release(), addr, CalculateKeyedNetGroup(addr), nonce, addr_bind, "", ConnectionType::INBOUND, inbound_onion);
1201 | +    CNode* pnode = new CNode(id, nodeServices, std::move(sock), addr, CalculateKeyedNetGroup(addr), nonce, addr_bind, "", ConnectionType::INBOUND, inbound_onion);

jonatack commented at 4:10 PM on January 6, 2022:

nit if you retouch this line

    CNode* pnode = new CNode(id, nodeServices, std::move(sock), addr, CalculateKeyedNetGroup(addr), nonce, addr_bind, /*addrNameIn=*/"", ConnectionType::INBOUND, inbound_onion);

vasild commented at 5:42 AM on January 28, 2022:

Done (in a separate commit to avoid combining style changes and logical changes in one commit).

jonatack commented at 1:03 PM on January 28, 2022:

I find the line wrapping style changes more difficult to read than before and the trivial named argument fixup could be done while touching the line in net: use Sock in CNode but ok.

jonatack commented at 4:12 PM on January 6, 2022: member

ACK af7fc2864e6fb9323fbefc9a9080ba23057143a9 :socks:

nina1234 approved

laanwj added this to the "Blockers" column in a project

in src/net.cpp:814 in af7fc2864e outdated

 803 | +            LOCK(node.m_sock_mutex);
 804 | +            if (!node.m_sock) {
 805 |                  break;
 806 | -            nBytes = send(node.hSocket, reinterpret_cast<const char*>(data.data()) + node.nSendOffset, data.size() - node.nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT);
 807 | +            }
 808 | +            nBytes = node.m_sock->Send(reinterpret_cast<const char*>(data.data()) + node.nSendOffset, data.size() - node.nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT);

kiminuo commented at 10:24 PM on January 21, 2022:

Probably a silly question: LOCK(node.m_sock_mutex); applies even to node.m_sock->Send(...) operation but then here one can return contained sockets out of the function.

I understand that m_sock_mutex guards only m_sock member read & write operations but not necessarily the contained socket. Is that correct or not?

vasild commented at 1:44 PM on January 27, 2022:

That is a good question.

Before this PR the mutex protects the value of the socket variable - that is, to make sure it does not change from e.g. 7 (it is a file descriptor) to INVALID_SOCKET in the middle of constructs like:

if (socket != INVALID_SOCKET) {
    ... do something with the socket, assuming it is valid ...
}

After this PR, it is the same with the exception that the variable is not a bare integer anymore, but a shared_ptr and an empty such pointer is used to designate what was designated before by INVALID_SOCKET.

in src/net.cpp:3000 in af7fc2864e outdated

2996 | @@ -2994,10 +2997,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const
2997 |      m_serializer = std::make_unique<V1TransportSerializer>(V1TransportSerializer());
2998 |  }
2999 |  
3000 | -CNode::~CNode()
3001 | -{
3002 | -    CloseSocket(hSocket);
3003 | -}
3004 | +CNode::~CNode() {}

PastaPastaPasta commented at 8:36 AM on January 27, 2022:

pls move to header and use = default (or document specific reason why it must be in cpp)

vasild commented at 5:47 AM on January 28, 2022:

I Just removed the destructor, it is not necessary now.

PastaPastaPasta commented at 8:38 AM on January 27, 2022: contributor

LGTM, one request

net: use Sock in CNode

Change `CNode` to use a pointer to `Sock` instead of a bare `SOCKET`.
This will help mocking / testing / fuzzing more code.

c41a1162ac

scripted-diff: rename CNode::cs_hSocket to CNode::m_sock_mutex

-BEGIN VERIFY SCRIPT-
sed -i -e 's/cs_hSocket/m_sock_mutex/g' $(git grep -l cs_hSocket)
-END VERIFY SCRIPT-

b683491648

style: wrap long lines in CNode creation and add some comments ef5014d256

vasild force-pushed on Jan 28, 2022

vasild commented at 5:49 AM on January 28, 2022: member

af7fc2864e...ef5014d256: address suggestions

Invalidates ACK from @jonatack

Previously invalidated ACKs from @theStack, @stratospher, @w0xlt

jonatack commented at 1:05 PM on January 28, 2022: member

re-ACK ef5014d256638735b292672c774446db4003f03b changes since last review are the removal of an unneeded dtor and the addition of a style commit

w0xlt approved

w0xlt commented at 3:55 PM on January 28, 2022: contributor

reACK ef5014d

in src/net.cpp:2997 in c41a1162ac outdated

2996 | @@ -2994,11 +2997,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const
2997 |      m_serializer = std::make_unique<V1TransportSerializer>(V1TransportSerializer());
2998 |  }
2999 |  
3000 | -CNode::~CNode()

ccdle12 commented at 7:03 AM on January 29, 2022:

Apologies if this is a silly question but just wanted to clarify - removing this destructor, it's implicit that the shared_ptr for Sock will be reset and the Sock's destructor will responsible for closing the socket?

PastaPastaPasta commented at 4:02 AM on January 30, 2022:

See https://en.cppreference.com/w/cpp/language/destructor "Implicitly-declared destructor".

The default destructor for CNode will call all the destructors of all of it's members. The destructor for shared_ptr will reduce the reference counter, and then if that reference counter is zero, the destructor for Sock will be called. If the RC is not zero, then the Sock will continue to exist on the heap (presumably being used by another thread), until the owner of that shared_ptr calls the destructor.

PastaPastaPasta approved

PastaPastaPasta commented at 9:04 AM on January 31, 2022: contributor

utACK ef5014d256638735b292672c774446db4003f03b, I have reviewed the code, and believe it makes sense to merge

theStack approved

theStack commented at 7:15 PM on January 31, 2022: member

Since my last ACK was only "light" and happened weeks ago, so I decided to do a full review of this PR again instead of looking at the range-diff. Changes LGTM!

Cod-review ACK ef5014d256638735b292672c774446db4003f03b

laanwj merged this on Feb 4, 2022

laanwj closed this on Feb 4, 2022

sidhujag referenced this in commit 015cd6e654 on Feb 6, 2022

laanwj removed this from the "Blockers" column in a project

DrahtBot locked this on Feb 4, 2023