net: assert CNode::m_inbound_onion is inbound in ctor, add getter, unit tests #20210

jonatack · 2020-10-21T10:14:15Z

The goal of this PR is to be able to depend on m_inbound_onion in AttemptToEvictConnection in #20197:

asserts CNode::m_inbound_onion is inbound in the CNode ctor to have a validity check at the class boundary
fixes a unit test and a fuzz utility that were passing invalid inbound onion values to the CNode ctor
drops an unneeded check in CNode::ConnectedThroughNetwork() for its inbound status
adds a public getter IsInboundOnion() that also allows unit testing it
adds unit test coverage

hebasto

ACK 39286b3, tested on Linux Mint 20 (x86_64).

practicalswift · 2020-10-21T12:51:23Z

Concept ACK

Thanks for improving testing!

DrahtBot · 2020-11-12T01:20:26Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

fuzz: Rework strong and weak net enum fuzzing #20789 (fuzz: Rework strong and weak net enum fuzzing by MarcoFalke)
net: [refactor] Prefer integral types in CNodeStats #20786 (net: [refactor] Prefer integral types in CNodeStats by MarcoFalke)
p2p: standardize outbound full/block relay connection type naming #20729 (p2p: standardize on outbound-{full, block}-relay connection type naming by jonatack)
Net: Move ping data to net_processing #20721 (Net: Move ping data to net_processing by jnewbery)
refactor: Remove nMyStartingHeight from CNode/Connman #20649 (refactor: Remove nMyStartingHeight from CNode/Connman by MarcoFalke)
refactor, net: Increase CNode data member encapsulation #20373 (refactor, net: Increase CNode data member encapsulation by hebasto)

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.

jonatack · 2020-11-13T17:27:11Z

Rebased.

theStack

ACK 0a30f5d 🏞️

theStack · 2020-11-22T18:17:26Z

src/net.h

@@ -1095,7 +1095,7 @@ class CNode
    CService addrLocal GUARDED_BY(cs_addrLocal);
    mutable RecursiveMutex cs_addrLocal;

-    //! Whether this peer connected via our Tor onion service.
+    //! Whether this peer is an inbound onion, e.g. connected via our Tor onion service.
    const bool m_inbound_onion{false};


yocto-nit (not directly related to this PR though, feel free to ignore): not sure if we have any guidelines (or dominant opinions) on this subject, but isn't it overkill to have two initializations for a class member: once via in-class initialization and once via initializer list in the ctor? (And yes, I'm aware that initializer lists have higher priority in C++). As reader of the class interface I'd not assume an in-class initialization is overruled in the ctor. Also, without the in-class initialization the compiler has the chance to throw an error if the const member was forgot to be initialized in the ctor.

Hm, good eye.

-> In this case, ISTM we could remove the default false argument in the constructor declaration

-> As to your last point, it suggests the default member m_inbound_onion initializer not set a value, since we are doing it dynamically in the ctor. It's true that I see a build error in this case, but not without the change in net.h:1099

/src/net.cpp - nMyStartingHeight(nMyStartingHeightIn), - m_inbound_onion(inbound_onion && conn_type_in == ConnectionType::INBOUND) + nMyStartingHeight(nMyStartingHeightIn) /src/net.h //! Whether this peer is an inbound onion, e.g. connected via our Tor onion service. - const bool m_inbound_onion{false}; + const bool m_inbound_onion;

net.cpp:2947:1: error: uninitialized const member in ‘const bool’ [-fpermissive] 2947 | CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion) | ^~~~~ In file included from net.cpp:10: ./net.h:1099:16: note: ‘const bool CNode::m_inbound_onion’ should be initialized 1099 | const bool m_inbound_onion; | ^~~~~~~~~~~~~~~

I don't mind pulling in a commit from your branch if you'd like to adjust this.

More generally, from what I can gather, the ideas behind in-class default member initializers added in C++11 were (a) to allow a non-static data member to be initialized where it is declared, e.g. in its class. A constructor can then use the initializer when run-time initialization is needed, which can then (b) tidy up the code up by avoiding doing it in the member initializer list of the constructor. This provides extra benefits in classes with multiple constructors.

I guess one can see the default member initializer as the general default, particularly useful for constant initializers, that can be overridden by any specific ones in member initializer lists in the constructors.

Some resources (send me any good ones you might suggest; my understanding on this is evolving):

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#Rc-in-class-initializer

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#c49-prefer-initialization-to-assignment-in-constructors

https://en.cppreference.com/w/cpp/language/data_members#member-initialization

https://www.stroustrup.com/C++11FAQ.html#member-init

not sure if we have any guidelines (or dominant opinions) on this subject, but isn't it overkill to have two initializations for a class member: once via in-class initialization and once via initializer list in the ctor? (And yes, I'm aware that initializer lists have higher priority in C++). As reader of the class interface I'd not assume an in-class initialization is overruled in the ctor. Also, without the in-class initialization the compiler has the chance to throw an error if the const member was forgot to be initialized in the ctor.

@theStack: done in #21167

laanwj · 2020-12-03T14:15:13Z

src/net.cpp

@@ -2957,7 +2957,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
    m_conn_type(conn_type_in),
    nLocalServices(nLocalServicesIn),
    nMyStartingHeight(nMyStartingHeightIn),
-    m_inbound_onion(inbound_onion)
+    m_inbound_onion(inbound_onion && conn_type_in == ConnectionType::INBOUND)


I might be misunderstanding but in what case would the caller pass inbound_onion parameter as true but lie about this, as it is an outbound connection? Is this a code bug?
Wouldn't this be better as an assertion?

Or an Assume, if Assert is too strong and risky

Good idea, inbound_onion should only be true when a new CNode is instantiated from CConnman::AcceptConnection(), so this should only fail if a bug is added. Made it an assert.

in what case would the caller pass inbound_onion parameter as true but lie about this

So, adding the assertion found one! Our net_tests had a case that lied; updating it.

Hm, a couple of fuzzers need to be updated, too. Edit: done

…nsumeNode() 23d8f34 fuzz: replace CNode code with fuzz/util.h::ConsumeNode() (Jon Atack) Pull request description: Noticed this while updating the CNode fuzzing in #20210. - cc26fab created `test/fuzz/net.cpp` in May 2020 - 79ef832 created a CNode factory utility `test/fuzz/util.h::ConsumeNode()` in October 2020 This PR updates `fuzz/net.cpp` from the first commit to use `ConsumeNode()` from the second commit. ACKs for top commit: MarcoFalke: ACK 23d8f34 Tree-SHA512: 26f7685395b3d48fcf40dde0d479d5c2fb4e953ec9371940b19eee16bb30aee4840b081e1a918b924a9704c1bef484302ea3e8fe63819a3bba73e7eb805164f1

as CNode ctor should only be passed inbound_onion = true when the connection is inbound

and drop an unneeded check in CNode::ConnectedThroughNetwork()

jonatack · 2020-12-17T19:00:44Z

Rebased/simplified after merge of #20686.

…l.h::ConsumeNode() 23d8f34 fuzz: replace CNode code with fuzz/util.h::ConsumeNode() (Jon Atack) Pull request description: Noticed this while updating the CNode fuzzing in bitcoin#20210. - cc26fab created `test/fuzz/net.cpp` in May 2020 - 79ef832 created a CNode factory utility `test/fuzz/util.h::ConsumeNode()` in October 2020 This PR updates `fuzz/net.cpp` from the first commit to use `ConsumeNode()` from the second commit. ACKs for top commit: MarcoFalke: ACK 23d8f34 Tree-SHA512: 26f7685395b3d48fcf40dde0d479d5c2fb4e953ec9371940b19eee16bb30aee4840b081e1a918b924a9704c1bef484302ea3e8fe63819a3bba73e7eb805164f1

sipa · 2020-12-27T05:06:04Z

utACK 86c4952

LarryRuane

ACK 86c4952
Compiled, ran unit tests.

vasild

ACK 86c4952

Two minor comments below, feel free to ignore.

vasild · 2020-12-30T13:05:34Z

src/test/fuzz/util.h

@@ -298,7 +298,7 @@ CNode ConsumeNode(FuzzedDataProvider& fuzzed_data_provider) noexcept
    const CAddress addr_bind = ConsumeAddress(fuzzed_data_provider);
    const std::string addr_name = fuzzed_data_provider.ConsumeRandomLengthString(64);
    const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray({ConnectionType::INBOUND, ConnectionType::OUTBOUND_FULL_RELAY, ConnectionType::MANUAL, ConnectionType::FEELER, ConnectionType::BLOCK_RELAY, ConnectionType::ADDR_FETCH});
-    const bool inbound_onion = fuzzed_data_provider.ConsumeBool();
+    const bool inbound_onion{conn_type == ConnectionType::INBOUND ? fuzzed_data_provider.ConsumeBool() : false};


nit: same as ConnectionType::INBOUND && fuzzed_data_provider.ConsumeBool()

indeed, will update if need to retouch or rebase

difference is that it needs to store one additional redundant byte for every non-inbound fuzz input file, so I think this can be kept as is

src/net.h

maflcko

review ACK 86c4952 🐍

Show signature and timestamp

Signature:

Timestamp of file with hash 683c8f1c6754ede192ead999e54c0440e14b98b058b5f69d6e0b9817be2c17bb -

maflcko · 2021-01-02T08:52:48Z

src/net.h

@@ -1234,6 +1234,9 @@ class CNode
    void MaybeSetAddrName(const std::string& addrNameIn);

    std::string ConnectionTypeAsString() const;
+
+    /** Whether this peer is an inbound onion, e.g. connected via our Tor onion service. */


nit question: should this say "i.e."? I mean are there any other ways to establish an inbound onion other than via the tor onion service?

I don't mind updating this and taking @vasild's suggestion at the same time.

Can do as a follow-up. The merge script is already running and this should land in master in the next couple of minutes.

Oh ok, will do in #20197.

nit question: should this say "i.e."? I mean are there any other ways to establish an inbound onion other than via the tor onion service?

Done in #21167

… ctor, add getter, unit tests 86c4952 net: add CNode::IsInboundOnion() public getter and unit tests (Jon Atack) 6609eb8 net: assert CNode::m_inbound_onion is inbound in ctor (Jon Atack) 993d1ec test, fuzz: fix constructing CNode with invalid inbound_onion (Jon Atack) Pull request description: The goal of this PR is to be able to depend on `m_inbound_onion` in AttemptToEvictConnection in bitcoin#20197: - asserts `CNode::m_inbound_onion` is inbound in the CNode ctor to have a validity check at the class boundary - fixes a unit test and a fuzz utility that were passing invalid inbound onion values to the CNode ctor - drops an unneeded check in `CNode::ConnectedThroughNetwork()` for its inbound status - adds a public getter `IsInboundOnion()` that also allows unit testing it - adds unit test coverage ACKs for top commit: sipa: utACK 86c4952 LarryRuane: ACK 86c4952 vasild: ACK 86c4952 MarcoFalke: review ACK 86c4952 🐍 Tree-SHA512: 21109105bc4e5e03076fadd489204be00eac710c9de0127708ca2d0a10a048ff81f640f589a7429967ac3eb51d35fe24bb2b12e53e7aa3efbc47aaff6396d204

jnewbery · 2021-01-23T15:11:34Z

Is there any benefit to adding getter boilerplate code to the header class instead of just making this const member public?

jonatack · 2021-01-26T18:25:23Z

@jnewbery Unless additional semantics will be needed for future changes or refactoring, no, probably not. I was being prudent.

jnewbery · 2021-01-26T18:40:55Z

oooh thanks for referencing the cpp core guidelines! I'd say that any getter of a const value is trivial :)

jonatack · 2021-02-12T22:41:27Z

oooh thanks for referencing the cpp core guidelines! I'd say that any getter of a const value is trivial :)

@jnewbery thanks! done in #21167

…licitly 2ee4a7a net: remove CNode::m_inbound_onion defaults for explicitness (Jon Atack) 24bda56 net: make CNode::m_inbound_onion public, drop getter, update tests (Jon Atack) Pull request description: Refactoring only, no change in behavior. This is a quick follow-up to #20210 to address these review comments: - bitcoin/bitcoin#20210 (comment) - bitcoin/bitcoin#20210 (comment) - bitcoin/bitcoin#20210 (comment) Changes: - make the `CNode::m_inbound_onion class` member public, update the Doxygen comment, drop the getter, and update the tests - remove the `CNode::m_inbound_onion` default value initialization in the ctor declaration and the member initializer in favor of always passing it explicitly to the ctor where we initialize it dynamically, to both clarify the caller code and to allow the compiler to warn if it is uninitialized in the ctor or omitted in the caller ACKs for top commit: MarcoFalke: review ACK 2ee4a7a 🏀 vasild: ACK 2ee4a7a Tree-SHA512: 72961c91168885a9d881756b10bad9d587f5ce297d5a6493c23e267b7178ff22b697bc6868e7761d6304e372d2781453d30011a020afd506b1e308b4ffa5feee

…eviction protection test coverage 0cca08a Add unit test coverage for our onion peer eviction protection (Jon Atack) caa21f5 Protect onion+localhost peers in ProtectEvictionCandidatesByRatio() (Jon Atack) 8f1a53e Use EraseLastKElements() throughout SelectNodeToEvict() (Jon Atack) 8b1e156 Add m_inbound_onion to AttemptToEvictConnection() (Jon Atack) 72e30e8 Add unit tests for ProtectEvictionCandidatesByRatio() (Jon Atack) ca63b53 Use std::unordered_set instead of std::vector in IsEvicted() (Jon Atack) 41f84d5 Move peer eviction tests to a separate test file (Jon Atack) f126cbd Extract ProtectEvictionCandidatesByRatio from SelectNodeToEvict (Jon Atack) Pull request description: Now that #19991 and #20210 have been merged, we can determine inbound onion peers using `CNode::m_inbound_onion` and add it to the localhost peers protection in `AttemptToEvictConnection`, which was added in #19670 to address issue #19500. Update 28 February 2021: I've updated this to follow gmaxwell's suggestion in #20197 (comment). This branch now protects up to 1/4 onion peers (connected via our tor control service), if any, sorted by longest uptime. If any (or all) onion slots remain after that operation, they are then allocated to protect localhost peers, or a minimum of 2 localhost peers in the case that no onion slots remain and 2 or more onion peers were protected, sorted as before by longest uptime. This patch also adds test coverage for the longest uptime, localhost, and onion peer eviction protection logic to build on the welcome initial unit testing of #20477. Suggest reviewing the commits that move code with `colorMoved = dimmed-zebra` and `colorMovedWs = allow-indentation-change`. Closes #11537. ACKs for top commit: laanwj: Code review ACK 0cca08a vasild: ACK 0cca08a Tree-SHA512: 2f5a63f942acaae7882920fc61f0185dcd51da85e5b736df9d1fc72343726dd17da740e02f30fa5dc5eb3b2d8345707aed96031bec143d48a2497a610aa19abd

…it tests Summary: > test: fix constructing CNode with invalid inbound_onion > > as CNode ctor should only be passed inbound_onion = true > when the connection is inbound > net: assert CNode::m_inbound_onion is inbound in ctor > > and drop an unneeded check in CNode::ConnectedThroughNetwork() > net: add CNode::IsInboundOnion() public getter and unit tests This is a backport of [[bitcoin/bitcoin#20210 | core#20210]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10971

hebasto approved these changes Oct 21, 2020

View reviewed changes

fanquake added the P2P label Oct 21, 2020

DrahtBot mentioned this pull request Nov 12, 2020

refactor, net: Increase CNode data member encapsulation #20373

Merged

jonatack force-pushed the CNode-IsInboundOnion branch from 39286b3 to 0a30f5d Compare November 13, 2020 17:26

theStack approved these changes Nov 22, 2020

View reviewed changes

laanwj reviewed Dec 3, 2020

View reviewed changes

This was referenced Dec 14, 2020

doc: Move addr relay comment in net to correct place #20653

Merged

refactor: Remove nMyStartingHeight from CNode/Connman #20649

Merged

DrahtBot added the Needs rebase label Dec 15, 2020

jonatack force-pushed the CNode-IsInboundOnion branch from 0a30f5d to dd01a23 Compare December 17, 2020 11:52

jonatack changed the title ~~net: ensure CNode::m_inbound_onion is inbound, add getter, unit tests~~ net: assert CNode::m_inbound_onion is inbound in ctor, add getter, unit tests Dec 17, 2020

jonatack force-pushed the CNode-IsInboundOnion branch from dd01a23 to 84b91db Compare December 17, 2020 12:48

DrahtBot removed the Needs rebase label Dec 17, 2020

jonatack force-pushed the CNode-IsInboundOnion branch 2 times, most recently from 390c735 to af18de1 Compare December 17, 2020 17:41

jonatack mentioned this pull request Dec 17, 2020

fuzz, refactor: replace CNode code with fuzz/util.h::ConsumeNode() #20686

Merged

jonatack added 3 commits December 17, 2020 19:56

test, fuzz: fix constructing CNode with invalid inbound_onion

993d1ec

as CNode ctor should only be passed inbound_onion = true when the connection is inbound

net: assert CNode::m_inbound_onion is inbound in ctor

6609eb8

and drop an unneeded check in CNode::ConnectedThroughNetwork()

net: add CNode::IsInboundOnion() public getter and unit tests

86c4952

jonatack force-pushed the CNode-IsInboundOnion branch from af18de1 to 86c4952 Compare December 17, 2020 18:59

DrahtBot mentioned this pull request Dec 20, 2020

p2p: standardize outbound full/block relay connection type naming #20729

Closed

jonatack mentioned this pull request Dec 23, 2020

p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage #20197

Merged

DrahtBot mentioned this pull request Dec 26, 2020

net, p2p, gui: replace direction with connection type in gui peer details window #20778

Closed

This was referenced Dec 28, 2020

net: [refactor] Prefer integral types in CNodeStats #20786

Merged

fuzz: Rework strong and weak net enum fuzzing #20789

Merged

LarryRuane reviewed Dec 29, 2020

View reviewed changes

DrahtBot mentioned this pull request Dec 29, 2020

Net: Move ping data to net_processing #20721

Merged

vasild approved these changes Dec 30, 2020

View reviewed changes

maflcko approved these changes Jan 2, 2021

View reviewed changes

maflcko merged commit ae8f797 into bitcoin:master Jan 2, 2021

jonatack deleted the CNode-IsInboundOnion branch January 2, 2021 09:00

jonatack mentioned this pull request Feb 12, 2021

net: make CNode::m_inbound_onion public, initialize explicitly #21167

Merged

jonatack mentioned this pull request Aug 24, 2021

init: Fix asmap/addrman initialization order bug #22791

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

net: assert CNode::m_inbound_onion is inbound in ctor, add getter, unit tests #20210

net: assert CNode::m_inbound_onion is inbound in ctor, add getter, unit tests #20210

Uh oh!

Conversation

jonatack commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Oct 21, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

jonatack commented Nov 13, 2020

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

theStack Nov 22, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Dec 3, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Dec 17, 2020 • 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!

jonatack commented Dec 17, 2020

Uh oh!

sipa commented Dec 27, 2020

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Jan 2, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jan 23, 2021

Uh oh!

jonatack commented Jan 26, 2021

Uh oh!

jnewbery commented Jan 26, 2021

jonatack commented Oct 21, 2020 •

edited

Loading

DrahtBot commented Nov 12, 2020 •

edited

Loading

theStack Nov 22, 2020 •

edited

Loading

laanwj Dec 3, 2020 •

edited

Loading

jonatack Dec 17, 2020 •

edited

Loading

jonatack Jan 2, 2021 •

edited

Loading