[net] Tighten scope in net

skeees commented at 8:38 pm on June 7, 2018: contributor

As part of a larger effort to decouple net_processing and validation a bit, these are a bunch of simple scope cleanups. I’ve moved things out of the header file that are only referenced in net_processing and added static (or anonymous namespace) modifiers to everything possible in net_processing.

There are a handful of functions which could be static except that they are exposed for the sake of unit testing - these are explicitly commented. There has been some discussion of a compile time annotation, but no conclusion has been reached on that yet.

This is somewhat related to other prs #12934 #13413 #13407 and will be followed by prs that reduce reliance on cs_main to synchronize data structures which are translation unit local to net_processing

in src/net_processing.cpp:71 in b6a37ffbf7 outdated

81-    {
82-        return &(*a) < &(*b);
83-    }
84-};
85+//=============================================================================
86+/** The below declarations are not translation unit static for unit test use */

Empact commented at 8:59 pm on June 7, 2018:

/** is a doxygen trigger. Seems not needed for this and the below.

Empact commented at 9:00 pm on June 7, 2018: member

4950ae2 could be a scripted-diff

in src/net_processing.cpp:95 in f026cb265c outdated

72@@ -74,6 +73,9 @@ static const int HISTORICAL_BLOCK_AGE = 7 * 24 * 60 * 60;
73 
74 // Internal stuff
75 namespace {
76+    /** Enable BIP61 (sending reject messages) */
77+    bool enable_bip61 = DEFAULT_ENABLE_BIP61;

MarcoFalke commented at 9:22 pm on June 7, 2018:

I know the developer notes don’t clarify what “global” is, but unless you have a reason to remove the prefix, I’d suggest keeping it for now to make the diff smaller?

Empact commented at 9:33 pm on June 7, 2018:

This commit could also be broken into 1 minor move commit and one rename scripted-diff.

practicalswift commented at 9:59 pm on June 7, 2018: contributor

Concept ACK

DrahtBot commented at 5:20 pm on June 8, 2018: member

#13423 ([net] Thread safety annotations in net_processing by skeees)
#13407 ([refactor, move-only-ish] Refactor mempool accept/reject logic by skeees)
#13298 (Net: Bucketing INV delays (1 bucket) for incoming connections to hide tx time by naumenkogs)
#13083 (Add compile time checking for cs_main runtime locking assertions by practicalswift)

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.

skeees commented at 6:04 pm on June 8, 2018: contributor

per reviewer feedback I’ll reshape these commits to use more scripted-diffs

skeees force-pushed on Jun 8, 2018

in src/net_processing.cpp:79 in c4d7d6c9c0 outdated

89+void EraseOrphansFor(NodeId peer);
90+unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans);
91+/** Increase a node's misbehavior score. */
92+void Misbehaving(NodeId nodeid, int howmuch, const std::string& message="");
93+// This function is used for testing the stale tip eviction logic
94+void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds);

Empact commented at 8:10 pm on June 8, 2018:

It’s a bit strange to forward-declare these methods when the references aren’t to be used prior to the actual definition. Maybe comment each method instead?

jnewbery commented at 4:52 pm on June 14, 2018:

I agree that it seems a bit strange to forward-declare these functions here.

MarcoFalke commented at 11:09 pm on June 19, 2018:

Agree here as well. Could either just not mention it at all or add a comment in-line to each function?

skeees commented at 7:21 pm on June 20, 2018:

Now removed - only remaining forward declarations are the ones strictly necessary for compilation

Empact commented at 8:12 pm on June 8, 2018: member

utACK c4d7d6c

sipa commented at 1:28 am on June 10, 2018: member

I realize that C++ doesn’t really have a concept of ‘global’ for variables; it has storage duration (static, automatic, dynamic) and linkage (internal and external). So the guidelines are confusing here.

I would prefer to keep the g_ prefix for all objects with static storage duration, regardless of linkage. It just means “there is just one of these for the entire program”, whether or not it can accessed everywhere.

fanquake added the label P2P on Jun 10, 2018

promag commented at 9:34 am on June 10, 2018: member

What @sipa suggests is in line with @MarcoFalke suggestion (don’t remember PR) and

I would prefer to keep the g_ prefix for all objects with static storage duration, regardless of linkage. It just means “there is just one of these for the entire program”, whether or not it can accessed everywhere.

:+1:

in src/net_processing.cpp:896 in aad9abfd51 outdated

870@@ -867,7 +871,7 @@ static uint256 most_recent_block_hash;
871 static bool fWitnessesPresentInMostRecentCompactBlock;
872 
873 /**
874- * Maintain state about the best-seen block and fast-announce a compact block 
875+ * Maintain state about the best-seen block and fast-announce a compact block

promag commented at 9:36 am on June 10, 2018:

Commit “[net] Tighten scope in net_processing”

Could not include these changes.

skeees force-pushed on Jun 10, 2018

skeees commented at 12:48 pm on June 10, 2018: contributor

Per reviewer feedback I’ve dropped the commits that did the s/g_// rename

skeees force-pushed on Jun 10, 2018

DrahtBot commented at 4:51 pm on June 12, 2018: member

DrahtBot added the label Needs rebase on Jun 12, 2018

jnewbery commented at 4:53 pm on June 14, 2018: member

This needs a trivial rebase.

I’ve tested a rebased branch. Seems good.

skeees commented at 5:00 pm on June 14, 2018: contributor

Thanks. @jnewbery what commit did you rebase onto? I’ll use the same to preserve the validity of your testing

Rescope g_enable_bip61 to net_processing 02bbc05310

[move-only] Move things only referenced in net_processing out of header file 1d4df02b7e

skeees force-pushed on Jun 19, 2018

skeees commented at 5:30 pm on June 19, 2018: contributor

@DrahtBot - rebased

DrahtBot removed the label Needs rebase on Jun 19, 2018

MarcoFalke added the label Refactoring on Jun 19, 2018

MarcoFalke commented at 11:10 pm on June 19, 2018: member

Concept ACK 6f7239437b5a61c33178d3e53641b8d2e1487f79 beside previous review comments.

Tagged with refactoring since this shouldn’t change behavior.

skeees force-pushed on Jun 20, 2018

skeees commented at 7:20 pm on June 20, 2018: contributor

I personally disagree, but by popular demand the forward declarations are removed

jnewbery commented at 8:23 pm on June 20, 2018: member

Tested ACK a7fb7259dd53c37b1922a44de23abbb3b23129c6

in src/net_processing.cpp:68 in a7fb7259dd outdated

73-    bool operator()(const I& a, const I& b) const
74-    {
75-        return &(*a) < &(*b);
76-    }
77-};
78+static CCriticalSection g_cs_orphans;

MarcoFalke commented at 8:38 pm on June 20, 2018:

Any reason to move this line (and the void functions below)? I’d prefer to preserve what git blame tells and keep the diff minimal.

skeees commented at 9:28 pm on June 20, 2018:

artefact of the previous forward declarations - moved back to original locations now

Restrict as much as possible in net_processing to translation unit

Mark everything else static or in an anonymous namespace.

6690a28606

skeees force-pushed on Jun 20, 2018

jnewbery commented at 9:39 pm on June 20, 2018: member

ACK 6690a28606d6ada23355de7d9a9878f841b21e67

Confirm that the difference from a7fb725 is simply a code move.

MarcoFalke commented at 6:59 pm on June 21, 2018: member

utACK 6690a28606d6ada23355de7d9a9878f841b21e67 (Used --ignore-all-space --color-moved=dimmed_zebra options to git diff)

in src/net_processing.cpp:816 in 02bbc05310 outdated

812@@ -811,6 +813,8 @@ static bool BlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Para
813 }
814 
815 PeerLogicValidation::PeerLogicValidation(CConnman* connmanIn, CScheduler &scheduler) : connman(connmanIn), m_stale_tip_check_time(0) {
816+    g_enable_bip61 = gArgs.GetBoolArg("-enablebip61", DEFAULT_ENABLE_BIP61);

sipa commented at 2:36 am on June 22, 2018:

It seems strange to initialize a global inside an object’s constructor. There could be multiple PeerLogicValidation objects, and each will re-initialize the global.

skeees commented at 3:17 am on June 22, 2018:

This is true, but:

this global is only ever read from either methods in PeerLogic or static functions called by peer logic methods
its value doesn’t change post startup so if its read more than once it will always have the same value, reinitializing it a couple times isn’t really a performance issue either

some other options could be:

make it a member variable instead of a global (will have to pass it as an arg to the static functions which reference it)
just call GetBoolArg directly (this is done in a couple other situations)

either of the above alternate options would inflate the diff quite a bit. doing it this way seemed the most compact

and it seems like a worthwhile tradeoff to be able to eliminate the extern declaration. thoughts?

laanwj commented at 5:51 pm on July 9, 2018:

I agree this is strange, would also prefer initializing it somewhere outside the constructor OR making it an instance variable if possible.

skeees commented at 6:24 pm on July 9, 2018:

There’s no good place that’s local to the file but also out of the PeerLogicValidation constructor (i don’t think i call GetBoolArg safely in a static initializer)

I think that making it an instance variable is the right way to go - but its going to substantially increase the size of this diff because there are a bunch of functions invoked from PeerLogicValidation that are static to net_processing but not PeerLogicValidation methods so it will change the argument signature of all of those

If people don’t mind the member var i will go ahead and do that but it will substantially increase review burden for IMO not much benefit

skeees commented at 7:03 pm on July 9, 2018:

(here’s roughly what that diff would look like: https://github.com/skeees/bitcoin/tree/net_processing-disentangle-yugediff)

promag commented at 8:52 pm on July 9, 2018:

@skeees not that big, LGTM. +1 to make it an instance variable.

Empact commented at 10:55 pm on July 9, 2018:

I’m also into this. +1

skeees commented at 1:29 am on July 10, 2018:

if you don’t mind the larger review then ok! three +1s, PR updated

Make g_enable_bip61 a member variable of PeerLogicValidation 3339ba28e9

Empact commented at 4:36 am on July 10, 2018: member

nit: some unrelated whitespace changes in 02bbc05

utACK 3339ba2

sipa commented at 10:36 pm on July 13, 2018: member

utACK 3339ba28e95aaaa355b7d33a69cffca7ab29b3fd

sipa merged this on Jul 14, 2018

sipa closed this on Jul 14, 2018

sipa referenced this in commit 1e90862f5d on Jul 14, 2018

laanwj referenced this in commit f58674a20a on Jul 26, 2018

PastaPastaPasta referenced this in commit 5c6fb6e75c on Apr 15, 2020

PastaPastaPasta referenced this in commit 614a0bbd78 on Apr 15, 2020

PastaPastaPasta referenced this in commit a3f0f3cf63 on Apr 16, 2020

PastaPastaPasta referenced this in commit 76d2e73d79 on Apr 16, 2020

PastaPastaPasta referenced this in commit 504b696b78 on Apr 16, 2020

PastaPastaPasta referenced this in commit b4d0e1149f on Apr 16, 2020

PastaPastaPasta referenced this in commit 7e588c7594 on Apr 19, 2020

PastaPastaPasta referenced this in commit 0fab32de36 on Apr 19, 2020

PastaPastaPasta referenced this in commit c7f131cb64 on Apr 20, 2020

PastaPastaPasta referenced this in commit 6cce7b22db on May 10, 2020

PastaPastaPasta referenced this in commit aeff62a7ad on May 10, 2020

PastaPastaPasta referenced this in commit abc246a698 on May 12, 2020

PastaPastaPasta referenced this in commit 10d9608654 on May 12, 2020

PastaPastaPasta referenced this in commit 5b72c199ff on Jun 9, 2020

PastaPastaPasta referenced this in commit 19ac12e516 on Jun 9, 2020

PastaPastaPasta referenced this in commit 9c6d174c4d on Jun 9, 2020

PastaPastaPasta referenced this in commit 0a2fb48943 on Jun 9, 2020

PastaPastaPasta referenced this in commit cfac0a9f3f on Jun 10, 2020

PastaPastaPasta referenced this in commit bcb03327d1 on Jun 10, 2020

PastaPastaPasta referenced this in commit c26f14a99c on Jun 11, 2020

PastaPastaPasta referenced this in commit 646b57c07b on Jun 11, 2020

DrahtBot locked this on Feb 15, 2022

[net] Tighten scope in net_processing #13417