net: Add CNodeOptions and increase constness #25962

ajtowns · 2022-08-31T08:17:36Z

Adds CNodeOptions to make it easier to add optional parameters to the CNode constructor, and makes prefer_evict and m_permissionFlags actually const.

src/net.h

vasild

Concept ACK

Why not put all arguments in CNodeOptions? It seems a bit confusing to have some arguments passed directly and some via CNodeOptions. Also, if both are used, then it is unclear which way should be used by a newly added argument in the future.

hebasto · 2022-08-31T09:15:56Z

Concept ACK.

ajtowns · 2022-08-31T19:02:33Z

Why not put all arguments in CNodeOptions?

It's only the optional arguments that are in there, which I think is a pretty reasonable dividing line, and also limits the code changes needed.

DrahtBot · 2022-08-31T19:23:28Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25923 (p2p: always provide CNodeStateStats and getpeerinfo/netinfo/gui updates by jonatack)

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.

vasild

It's only the optional arguments that are in there, which I think is a pretty reasonable dividing line, and also limits the code changes needed.

I see. Maybe it makes sense to put all arguments in CNodeOptions, but even as it is this PR seems like an improvement over master. Would be happy to review also if all arguments are passed via CNodeOptions.

src/net.cpp

vasild · 2022-09-01T06:39:09Z

src/net.h

@@ -334,6 +334,13 @@ class V1TransportSerializer : public TransportSerializer {
    void prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) const override;
 };

+struct CNodeOptions


nit: (feel free to ignore) If this is defined inside CNode, then callers would use CNode::Options instead of CNodeOptions and inside the class it would be just Options instead of CNodeOptions. Also, if/when CNode is renamed to something else, it would not be necessary to rename CNodeOptions too.

Unfortunately there's a gcc/clang bug that means this doesn't work right:

class X { public: struct Y { int foo = 42; }; int SomeFunc(Y y = {}); };

gives:

$ g++ -std=c++17 -Wall -W -c -o lame.o lame.cpp lame.cpp:11:25: error: could not convert ‘<brace-enclosed initializer list>()’ from ‘<brace-enclosed initializer list>’ to ‘X::Y’ 11 | int SomeFunc(Y y = {}); | ^ | | | <brace-enclosed initializer list> $ clang -std=c++17 -Wall -W -c -o lame.o lame.cpp lame.cpp:11:25: error: default member initializer for 'foo' needed within definition of enclosing class 'X' outside of member functions int SomeFunc(Y y = {}); ^ lame.cpp:7:13: note: default member initializer declared here int foo = 42; ^ 1 error generated.

Moving Y outside of X fixes this. https://stackoverflow.com/a/53423881/ has more details.

I see. Omitting = {} from SomeFunc() definition seems to resolve this, but then callers need to pass {} at the end if they don't wish to pass optional arguments. Or if all arguments are passed via the Options variable, then it will be ok.

naumenkogs · 2022-09-01T07:18:32Z

Concept ACK. Will do code review once the pending feedback is resolved.

vasild

ACK 2f3602b

vasild · 2022-09-01T09:44:49Z

src/test/fuzz/util.h

@@ -310,7 +311,8 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
                                       addr_bind,
                                       addr_name,
                                       conn_type,
-                                       inbound_onion);
+                                       inbound_onion,
+                                       CNodeOptions{ .permission_flags = permission_flags });


nit: here and below, could omit CNodeOptions, like done in net.cpp:

Suggested change

CNodeOptions{ .permission_flags = permission_flags });

{ .permission_flags = permission_flags });

Or maybe not: CI is upset:

./net.h:523:5: note: no known conversion for argument 10 from ‘<brace-enclosed initializer list>’ to ‘CNodeOptions&&’

Can't omit it here because std::make_unique<CNode>(...) can't resolve to a specific function without knowing what type { .permission_flags = .. } is going to be. (Could omit it in the else clause, but figured it was better to have those two clauses be as similar as possible)

Looks like you can't omit it when using .foo = std::move(unique_ptr) either, as gcc 8 gives errors. https://godbolt.org/z/rbn6csbax

Can't omit it here because std::make_unique<CNode>(...) can't resolve to a specific function without knowing what type { .permission_flags = .. } is going to be.

This is ok, but an alternative is to switch from make_unique<CNode> to new CNode

jonatack · 2022-09-01T10:44:23Z

Review ACK modulo the CI issue

-BEGIN VERIFY SCRIPT- sed -i 's/permissionFlags/permission_flags/g' $(git grep -l permissionFlags) -END VERIFY SCRIPT-

jonatack

ACK 377e9cc per git range-diff 52dcb1d 2f3602b 377e9cc

Some clang-format nits if you repush

diff --git a/src/net.cpp b/src/net.cpp
index 6659b64246..138486e468 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -556,7 +556,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
                              pszDest ? pszDest : "",
                              conn_type,
                              /*inbound_onion=*/false,
-                             CNodeOptions{ .i2p_sam_session = std::move(i2p_transient_session) });
+                             CNodeOptions{.i2p_sam_session = std::move(i2p_transient_session)});
     pnode->AddRef();

@@ -1029,8 +1027,8 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
                              ConnectionType::INBOUND,
                              inbound_onion,
                              CNodeOptions{
-                               .permission_flags = permission_flags,
-                               .prefer_evict = discouraged,
+                                 .permission_flags = permission_flags,
+                                 .prefer_evict = discouraged,
                              });
diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h
index 6d652c922b..46cd422cf4 100644
--- a/src/test/fuzz/util.h
+++ b/src/test/fuzz/util.h
@@ -312,7 +312,7 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
                                        addr_name,
                                        conn_type,
                                        inbound_onion,
-                                       CNodeOptions{ .permission_flags = permission_flags });
+                                       CNodeOptions{.permission_flags = permission_flags});
     } else {
         return CNode{node_id,
                      sock,
@@ -323,7 +323,7 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
                      addr_name,
                      conn_type,
                      inbound_onion,
-                     CNodeOptions{ .permission_flags = permission_flags }};
+                     CNodeOptions{.permission_flags = permission_flags}};
     }
 }

vasild

ACK 377e9cc

ryanofsky

Code review ACK 377e9cc. Looks good and feel free to ignore suggestions!

ryanofsky · 2022-09-01T16:14:31Z

src/net.cpp

@@ -556,7 +556,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
                             pszDest ? pszDest : "",
                             conn_type,
                             /*inbound_onion=*/false,
-                             std::move(i2p_transient_session));
+                             CNodeOptions{ .i2p_sam_session = std::move(i2p_transient_session) });


In commit "net: add CNodeOptions for optional CNode constructor params" (9dccc33)

I'd shorten this as follows, but it's also reasonable to keep if you think the struct name is helpful.

--- a/src/net.cpp +++ b/src/net.cpp @@ -556,7 +556,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo pszDest ? pszDest : "", conn_type, /*inbound_onion=*/false, - CNodeOptions{ .i2p_sam_session = std::move(i2p_transient_session) }); + { .i2p_sam_session = std::move(i2p_transient_session) }); pnode->AddRef(); // We're making a new connection, harvest entropy from the time (and our peer count)

The short version is nice because it's basically a substitute for having named arguments in c++.

Same comment also applies to later commits, as well.

Agree, but I don't think this is possible until we drop support for gcc 8? #25962 (comment)

ryanofsky · 2022-09-01T16:23:40Z

src/test/fuzz/util.h

@@ -310,7 +311,8 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
                                       addr_bind,
                                       addr_name,
                                       conn_type,
-                                       inbound_onion);
+                                       inbound_onion,
+                                       CNodeOptions{ .permission_flags = permission_flags });


Can't omit it here because std::make_unique<CNode>(...) can't resolve to a specific function without knowing what type { .permission_flags = .. } is going to be.

This is ok, but an alternative is to switch from make_unique<CNode> to new CNode

naumenkogs · 2022-09-02T06:44:25Z

ACK 377e9cc

fanquake added the P2P label Aug 31, 2022

ajtowns mentioned this pull request Aug 31, 2022

net/net_processing: Add thread safety related annotations for CNode and Peer #25174

Merged

maflcko reviewed Aug 31, 2022

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

vasild reviewed Aug 31, 2022

View reviewed changes

maflcko added the Refactoring label Aug 31, 2022

This was referenced Aug 31, 2022

p2p: always provide CNodeStateStats and getpeerinfo/netinfo/gui updates #25923

Closed

BIP324: Handshake prerequisites #23561

Closed

vasild reviewed Sep 1, 2022

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

vasild reviewed Sep 1, 2022

View reviewed changes

ajtowns force-pushed the 202208-cnodeoptions branch from 77a33f5 to 2f3602b Compare September 1, 2022 09:17

vasild approved these changes Sep 1, 2022

View reviewed changes

ajtowns added 4 commits September 1, 2022 20:52

net: add CNodeOptions for optional CNode constructor params

9dccc33

net: make CNode::m_permissionFlags const

d394156

net: make CNode::m_prefer_evict const

0a7fc42

scripted-diff: net: rename permissionFlags to permission_flags

377e9cc

-BEGIN VERIFY SCRIPT- sed -i 's/permissionFlags/permission_flags/g' $(git grep -l permissionFlags) -END VERIFY SCRIPT-

ajtowns force-pushed the 202208-cnodeoptions branch from 2f3602b to 377e9cc Compare September 1, 2022 11:00

jonatack reviewed Sep 1, 2022

View reviewed changes

vasild approved these changes Sep 1, 2022

View reviewed changes

ryanofsky approved these changes Sep 1, 2022

View reviewed changes

maflcko merged commit ea67232 into bitcoin:master Sep 2, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 2, 2022

Merge bitcoin#25962: net: Add CNodeOptions and increase constness

7960551

brunoerg mentioned this pull request May 16, 2023

fuzz: net, add recv_flood_size, prefer_evict in ConsumeNode #27678

Closed

bitcoin locked and limited conversation to collaborators Sep 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: Add CNodeOptions and increase constness #25962

net: Add CNodeOptions and increase constness #25962

ajtowns commented Aug 31, 2022

vasild left a comment

hebasto commented Aug 31, 2022

ajtowns commented Aug 31, 2022 •

edited

Loading

DrahtBot commented Aug 31, 2022

vasild left a comment

vasild Sep 1, 2022

ajtowns Sep 1, 2022

vasild Sep 1, 2022

naumenkogs commented Sep 1, 2022

vasild left a comment

vasild Sep 1, 2022

vasild Sep 1, 2022

ajtowns Sep 1, 2022

ajtowns Sep 1, 2022

ryanofsky Sep 1, 2022

jonatack commented Sep 1, 2022

jonatack left a comment

vasild left a comment

ryanofsky left a comment

ryanofsky Sep 1, 2022

ajtowns Sep 2, 2022

ryanofsky Sep 1, 2022

naumenkogs commented Sep 2, 2022

	CNodeOptions{ .permission_flags = permission_flags });
	{ .permission_flags = permission_flags });

net: Add CNodeOptions and increase constness #25962

net: Add CNodeOptions and increase constness #25962

Conversation

ajtowns commented Aug 31, 2022

vasild left a comment

Choose a reason for hiding this comment

hebasto commented Aug 31, 2022

ajtowns commented Aug 31, 2022 • edited Loading

DrahtBot commented Aug 31, 2022

Conflicts

vasild left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naumenkogs commented Sep 1, 2022

vasild left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack commented Sep 1, 2022

jonatack left a comment

Choose a reason for hiding this comment

vasild left a comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naumenkogs commented Sep 2, 2022

ajtowns commented Aug 31, 2022 •

edited

Loading