Use Sock in CNode #23604

vasild · 2021-11-26T12:35:39Z

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 · 2021-11-27T05:18:16Z

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

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 · 2021-11-29T08:31:01Z

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 🤦

vasild · 2021-12-01T15:40:06Z

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

Previously invalidated (light) ACK from @theStack

theStack

Light re-ACK 78cda19bb859be69a8bdb006c210b8d47d56acc9

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

vasild · 2021-12-07T09:53:45Z

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

Invalidates (light) ACK from @theStack

jonatack · 2021-12-07T12:58:36Z

Concept ACK. CI failures are unrelated.

jonatack

ACK 0ec56c651ee87d10e7c9b730dcdfffe26b95d88c

src/test/net_tests.cpp

src/test/fuzz/util.h

vasild · 2021-12-09T12:16:11Z

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

Invalidates ACK from @jonatack

Previously invalidated ACK from @theStack

jonatack · 2021-12-09T13:56:46Z

ACK d22a47754208f18d5bd674c28458bb82bbd1b9b9

theStack

Light re-ACK 1477f3aa73c4bec6e04682be63071592a032d506

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

stratospher

ACK 1477f3a.

w0xlt

tACK 1477f3a

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

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

vasild · 2022-01-06T10:07:55Z

1477f3aa73...af7fc2864e: rebase due to conflicts

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

jonatack

ACK af7fc2864e6fb9323fbefc9a9080ba23057143a9 🧦

src/net.cpp

kiminuo · 2022-01-21T22:24:29Z

src/net.cpp

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?

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.

PastaPastaPasta

LGTM, one request

src/net.cpp

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

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

vasild · 2022-01-28T05:49:05Z

af7fc2864e...ef5014d256: address suggestions

Invalidates ACK from @jonatack

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

jonatack · 2022-01-28T13:05:20Z

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

w0xlt

reACK ef5014d

ccdle12 · 2022-01-29T07:03:02Z

src/net.cpp

    m_serializer = std::make_unique<V1TransportSerializer>(V1TransportSerializer());
 }

-CNode::~CNode()


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?

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

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

theStack

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 ef5014d

This was referenced Nov 26, 2021

Make all networking code mockable #21878

Closed

Dedup and RAII-fy the creation of a copy of CConnman::vNodes #21943

Closed

DrahtBot added the P2P label Nov 26, 2021

theStack approved these changes Nov 28, 2021

View reviewed changes

vasild force-pushed the Sock_in_CNode branch from ba8cc5b to 846e995 Compare November 29, 2021 08:29

DrahtBot mentioned this pull request Nov 29, 2021

test: Extend stale_tip_peer_management test #23352

Closed

DrahtBot added the Needs rebase label Dec 1, 2021

vasild force-pushed the Sock_in_CNode branch from 846e995 to 78cda19 Compare December 1, 2021 15:37

DrahtBot removed the Needs rebase label Dec 1, 2021

DrahtBot mentioned this pull request Dec 2, 2021

scripted-diff: Use clang-tidy syntax for C++ named arguments #23545

Closed

theStack approved these changes Dec 2, 2021

View reviewed changes

DrahtBot added the Needs rebase label Dec 6, 2021

vasild force-pushed the Sock_in_CNode branch from 78cda19 to 0ec56c6 Compare December 7, 2021 09:53

DrahtBot removed the Needs rebase label Dec 7, 2021

jonatack reviewed Dec 7, 2021

View reviewed changes

src/test/net_tests.cpp Outdated Show resolved Hide resolved

src/test/fuzz/util.h Outdated Show resolved Hide resolved

vasild force-pushed the Sock_in_CNode branch from 0ec56c6 to d22a477 Compare December 9, 2021 12:14

theStack approved these changes Dec 15, 2021

View reviewed changes

DrahtBot mentioned this pull request Dec 18, 2021

docs: avoid C-style casts; use modern C++ casts #23810

Merged

stratospher reviewed Dec 18, 2021

View reviewed changes

w0xlt approved these changes Dec 27, 2021

View reviewed changes

DrahtBot mentioned this pull request Dec 30, 2021

rpc: p2p_v2 rpc argument for addnode #23900

Closed

DrahtBot added the Needs rebase label Jan 5, 2022

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

c5dd72e

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

vasild force-pushed the Sock_in_CNode branch from 1477f3a to af7fc28 Compare January 6, 2022 10:05

DrahtBot removed the Needs rebase label Jan 6, 2022

jonatack reviewed Jan 6, 2022

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

nina1234 approved these changes Jan 10, 2022

View reviewed changes

kiminuo reviewed Jan 21, 2022

View reviewed changes

DrahtBot mentioned this pull request Jan 26, 2022

p2p, rpc: Manual block-relay-only connections with addnode #24170

Closed

PastaPastaPasta reviewed Jan 27, 2022

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

vasild added 3 commits January 28, 2022 06:41

net: use Sock in CNode

c41a116

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

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

b683491

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

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

ef5014d

vasild force-pushed the Sock_in_CNode branch from af7fc28 to ef5014d Compare January 28, 2022 05:47

w0xlt approved these changes Jan 28, 2022

View reviewed changes

DrahtBot mentioned this pull request Jan 28, 2022

refactor: only use explicit reinterpret/const casts, not implicit #24185

Closed

ccdle12 reviewed Jan 29, 2022

View reviewed changes

PastaPastaPasta approved these changes Jan 31, 2022

View reviewed changes

theStack approved these changes Jan 31, 2022

View reviewed changes

DrahtBot mentioned this pull request Feb 4, 2022

p2p: Split network logging into two categories #24247

Closed

laanwj merged commit e8a3882 into bitcoin:master Feb 4, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 6, 2022

Merge bitcoin#23604: Use Sock in CNode

015cd6e

bitcoin locked and limited conversation to collaborators Feb 4, 2023

Use Sock in CNode #23604

Use Sock in CNode #23604

Uh oh!

Conversation

vasild commented Nov 26, 2021

Uh oh!

DrahtBot commented Nov 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

theStack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasild commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Dec 1, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

vasild commented Dec 7, 2021

Uh oh!

jonatack commented Dec 7, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vasild commented Dec 9, 2021

Uh oh!

jonatack commented Dec 9, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

stratospher left a comment

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

vasild commented Jan 6, 2022

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kiminuo Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

vasild Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild commented Jan 28, 2022

Uh oh!

jonatack commented Jan 28, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

ccdle12 Jan 29, 2022

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Jan 30, 2022

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 27, 2021 •

edited

Loading

theStack left a comment •

edited

Loading

vasild commented Nov 29, 2021 •

edited

Loading