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

jnewbery · 2021-03-31T17:58:42Z

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

fanquake · 2021-04-01T02:12:36Z

Cirrus failures:

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

DrahtBot · 2021-04-01T04:25:58Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: Implicitly sync after generate* to preempt races and intermittent test failures #22567 by MarcoFalke
test: Implicitly sync after generate* to preempt races and intermittent test failures #20362 by MarcoFalke
test: Extend stale tip test #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.

practicalswift · 2021-04-01T09:14:50Z

Strong Concept ACK

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

maflcko · 2021-04-01T14:00:03Z

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 · 2021-04-01T15:27:44Z

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

hebasto · 2021-04-02T22:55:33Z

@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 · 2021-04-06T15:57:38Z

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.

hebasto · 2021-04-11T06:53:00Z

src/net_processing.cpp

+PeerManagerImpl::~PeerManagerImpl()
+{
+    // Do consistency checks on shutdown
+    LOCK(cs_main);


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

DrahtBot · 2021-05-03T09:32:45Z

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

jnewbery · 2021-05-03T11:20:43Z

@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.

Crypt-iQ · 2021-05-09T00:58:28Z

Code review ACK b26a5ad

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

jnewbery · 2021-05-10T06:45:01Z

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 · 2021-05-12T15:34:15Z

Code review ACK d6cef4d

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

jnewbery · 2021-06-02T15:12:15Z

Rebased on master to fix fuzz CI timeout.

Crypt-iQ · 2021-06-05T21:40:50Z

Code review ACK 93e881f

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

jnewbery · 2021-06-14T09:23:52Z

Rebased.

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

Fixed. Thank you!

maflcko · 2021-06-14T16:44:56Z

review ACK d5dd3fa 🔠

Show signature and timestamp

Signature:

Timestamp of file with hash cb3d358ad6eccc7781819f9917bb2f9a6f7b802e636f39fe7a8d779c985d5dfc -

hebasto

ACK d5dd3fa

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

bitcoin/src/net_processing.cpp

Line 470 in 5c2e2af

* - the block has been recieved from a peer

recieved ==> received

src/txorphanage.h

src/net_processing.cpp

practicalswift · 2021-06-23T19:41:52Z

cr ACK d5dd3fa

jnewbery · 2021-06-30T16:23:17Z

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

Code-review ACK 214e91a

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):

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 25fbd33b5..772ea2ced 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -453,7 +453,7 @@ private:
      *
      * Memory used: 1.3 MB
      */
-    CRollingBloomFilter m_recent_rejects GUARDED_BY(::cs_main){120'000, 0.000001};
+    CRollingBloomFilter m_recent_rejects GUARDED_BY(::cs_main){120'000, 0.000'001};
     uint256 hashRecentRejectsChainTip GUARDED_BY(cs_main);

     /*
@@ -472,7 +472,7 @@ private:
      * same probability that we have in the reject filter).
      */
     Mutex m_recent_confirmed_transactions_mutex;
-    CRollingBloomFilter m_recent_confirmed_transactions GUARDED_BY(m_recent_confirmed_transactions_mutex){48'000, 0.000001};
+    CRollingBloomFilter m_recent_confirmed_transactions GUARDED_BY(m_recent_confirmed_transactions_mutex){48'000, 0.000'001};

     /** Have we requested this block from a peer */
     bool IsBlockRequested(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

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

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.

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

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.

jnewbery · 2021-07-20T12:32:55Z

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

re-ACK fde1bf4

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.

maflcko · 2021-07-28T14:30:20Z

ACK fde1bf4 👞

Show signature and timestamp

Signature:

Timestamp of file with hash d2de22e790c8247f5ed87c9b394b8c69ce8ccefa133cb79fea5a41603e328f92 -

…erImpl ctor fde1bf4 [net processing] Default initialize m_recent_confirmed_transactions (John Newbery) 37dcd12 scripted-diff: Rename recentRejects (John Newbery) cd9902a [net processing] Default initialize recentRejects (John Newbery) a28bfd1 [net processing] Default initialize m_stale_tip_check_time (John Newbery) 9190b01 [net processing] Add Orphanage empty consistency check (John Newbery) Pull request description: - Use default initialization of PeerManagerImpl members where possible - Remove unique_ptr indirection where it's not needed ACKs for top commit: MarcoFalke: ACK fde1bf4 👞 theStack: re-ACK fde1bf4 Tree-SHA512: 7ddedcc972df8e933e1fbe5c88b8ea17df89e1e58fc769518512c5540e49dc8eddb3f47e78d1329a6fc5644d2c1d11c981f681fd633f5218bfa4b3e6a86f3d7b

DrahtBot added the P2P label Mar 31, 2021

DrahtBot mentioned this pull request Apr 1, 2021

[p2p] Introduce node rebroadcast module #21061

Closed

jnewbery force-pushed the 2021-03-peer-manager-impl-ctor-dtor branch from 94b5fea to b458400 Compare April 1, 2021 08:25

jnewbery force-pushed the 2021-03-peer-manager-impl-ctor-dtor branch from b458400 to 44c05f7 Compare April 1, 2021 10:33

jnewbery marked this pull request as ready for review April 1, 2021 13:49

jnewbery closed this Apr 1, 2021

jnewbery reopened this Apr 1, 2021

jnewbery force-pushed the 2021-03-peer-manager-impl-ctor-dtor branch from 44c05f7 to a1f2f65 Compare April 1, 2021 15:27

hebasto reviewed Apr 11, 2021

View reviewed changes

jnewbery force-pushed the 2021-03-peer-manager-impl-ctor-dtor branch from a1f2f65 to b26a5ad Compare May 3, 2021 11:18

jnewbery changed the title ~~[net processing] Various tidying up of PeerManagerImpl ctor and dtor~~ [net processing] Various tidying up of PeerManagerImpl ctor May 3, 2021

bitcoin deleted a comment from 78051301012 May 3, 2021

DrahtBot mentioned this pull request May 6, 2021

[Bundle 7/7] validation: Farewell, global Chainstate! #21866

Merged

jnewbery force-pushed the 2021-03-peer-manager-impl-ctor-dtor branch from b26a5ad to d6cef4d Compare May 10, 2021 06:42

jnewbery force-pushed the 2021-03-peer-manager-impl-ctor-dtor branch from d6cef4d to 93e881f Compare June 2, 2021 15:12

jnewbery mentioned this pull request Jun 7, 2021

qa: Another intermittent failure in p2p_compactblocks.py #21535

Closed

DrahtBot mentioned this pull request Jun 10, 2021

test: Extend stale tip test #18470

Closed

DrahtBot added the Needs rebase label Jun 12, 2021

jnewbery force-pushed the 2021-03-peer-manager-impl-ctor-dtor branch from 93e881f to d5dd3fa Compare June 14, 2021 09:23

DrahtBot removed the Needs rebase label Jun 14, 2021

hebasto approved these changes Jun 18, 2021

View reviewed changes

src/txorphanage.h Outdated Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jun 24, 2021

test: Implicitly sync after generate* to preempt races and intermittent test failures #20362

Closed

jnewbery force-pushed the 2021-03-peer-manager-impl-ctor-dtor branch from d5dd3fa to 214e91a Compare June 30, 2021 16:22

theStack approved these changes Jul 20, 2021

View reviewed changes

jnewbery added 5 commits July 20, 2021 13:12

[net processing] Add Orphanage empty consistency check

9190b01

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

[net processing] Default initialize m_stale_tip_check_time

a28bfd1

[net processing] Default initialize recentRejects

cd9902a

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.

scripted-diff: Rename recentRejects

37dcd12

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

jnewbery force-pushed the 2021-03-peer-manager-impl-ctor-dtor branch from 214e91a to fde1bf4 Compare July 20, 2021 12:31

theStack approved these changes Jul 20, 2021

View reviewed changes

DrahtBot mentioned this pull request Jul 28, 2021

test: Implicitly sync after generate* to preempt races and intermittent test failures #22567

Closed

maflcko merged commit 67b9416 into bitcoin:master Jul 28, 2021

jnewbery deleted the 2021-03-peer-manager-impl-ctor-dtor branch July 28, 2021 14:52

ajtowns mentioned this pull request Sep 2, 2021

net_processing: lock clean up #21527

Closed

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

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

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

Uh oh!

Conversation

jnewbery commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Apr 1, 2021

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

practicalswift commented Apr 1, 2021

Uh oh!

maflcko commented Apr 1, 2021

Uh oh!

jnewbery commented Apr 1, 2021

Uh oh!

hebasto commented Apr 2, 2021

Uh oh!

jnewbery commented Apr 6, 2021

Uh oh!

hebasto Apr 11, 2021

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented May 3, 2021

Uh oh!

jnewbery commented May 3, 2021

Uh oh!

Crypt-iQ commented May 9, 2021

Uh oh!

jnewbery commented May 10, 2021

Uh oh!

Crypt-iQ commented May 12, 2021

Uh oh!

jnewbery commented Jun 2, 2021

Uh oh!

Crypt-iQ commented Jun 5, 2021

Uh oh!

jnewbery commented Jun 14, 2021

Uh oh!

maflcko commented Jun 14, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Jun 23, 2021

Uh oh!

jnewbery commented Jun 30, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jul 20, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 28, 2021

Uh oh!

Uh oh!

jnewbery commented Mar 31, 2021 •

edited

Loading

DrahtBot commented Apr 1, 2021 •

edited

Loading