net: Add CNodeOptions and increase constness #25962

pull ajtowns wants to merge 4 commits into bitcoin:master from ajtowns:202208-cnodeoptions changing 9 files +47 −38
  1. ajtowns commented at 8:17 am on August 31, 2022: contributor
    Adds CNodeOptions to make it easier to add optional parameters to the CNode constructor, and makes prefer_evict and m_permissionFlags actually const.
  2. fanquake added the label P2P on Aug 31, 2022
  3. in src/net.h:354 in 77a33f5773 outdated 
    350@@ -344,7 +351,7 @@ class CNode
351     const std::unique_ptr<TransportDeserializer> m_deserializer; // Used only by SocketHandler thread
352     const std::unique_ptr<const TransportSerializer> m_serializer;
353 
354-    NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; // treated as const outside of fuzz tester
355+    const NetPermissionFlags m_permissionFlags;
    MarcoFalke commented at 8:31 am on August 31, 2022:

    nit (feel free to ignore), but it always felt weird to me to mix camels with snakes. Suggested commit:

    0    scripted-diff: Rename m_permissionFlags to m_permission_flags
1    
2    Seems better to use consistent style within one symbol name.
3    Also, rename permissionFlags to permission_flags.
4    
5    -BEGIN VERIFY SCRIPT-
6     sed -i 's/permissionFlags/permission_flags/g' $(git grep -l permissionFlags)
7    -END VERIFY SCRIPT-
    ajtowns commented at 9:17 am on September 1, 2022:
    Camels eaten by snakes.
  4. vasild commented at 8:34 am on August 31, 2022: contributor

    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.

  5. hebasto commented at 9:15 am on August 31, 2022: member
    Concept ACK.
  6. MarcoFalke added the label Refactoring on Aug 31, 2022
  7. ajtowns commented at 7:02 pm on August 31, 2022: contributor

    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.

  8. DrahtBot commented at 7:23 pm on August 31, 2022: contributor

    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.

  9. in src/net.cpp:1025 in 77a33f5773 outdated 
    1020@@ -1019,6 +1021,9 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
1021     }
1022 
1023     const bool inbound_onion = std::find(m_onion_binds.begin(), m_onion_binds.end(), addr_bind) != m_onion_binds.end();
1024+    CNodeOptions node_opts;
1025+    node_opts.prefer_evict = discouraged;
    vasild commented at 6:09 am on September 1, 2022:

    prefer_evict is set here, but later ignored by the CNode constructor. Maybe this was forgotten:

     0--- i/src/net.cpp
 1+++ w/src/net.cpp
 2@@ -2733,12 +2733,13 @@ CNode::CNode(NodeId idIn,
 3       m_sock{sock},
 4       m_connected{GetTime<std::chrono::seconds>()},
 5       addr{addrIn},
 6       addrBind{addrBindIn},
 7       m_addr_name{addrNameIn.empty() ? addr.ToStringIPPort() : addrNameIn},
 8       m_inbound_onion{inbound_onion},
 9+      m_prefer_evict{node_opts.prefer_evict},
10       nKeyedNetGroup{nKeyedNetGroupIn},
11       id{idIn},
12       nLocalHostNonce{nLocalHostNonceIn},
13       m_conn_type{conn_type_in},
14       m_i2p_sam_session{std::move(node_opts.i2p_sam_session)}
15 {
  10. in src/net.cpp:1026 in 77a33f5773 outdated 
    1020@@ -1019,6 +1021,9 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
1021     }
1022 
1023     const bool inbound_onion = std::find(m_onion_binds.begin(), m_onion_binds.end(), addr_bind) != m_onion_binds.end();
1024+    CNodeOptions node_opts;
1025+    node_opts.prefer_evict = discouraged;
1026+    node_opts.permission_flags = permissionFlags;
    vasild commented at 6:29 am on September 1, 2022:

    It could be a bit shorter if designated initializers are used, instead of:

    0Opts opts;
1opts.x = 1;
2opts.y = 2;
3opts.z = 3;
4new CNode(
5    ...
6    opts
7);

    it would become:

    0new CNode(
1    ...
2    {
3        .x = 1,
4        .y = 2,
5        .z = 3
6    }
7);
    ajtowns commented at 9:01 am on September 1, 2022:
    Err, yeah; I had the same thought, but apparently didn’t push it.
  11. vasild commented at 6:35 am on September 1, 2022: contributor

    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.

  12. in src/net.h:337 in 77a33f5773 outdated 
    333@@ -334,6 +334,13 @@ class V1TransportSerializer : public TransportSerializer {
334     void prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) const override;
335 };
336 
337+struct CNodeOptions
    vasild commented at 6:39 am on September 1, 2022:
    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.
    ajtowns commented at 9:16 am on September 1, 2022:

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

    0class X
1{
2public:
3    struct Y
4    {
5        int foo = 42;
6    };
7
8    int SomeFunc(Y y = {});
9};

    gives:

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

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

    vasild commented at 9:53 am on September 1, 2022:
    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.
  13. naumenkogs commented at 7:18 am on September 1, 2022: member
    Concept ACK. Will do code review once the pending feedback is resolved.
  14. ajtowns force-pushed on Sep 1, 2022
  15. in src/test/fuzz/util.h:315 in 2f3602b655 outdated 
    310@@ -310,7 +311,8 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
311                                        addr_bind,
312                                        addr_name,
313                                        conn_type,
314-                                       inbound_onion);
315+                                       inbound_onion,
316+                                       CNodeOptions{ .permission_flags = permission_flags });
    vasild commented at 9:44 am on September 1, 2022:

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

    0                                       { .permission_flags = permission_flags });
    vasild commented at 9:57 am on September 1, 2022:

    Or maybe not: CI is upset:

    0./net.h:523:5: note:   no known conversion for argument 10 from ‘<brace-enclosed initializer list>’ to ‘CNodeOptions&&’
    ajtowns commented at 10:13 am on September 1, 2022:
    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)
    ajtowns commented at 10:50 am on September 1, 2022:
    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
    ryanofsky commented at 4:23 pm on September 1, 2022:

    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

  16. vasild approved
  17. vasild commented at 9:48 am on September 1, 2022: contributor
    ACK 2f3602b655cac8ada33ba2a5371c216ae1f1025e
  18. jonatack commented at 10:44 am on September 1, 2022: contributor
    Review ACK modulo the CI issue
  19. net: add CNodeOptions for optional CNode constructor params 9dccc3328e
  20. net: make CNode::m_permissionFlags const d394156b99
  21. net: make CNode::m_prefer_evict const 0a7fc42897
  22. scripted-diff: net: rename permissionFlags to permission_flags 
    -BEGIN VERIFY SCRIPT-
sed -i 's/permissionFlags/permission_flags/g' $(git grep -l permissionFlags)
-END VERIFY SCRIPT-
    377e9ccda4
  23. ajtowns force-pushed on Sep 1, 2022
  24. jonatack commented at 11:26 am on September 1, 2022: contributor

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

     0diff --git a/src/net.cpp b/src/net.cpp
 1index 6659b64246..138486e468 100644
 2--- a/src/net.cpp
 3+++ b/src/net.cpp
 4@@ -556,7 +556,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
 5                              pszDest ? pszDest : "",
 6                              conn_type,
 7                              /*inbound_onion=*/false,
 8-                             CNodeOptions{ .i2p_sam_session = std::move(i2p_transient_session) });
 9+                             CNodeOptions{.i2p_sam_session = std::move(i2p_transient_session)});
10     pnode->AddRef();
11
12@@ -1029,8 +1027,8 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
13                              ConnectionType::INBOUND,
14                              inbound_onion,
15                              CNodeOptions{
16-                               .permission_flags = permission_flags,
17-                               .prefer_evict = discouraged,
18+                                 .permission_flags = permission_flags,
19+                                 .prefer_evict = discouraged,
20                              });
21diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h
22index 6d652c922b..46cd422cf4 100644
23--- a/src/test/fuzz/util.h
24+++ b/src/test/fuzz/util.h
25@@ -312,7 +312,7 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
26                                        addr_name,
27                                        conn_type,
28                                        inbound_onion,
29-                                       CNodeOptions{ .permission_flags = permission_flags });
30+                                       CNodeOptions{.permission_flags = permission_flags});
31     } else {
32         return CNode{node_id,
33                      sock,
34@@ -323,7 +323,7 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
35                      addr_name,
36                      conn_type,
37                      inbound_onion,
38-                     CNodeOptions{ .permission_flags = permission_flags }};
39+                     CNodeOptions{.permission_flags = permission_flags}};
40     }
41 }
  25. vasild approved
  26. vasild commented at 2:55 pm on September 1, 2022: contributor
    ACK 377e9ccda469731d535829f184b70c73ed46b6ef
  27. in src/net.cpp:559 in 9dccc3328e outdated 
    555@@ -556,7 +556,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
556                              pszDest ? pszDest : "",
557                              conn_type,
558                              /*inbound_onion=*/false,
559-                             std::move(i2p_transient_session));
560+                             CNodeOptions{ .i2p_sam_session = std::move(i2p_transient_session) });
    ryanofsky commented at 4:14 pm on September 1, 2022:

    In commit “net: add CNodeOptions for optional CNode constructor params” (9dccc3328eeaf9cd66518d812c878def5d014e36)

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

     0--- a/src/net.cpp
 1+++ b/src/net.cpp
 2@@ -556,7 +556,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
 3                              pszDest ? pszDest : "",
 4                              conn_type,
 5                              /*inbound_onion=*/false,
 6-                             CNodeOptions{ .i2p_sam_session = std::move(i2p_transient_session) });
 7+                             { .i2p_sam_session = std::move(i2p_transient_session) });
 8     pnode->AddRef();
 9 
10     // 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.

    ajtowns commented at 4:05 am on September 2, 2022:
    Agree, but I don’t think this is possible until we drop support for gcc 8? #25962 (review)
  28. ryanofsky approved
  29. ryanofsky commented at 4:27 pm on September 1, 2022: contributor
    Code review ACK 377e9ccda469731d535829f184b70c73ed46b6ef. Looks good and feel free to ignore suggestions!
  30. naumenkogs commented at 6:44 am on September 2, 2022: member
    ACK 377e9ccda469731d535829f184b70c73ed46b6ef
  31. MarcoFalke merged this on Sep 2, 2022
  32. MarcoFalke closed this on Sep 2, 2022
  33. sidhujag referenced this in commit 7960551c83 on Sep 2, 2022
  34. bitcoin locked this on Sep 2, 2023


ajtowns MarcoFalke vasild hebasto DrahtBot naumenkogs ryanofsky jonatack

Labels
Refactoring P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-05 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me