[net] Tighten scope in net_processing by skeees · Pull Request #13417 · bitcoin/bitcoin

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

/** 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

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

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

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 <[email protected]> # 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

Empact reviewed Jun 7, 2018

View reviewed changes

src/net_processing.cpp Outdated

Copy link

Contributor

Empact Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

maflcko reviewed Jun 7, 2018

View reviewed changes

This was referenced Jun 8, 2018

[net,mempool] Call AcceptToMemoryPool() asynchronously in p2p #13413

Closed

[refactor, move-only-ish] Refactor mempool accept/reject logic #13407

Closed

skeees mentioned this pull request Jun 8, 2018

[net] Thread safety annotations in net_processing #13423

Merged

skeees force-pushed the net_processing-disentangle branch 2 times, most recently from f3574c1 to c4d7d6c Compare June 8, 2018 19:23

Empact reviewed Jun 8, 2018

View reviewed changes

fanquake added the P2P label Jun 10, 2018

promag reviewed Jun 10, 2018

View reviewed changes

src/net_processing.cpp Outdated

Copy link

Contributor

promag Jun 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit "[net] Tighten scope in net_processing"

Could not include these changes.

skeees force-pushed the net_processing-disentangle branch from c4d7d6c to a7502b8 Compare June 10, 2018 12:44

skeees force-pushed the net_processing-disentangle branch 2 times, most recently from ab47453 to e7d2f78 Compare June 10, 2018 13:01

DrahtBot added the Needs rebase label Jun 12, 2018

skeees added 2 commits June 19, 2018 13:02

Rescope g_enable_bip61 to net_processing

02bbc05

[move-only] Move things only referenced in net_processing out of head…

1d4df02

…er file

skeees force-pushed the net_processing-disentangle branch from e7d2f78 to 6f72394 Compare June 19, 2018 17:26

DrahtBot removed the Needs rebase label Jun 19, 2018

Make g_enable_bip61 a member variable of PeerLogicValidation

3339ba2

sipa merged commit 3339ba2 into bitcoin:master Jul 14, 2018

Bushstar mentioned this pull request Jul 16, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#355

Merged

HashUnlimited mentioned this pull request Sep 12, 2018

[net] Thread safety annotations in net_processing chaincoin/chaincoin#581

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

skeees commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Jun 7, 2018

Uh oh!

DrahtBot commented Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skeees commented Jun 8, 2018

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!

Empact commented Jun 8, 2018

Uh oh!

sipa commented Jun 10, 2018

Uh oh!

promag commented Jun 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skeees commented Jun 10, 2018

Uh oh!

DrahtBot commented Jun 12, 2018

Uh oh!

jnewbery commented Jun 14, 2018

Uh oh!

skeees commented Jun 14, 2018

Uh oh!

skeees commented Jun 19, 2018

Uh oh!

Empact commented Jul 10, 2018

Uh oh!

sipa commented Jul 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

skeees commented Jun 7, 2018 •

edited

Loading

DrahtBot commented Jun 8, 2018 •

edited

Loading