[net] Tighten scope in net_processing #13417

skeees · 2018-06-07T20:38:42Z

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

Empact · 2018-06-07T20:59:15Z

src/net_processing.cpp

-    }
-};
+//=============================================================================
+/** The below declarations are not translation unit static for unit test use */


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

Empact · 2018-06-07T21:00:43Z

4950ae2 could be a scripted-diff

maflcko · 2018-06-07T21:22:20Z

src/net_processing.cpp

@@ -74,6 +73,9 @@ static const int HISTORICAL_BLOCK_AGE = 7 * 24 * 60 * 60;

 // Internal stuff
 namespace {
+    /** Enable BIP61 (sending reject messages) */
+    bool enable_bip61 = DEFAULT_ENABLE_BIP61;


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?

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

practicalswift · 2018-06-07T21:59:01Z

Concept ACK

DrahtBot · 2018-06-08T17:20:38Z

Note to reviewers: This pull request conflicts with the following ones:

[net] Thread safety annotations in net_processing #13423 ([net] Thread safety annotations in net_processing by skeees)
[refactor, move-only-ish] Refactor mempool accept/reject logic #13407 ([refactor, move-only-ish] Refactor mempool accept/reject logic by skeees)
Net: Bucketing INV delays (1 bucket) for incoming connections to hide tx time #13298 (Net: Bucketing INV delays (1 bucket) for incoming connections to hide tx time by naumenkogs)
Add compile time checking for cs_main runtime locking assertions #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 · 2018-06-08T18:04:15Z

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

Empact · 2018-06-08T20:10:47Z

src/net_processing.cpp

+/** Increase a node's misbehavior score. */
+void Misbehaving(NodeId nodeid, int howmuch, const std::string& message="");
+// This function is used for testing the stale tip eviction logic
+void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds);


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?

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

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

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

Empact · 2018-06-08T20:12:25Z

utACK c4d7d6c

sipa · 2018-06-10T01:28:14Z

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.

promag · 2018-06-10T09:34:56Z

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.

👍

promag · 2018-06-10T09:36:49Z

src/net_processing.cpp

@@ -867,7 +871,7 @@ static uint256 most_recent_block_hash;
 static bool fWitnessesPresentInMostRecentCompactBlock;

 /**
- * Maintain state about the best-seen block and fast-announce a compact block 
+ * Maintain state about the best-seen block and fast-announce a compact block


Commit "[net] Tighten scope in net_processing"

Could not include these changes.

skeees · 2018-06-10T12:48:11Z

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

DrahtBot · 2018-06-12T16:51:14Z

Needs rebase

jnewbery · 2018-06-14T16:53:57Z

This needs a trivial rebase.

I've tested a rebased branch. Seems good.

skeees · 2018-06-14T17:00:13Z

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

…er file

skeees · 2018-06-19T17:30:14Z

@DrahtBot - rebased

Empact · 2018-07-10T04:36:14Z

nit: some unrelated whitespace changes in 02bbc05

utACK 3339ba2

sipa · 2018-07-13T22:36:04Z

utACK 3339ba2

3339ba2 Make g_enable_bip61 a member variable of PeerLogicValidation (Jesse Cohen) 6690a28 Restrict as much as possible in net_processing to translation unit (Jesse Cohen) 1d4df02 [move-only] Move things only referenced in net_processing out of header file (Jesse Cohen) 02bbc05 Rescope g_enable_bip61 to net_processing (Jesse Cohen) Pull request description: 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 Tree-SHA512: 46c9660ee4e06653feb42ba92189565b0aea17aac2375c20747c0d091054c63829cbf66d2daddf65682b58ce1d6922e23aefea051a7f2c8abbb6db253a609082

1e3bcd2 [net_processing] Add thread safety annotations (Jesse Cohen) f393a53 Annotate AssertLockHeld() with ASSERT_CAPABILITY() for thread safety analysis (Jesse Cohen) Pull request description: (note that this depends on #13417) This commit fully annotates all globals in net_processing with clang thread safety annotations. Subsequent commits will begin transitioning some of this data away from cs_main into locks that are local to net_processing. Static thread safety analysis should it easier to verify correctness of that process. Tree-SHA512: b47aa410cb9ada21072370176aea9a74c575643fa1ee8cf1d43c8e28675eef17f33e5242ac422f840e8178e132ecb58412034c6334b68f1b57c686df80d4e8e2

3339ba2 Make g_enable_bip61 a member variable of PeerLogicValidation (Jesse Cohen) 6690a28 Restrict as much as possible in net_processing to translation unit (Jesse Cohen) 1d4df02 [move-only] Move things only referenced in net_processing out of header file (Jesse Cohen) 02bbc05 Rescope g_enable_bip61 to net_processing (Jesse Cohen) Pull request description: 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 bitcoin#12934 bitcoin#13413 bitcoin#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 Tree-SHA512: 46c9660ee4e06653feb42ba92189565b0aea17aac2375c20747c0d091054c63829cbf66d2daddf65682b58ce1d6922e23aefea051a7f2c8abbb6db253a609082 Signed-off-by: Pasta <pasta@dashboost.org> # Conflicts: # src/init.cpp # src/net_processing.cpp # src/net_processing.h # src/test/test_dash.cpp

1e3bcd2 [net_processing] Add thread safety annotations (Jesse Cohen) f393a53 Annotate AssertLockHeld() with ASSERT_CAPABILITY() for thread safety analysis (Jesse Cohen) Pull request description: (note that this depends on bitcoin#13417) This commit fully annotates all globals in net_processing with clang thread safety annotations. Subsequent commits will begin transitioning some of this data away from cs_main into locks that are local to net_processing. Static thread safety analysis should it easier to verify correctness of that process. Tree-SHA512: b47aa410cb9ada21072370176aea9a74c575643fa1ee8cf1d43c8e28675eef17f33e5242ac422f840e8178e132ecb58412034c6334b68f1b57c686df80d4e8e2