[net processing] Various tidying up of PeerManagerImpl ctor

jnewbery commented at 5:58 pm on March 31, 2021: member

Use default initialization of PeerManagerImpl members where possible
Remove unique_ptr indirection where it’s not needed

DrahtBot added the label P2P on Mar 31, 2021

fanquake commented at 2:12 am on April 1, 2021: member

Cirrus failures:

 0In file included from init.cpp:55:
 1./txorphanage.h:50:28: error: reading variable 'm_orphans' requires holding mutex 'g_cs_orphans' [-Werror,-Wthread-safety-analysis]
 2    size_t Size() { return m_orphans.size(); };
 3                           ^
 41 error generated.
 5make[2]: *** [libbitcoin_server_a-init.o] Error 1
 6make[2]: *** Waiting for unfinished jobs....
 7  CXX      libbitcoin_server_a-net_processing.o
 8In file included from net_processing.cpp:29:
 9./txorphanage.h:50:28: error: reading variable 'm_orphans' requires holding mutex 'g_cs_orphans' [-Werror,-Wthread-safety-analysis]
10    size_t Size() { return m_orphans.size(); };
11                           ^
12net_processing.cpp:410:59: error: expected ';' at end of declaration list
13    CRollingBloomFilter m_recent_rejects{120000, 0.000001} GUARDED_BY(cs_main);
14                                                          ^
15                                                          ;
16net_processing.cpp:429:73: error: expected ';' at end of declaration list
17    CRollingBloomFilter m_recent_confirmed_transactions{48000, 0.000001} GUARDED_BY(m_recent_confirmed_transactions_mutex);
18                                                                        ^
19                                                                        ;
203 errors generated.

DrahtBot commented at 4:25 am on April 1, 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:

#22567 by MarcoFalke
#20362 by MarcoFalke
#18470 by MarcoFalke

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.

jnewbery force-pushed on Apr 1, 2021

practicalswift commented at 9:14 am on April 1, 2021: contributor

Strong Concept ACK

I find the suggested code easier to reason about from a safety and correctness perspective.

jnewbery force-pushed on Apr 1, 2021

jnewbery marked this as ready for review on Apr 1, 2021

jnewbery closed this on Apr 1, 2021

jnewbery reopened this on Apr 1, 2021

MarcoFalke commented at 2:00 pm on April 1, 2021: member

The build failure is because Assume breaks static lock analysis. One way to fix is to assign the assumed-value to a bool first.

jnewbery force-pushed on Apr 1, 2021

jnewbery commented at 3:27 pm on April 1, 2021: member

Thanks Marco! I’ve just reverted these checks to assert() for now.

hebasto commented at 10:55 pm on April 2, 2021: member

@jnewbery Help me to understand the motivation for “[net processing] Move consistency checks to PeerManagerImpl dtor” commit please.

My current understanding is following. A consistency is an invariant that the PeerManagerImpl class must hold. It’s ctor’s job to establish class’s invariants, and member functions’ job to preserve invariants. Delegating such a task to dtor seems unusual for me.

jnewbery commented at 3:57 pm on April 6, 2021: member

Help me to understand the motivation for “[net processing] Move consistency checks to PeerManagerImpl dtor” commit please.

My current understanding is following. A consistency is an invariant that the PeerManagerImpl class must hold. It’s ctor’s job to establish class’s invariants, and member functions’ job to preserve invariants. Delegating such a task to dtor seems unusual for me.

I think that’s fair. My reasoning was that these invariant checks are all test/development asserts. We’d hope that they’d never be hit in the wild. They currently run when the peer count drops to zero, but if there’s an inconsistency then, we’d expect those invariants to also be false when the PeerManagerImpl is destructed on shutdown.

My motivation is that I want to pull some of these members out from being guarded by cs_main. The global counts like m_outbound_peers_with_protect_from_disconnect and m_wtxid_relay_peers don’t actually need to be atomically updated with the correspondingCNodeState member variables m_chain_sync.m_protect and m_wtxid_relay. We could pull those things out of cs_main and just make them separate atomics and that’d be safe, but not obviously so. FinalizeNode() is called by the same thread that increments the counters, so there’s no way that they can be non-zero whilst the final peer is being removed. However, if other threads were able to increment the counters, these invariants would no longer be true. That’s not the case if we move the invariant checks to the destructor. Inside the dtor, we know that no other thread can be mutating the members, so the counters must be at zero.

in src/net_processing.cpp:1251 in a1f2f65be4 outdated

1244@@ -1256,6 +1245,20 @@ PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& conn
1245     scheduler.scheduleFromNow([&] { ReattemptInitialBroadcast(scheduler); }, delta);
1246 }
1247 
1248+PeerManagerImpl::~PeerManagerImpl()
1249+{
1250+    // Do consistency checks on shutdown
1251+    LOCK(cs_main);

hebasto commented at 6:53 am on April 11, 2021:

Only one thread has an access to the object while a dtor is called. Why lock?

DrahtBot commented at 9:32 am on May 3, 2021: member

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

jnewbery force-pushed on May 3, 2021

jnewbery renamed this:
~~[net processing] Various tidying up of PeerManagerImpl ctor and dtor~~
[net processing] Various tidying up of PeerManagerImpl ctor
on May 3, 2021

jnewbery commented at 11:20 am on May 3, 2021: member

@hebasto - there are various changes to net_processing logging happening in 21527, so I’ve removed the changes to the destructor here until those land. Let me know if you think the remaining changes to the constructor in this PR are worth keeping.

Also rebased on current master.

jnewbery deleted a comment on May 3, 2021

Crypt-iQ commented at 0:58 am on May 9, 2021: contributor

Code review ACK b26a5adceaf4e1dbf35aa595f7870fe059940959

I was confused by the asserts prior to this PR. Also there are two typos in the commit messages “contetx” should be “context”?

jnewbery force-pushed on May 10, 2021

jnewbery commented at 6:45 am on May 10, 2021: member

Thanks @Crypt-iQ. I’ve corrected the typos in the commit logs.

I was confused by the asserts prior to this PR.

These were to ensure that recentRejects wasn’t null before dereferencing it. No need for that now that m_recent_rejects is not a pointer.

Crypt-iQ commented at 3:34 pm on May 12, 2021: contributor

Code review ACK d6cef4d8a6a70920e98f40530113733142025f6a

Only the commit messages were fixed from b26a5adceaf4e1dbf35aa595f7870fe059940959, so should be good.

jnewbery force-pushed on Jun 2, 2021

jnewbery commented at 3:12 pm on June 2, 2021: member

Rebased on master to fix fuzz CI timeout.

Crypt-iQ commented at 9:40 pm on June 5, 2021: contributor

Code review ACK 93e881f

Commit message 9e6bf785 has a typo: rejectRejects should be recentRejects

DrahtBot added the label Needs rebase on Jun 12, 2021

jnewbery commented at 9:23 am on June 14, 2021: member

Rebased.

Commit message 9e6bf78 has a typo: rejectRejects should be recentRejects

Fixed. Thank you!

jnewbery force-pushed on Jun 14, 2021

DrahtBot removed the label Needs rebase on Jun 14, 2021

MarcoFalke commented at 4:44 pm on June 14, 2021: member

review ACK d5dd3fa1de2f96a7ddfd8b300829c68b93bd4607 🔠

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK d5dd3fa1de2f96a7ddfd8b300829c68b93bd4607 🔠
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhpNwwAjxKP8RmX2H5ReC3tV5MQ/WvFMI9x+WHwIiNcpkRQ8wk4kErsemjnoroT
 8gEZg/nq3IfgMQL3LvaU270gGOdH3J45TFJtoGCtq+nogV/nU8TwjlCzP0vUsRzfP
 93hiarnsn3GIJs9r01uC31GII/TrTSZPDjsKxd9X5aLKAtjsMlB9C5mZdQtQ5CYul
10+3QGhnGGZpTIqE9V24dY0sOUrO54UtSW87qaxRXx88MkgVz+LTF1TDjSJy9We6Hj
11ehe8OTC9MoHOYJ5rv7aiLsrJ9H28Ntal6w84Q9lts1laTEyMlazegtjIvoUogrP4
12eswz53RfJg1APyIDZPn62+xNp3XYIG7ib29L3A/lsjjbw6d862EDOmyjVVloTMcM
13Tb7ruvFaZdr8k9TBeKtZ1P/ecEPszUWldCIJ5RXo9iIIGaks7f8OKJNyTmgxjfVj
14HOVgn1gwuE2sTQHb6pSuW45JBOslXCrYaCOQNWrCpX29X0iOpT4bJM31WVX1lFzH
15TtiamXSG
16=v+4O
17-----END PGP SIGNATURE-----

Timestamp of file with hash cb3d358ad6eccc7781819f9917bb2f9a6f7b802e636f39fe7a8d779c985d5dfc -

in src/txorphanage.h:50 in d5dd3fa1de outdated

46@@ -47,6 +47,8 @@ class TxOrphanage {
47      * (ie orphans that may have found their final missing parent, and so should be reconsidered for the mempool) */
48     void AddChildrenToWorkSet(const CTransaction& tx, std::set<uint256>& orphan_work_set) const EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans);
49 
50+    size_t Size() { return WITH_LOCK(g_cs_orphans, return m_orphans.size(););};

hebasto commented at 1:06 am on June 18, 2021:

60cf71cfc16ec59857bd97608f599f8d2346c23c

style-nit: redundant ; and missed space:

0    size_t Size() { return WITH_LOCK(g_cs_orphans, return m_orphans.size()); };

what about LOCKS_EXCLUDED(::g_cs_orphans) thread-safety annotation?
style-nit: isn’t LOCK(::g_cs_orphans); return m_orphans.size(); shorter and more readable?
why not use global namespace :: for global mutexes consistently?

jnewbery commented at 4:11 pm on June 30, 2021:

All fixed. Thanks for the suggestions!

in src/net_processing.cpp:400 in d5dd3fa1de outdated

378@@ -379,7 +379,8 @@ class PeerManagerImpl final : public PeerManager
379     /** The height of the best chain */
380     std::atomic<int> m_best_height{-1};
381 
382-    int64_t m_stale_tip_check_time; //!< Next time to check for stale tip
383+    /** Next time to check for stale tip */
384+    int64_t m_stale_tip_check_time{0};

hebasto commented at 1:19 am on June 18, 2021:

754bdaa61fe6473ca7cf9e89146eb8419fdd7c0a

I’m not an expert in Doxygen comments, but our Developer Notes suggest //! or //!< for variables.

jnewbery commented at 3:58 pm on June 30, 2021:

Hmm, I wasn’t aware of that guidance in the developer notes. Perhaps it should be updated - /** comment */ is perfectly acceptable to doxygen, and many of the variables/members in the code base use that style.

in src/net_processing.cpp:456 in d5dd3fa1de outdated

452@@ -452,16 +453,26 @@ class PeerManagerImpl final : public PeerManager
453      *
454      * Memory used: 1.3 MB
455      */
456-    std::unique_ptr<CRollingBloomFilter> recentRejects GUARDED_BY(cs_main);
457+    CRollingBloomFilter m_recent_rejects GUARDED_BY(cs_main) {120000, 0.000001};

hebasto commented at 1:27 am on June 18, 2021:

6e48bfe706934fa4b71a3021d979321e94343c21, style nit:

0    CRollingBloomFilter m_recent_rejects GUARDED_BY(::cs_main){120'000, 0.000001};

(clang-format-diff.py states that the space is redundant)

jnewbery commented at 4:22 pm on June 30, 2021:

Done. Thanks!

in src/net_processing.cpp:475 in d5dd3fa1de outdated

472+     * transaction per day that would be inadvertently ignored (which is the
473+     * same probability that we have in the reject filter).
474      */
475     Mutex m_recent_confirmed_transactions_mutex;
476-    std::unique_ptr<CRollingBloomFilter> m_recent_confirmed_transactions GUARDED_BY(m_recent_confirmed_transactions_mutex);
477+    CRollingBloomFilter m_recent_confirmed_transactions GUARDED_BY(m_recent_confirmed_transactions_mutex) {48000, 0.000001};

hebasto commented at 1:54 am on June 18, 2021:

d5dd3fa1de2f96a7ddfd8b300829c68b93bd4607, style-nit:

0    CRollingBloomFilter m_recent_confirmed_transactions GUARDED_BY(m_recent_confirmed_transactions_mutex){48'000, 0.000001};

jnewbery commented at 4:22 pm on June 30, 2021:

Done

hebasto approved

hebasto commented at 1:59 am on June 18, 2021: member

ACK d5dd3fa1de2f96a7ddfd8b300829c68b93bd4607

While touching the net_processing.cpp, also a typo which was introduced in #22141 could be fixed: https://github.com/bitcoin/bitcoin/blob/5c2e2afe990ad6e1cc8b08513fa2d853d497dace/src/net_processing.cpp#L470

recieved ==> received

practicalswift commented at 7:41 pm on June 23, 2021: contributor

cr ACK d5dd3fa1de2f96a7ddfd8b300829c68b93bd4607

jnewbery force-pushed on Jun 30, 2021

jnewbery commented at 4:23 pm on June 30, 2021: member

Thanks for the reviews @MarcoFalke, @hebasto, @practicalswift. I’ve addressed all of the review comments from @hebasto so this is now ready for re-review.

While touching the net_processing.cpp, also a typo which was introduced in #22141 could be fixed:

I was too slow. It’s been fixed in #22323.

theStack approved

theStack commented at 11:43 am on July 20, 2021: member

Code-review ACK 214e91a31b483c19a32ca22962e117a3e2688f86

one stylistic/readability-improvement nit: could also use the digit separators (’) for the floating point numeral for the CRollingBloomFilter (valid since C++14 according to https://en.cppreference.com/w/cpp/language/floating_literal):

 0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index 25fbd33b5..772ea2ced 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -453,7 +453,7 @@ private:
 5      *
 6      * Memory used: 1.3 MB
 7      */
 8-    CRollingBloomFilter m_recent_rejects GUARDED_BY(::cs_main){120'000, 0.000001};
 9+    CRollingBloomFilter m_recent_rejects GUARDED_BY(::cs_main){120'000, 0.000'001};
10     uint256 hashRecentRejectsChainTip GUARDED_BY(cs_main);
11
12     /*
13@@ -472,7 +472,7 @@ private:
14      * same probability that we have in the reject filter).
15      */
16     Mutex m_recent_confirmed_transactions_mutex;
17-    CRollingBloomFilter m_recent_confirmed_transactions GUARDED_BY(m_recent_confirmed_transactions_mutex){48'000, 0.000001};
18+    CRollingBloomFilter m_recent_confirmed_transactions GUARDED_BY(m_recent_confirmed_transactions_mutex){48'000, 0.000'001};
19
20     /** Have we requested this block from a peer */
21     bool IsBlockRequested(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

[net processing] Add Orphanage empty consistency check

When removing the final peer, assert that m_tx_orphanage is empty.

9190b01d8d

[net processing] Default initialize m_stale_tip_check_time a28bfd1d4c

[net processing] Default initialize recentRejects

Now that recentRejects is owned by PeerManagerImpl, and
PeerManagerImpl's lifetime is managed by the node context, we can just
default initialize recentRejects during object initialization. We can
also remove the unique_ptr indirection.

cd9902ac50

scripted-diff: Rename recentRejects

-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }

ren recentRejects m_recent_rejects
-END VERIFY SCRIPT-

37dcd12d53

[net processing] Default initialize m_recent_confirmed_transactions

Now that m_recent_confirmed_transactions is owned by PeerManagerImpl,
and PeerManagerImpl's lifetime is managed by the node context, we can
just default initialize m_recent_confirmed_transactions during object
initialization. We can also remove the unique_ptr indirection.

fde1bf4f61

jnewbery force-pushed on Jul 20, 2021

jnewbery commented at 12:32 pm on July 20, 2021: member

Thanks for the review @theStack. I didn’t realize that the digit separator was permitted after the decimal point. I’ve taken your suggestion and rebased on master.

theStack approved

theStack commented at 3:17 pm on July 20, 2021: member

re-ACK fde1bf4f6136638e84cdf9806eedaae08e841bbf

checked via git range-diff 214e91a3...fde1bf4f that since my previous ACK, the only non-rebase-related change is using the digit separator also for floating points now, as suggested.

MarcoFalke commented at 2:30 pm on July 28, 2021: member

ACK fde1bf4f6136638e84cdf9806eedaae08e841bbf 👞

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK fde1bf4f6136638e84cdf9806eedaae08e841bbf 👞
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgIaAv+PZATwaBhcsf9/tfSEfMxRLP+BpK9c5vKV+URqEVG8wL+rV+vi/4Sdv5K
 8C7aNNt2AXAYZgqnIAvmsg1uL8G3uFZHHAo54ziHVjS7MlrG0+SSg5jKOerW/QjwA
 9Qb1BJqMP/ra4Jm7PHVcbGTjQg6Z0t3kJ+11jzqpVCRJ21NTPNwajBrrcpNKkdwHP
107sIKPVi5LLvK8V43gwGoAbEl9Dtwj/SfOIkaINpeZv8URYeKLYey3s3yjiO06wH1
119eXj+Sglww9GiPETOBuFMEMKcc9hCktK2oEHMv3U3d5cC12F9Dtjp1NazPNmBnvj
12FwNAVbP//HwTIv0oZ+7dsZ/SKNIgKlgXR8fAsKH0ZiD85vC1ZalobhVqkmrRMX86
13vW9ZP2Fj8X36Ez6vD+xRvy0cFMs+oQPPE4VLBnysmB5qT8uzcLSvSLTdwklW7FWK
14T8XSw4GuNv/l/R7lEARBrkzajpMxtxRxNzvI5Zb7mu5CQWTZxogQFbCji+Vnk7ok
15R+vr6Dyk
16=9Rfq
17-----END PGP SIGNATURE-----

Timestamp of file with hash d2de22e790c8247f5ed87c9b394b8c69ce8ccefa133cb79fea5a41603e328f92 -

MarcoFalke merged this on Jul 28, 2021

MarcoFalke closed this on Jul 28, 2021

jnewbery deleted the branch on Jul 28, 2021

sidhujag referenced this in commit 379c464b29 on Jul 28, 2021

DrahtBot locked this on Aug 18, 2022

[net processing] Various tidying up of PeerManagerImpl ctor #21562

Conflicts