[p2p] Add Peer struct for per-peer data in net processing #19607

jnewbery · 2020-07-28T09:16:57Z

We currently have two structures for per-peer data:

CNode in net, which should just contain connection layer data (eg socket, send/recv buffers, etc), but currently also contains some application layer data (eg tx/block inventory).
CNodeState in net processing, which contains p2p application layer data, but requires cs_main to be locked for access.

This PR adds a third struct Peer, which is for p2p application layer data, and doesn't require cs_main. Eventually all application layer data from CNode should be moved to Peer, and any data that doesn't strictly require cs_main should be moved from CNodeState to Peer (probably all of CNodeState eventually).

Peer objects are stored as shared pointers in a net processing global map g_peer_map, which is protected by g_peer_mutex. To use a Peer object, g_peer_mutex is locked, a copy of the shared pointer is taken, and the lock is released. Individual members of Peer are protected by different mutexes that guard related data. The lifetime of the Peer object is managed by the shared_ptr refcount.

This PR adds the Peer object and moves the misbehaving data from CNodeState to Peer. This allows us to immediately remove 15 LOCK(cs_main) instances.

For more motivation see #19398

jnewbery · 2020-07-28T09:24:29Z

Thank you @MarcoFalke @promag @jonatack @dongcarl @ariard @theuni @fjahr @hebasto @troygiorshev for reviewing the prerequisite PRs! This is the first PR in the cs_main_split sequence where we start getting benefit by reducing cs_main scope in net processing. It also introduces the new structure and locking model, so it's the PR that requires the most conceptual/approach review in the sequence. If this gets merged, then it's a long procession of PRs to slowly move data into the new Peer object.

maflcko

Approach ACK 3fa130d, only some style-nits

src/net_processing.cpp

DrahtBot · 2020-07-28T17:04:52Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

[net processing] Move Misbehaving() to PeerManager #19791 ([net processing] Move Misbehaving() to PeerManager by jnewbery)
Ignore unknown messages before VERACK #19723 (Ignore unknown messages before VERACK by sdaftuar)

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.

troygiorshev

tACK 3fa130d with some questions and suggestions.

Is the plan to lock every member of Peer behind its own mutex? Why not give each Peer its own mutex instead?
Misbehaving's cs_main locks can be removed from denialofservice_tests.cpp: around here. I looked and couldn't find any further places.
clang-format complains a bit on each commit.

src/net_processing.cpp

jnewbery · 2020-07-29T08:34:57Z

Is the plan to lock every member of Peer behind its own mutex? Why not give each Peer its own mutex instead?

Currently the plan is to have related members behind a single mutex, eg all tx relay fields behind the same mutex. The main reason is that that model maps most closely to what exists already (look at the various locks inside CNode). Once everything has moved to Peer then it should be much easier to change the locking model, but I didn't want to add a debate about what the ideal locking model is to this project.

Misbehaving's cs_main locks can be removed from denialofservice_tests.cpp: around here. I looked and couldn't find any further places.

Fixed. Thanks!

clang-format complains a bit on each commit.

Can you expand a bit. What am I supposed to be looking for here?

jnewbery · 2020-07-29T13:48:06Z

clang-format complains a bit on each commit.

ok, I've taken clang-format's suggestions, except one that joins an initializer list onto the same line as the function and parameter list (which makes a really long line), and one which would break the scripted-diff linter.

jonatack · 2020-07-30T06:55:46Z

@jnewbery it looks like there is an incomplete rebase-in-progress in the last commit 123bba3; will review on next push.

jnewbery · 2020-07-30T08:23:33Z

@jonatack oops. Fixed. Thanks!

jonatack

ACK afe3604

Some questions below, feel free to ignore.

src/net_processing.cpp

jnewbery · 2020-07-30T11:41:22Z

Thanks for the review @jonatack and @MarcoFalke . All your latest review comments are addressed.

troygiorshev · 2020-07-30T12:11:55Z

reACK 9510938

laanwj · 2020-07-30T14:49:49Z

Code review ACK 8e35bf5
~~9510938~~

@theuni As you made the CNode and CNodeState separation originally can you take a look here please.

jonatack · 2020-07-30T15:23:52Z

re-ACK 9510938 per git diff afe3604 9510938

fjahr · 2020-07-30T19:46:55Z

Code review ACK 9510938

Still testing a bit but everything looks good!

jnewbery · 2020-07-30T20:04:42Z

Note for maintainers: please don't merge this yet. It has 4 ACKs, but going down this path is quite an important design decision. I'd like @theuni, @sipa or @sdaftuar to weight in before we move forward.

ariard

Approach ACK 9510938

I really like the approach to gather members by logical usage and progressively swap their cs_main guard by a new per-structure one. I thought at first, it would add new lock dependencies if we moved everything from CNodeState directly into Peer as a quick read of related issue description made it thought but in fact no.

Perhaps #19398 should laid out more the lock refactoring strategy and compare with any concurrent one (wasn't this discussed on IRC a while ago ?). It may help to take a decision here.

I glanced back on #18963, it adds also a CPeerState, aiming also to dry-up CNodeState but as branch tip was only used to signal end of validation processing back to source peer and resume its message processing. I think this current approach is somehow concurrent to #18963 but not incompatible with its end-goal of introducing asynchronous headers/block processing. Once this refactoring is done, it would be easier to reason on adding a new async "validation" thread, still challenging but easier.

src/net_processing.cpp

jonatack

ACK 8e35bf5 keeping in mind Cory's comment (#19607 (comment)) for the follow-up

sipa · 2020-08-28T18:16:10Z

Concept ACK.

Suggested here: bitcoin#19607 (comment)

…rocessing 8e35bf5 scripted-diff: rename misbehavior members (John Newbery) 1f96d2e [net processing] Move misbehavior tracking state to Peer (John Newbery) 7cd4159 [net processing] Add Peer (John Newbery) aba0335 [net processing] Remove CNodeState.name (John Newbery) Pull request description: We currently have two structures for per-peer data: - `CNode` in net, which should just contain connection layer data (eg socket, send/recv buffers, etc), but currently also contains some application layer data (eg tx/block inventory). - `CNodeState` in net processing, which contains p2p application layer data, but requires cs_main to be locked for access. This PR adds a third struct `Peer`, which is for p2p application layer data, and doesn't require cs_main. Eventually all application layer data from `CNode` should be moved to `Peer`, and any data that doesn't strictly require cs_main should be moved from `CNodeState` to `Peer` (probably all of `CNodeState` eventually). `Peer` objects are stored as shared pointers in a net processing global map `g_peer_map`, which is protected by `g_peer_mutex`. To use a `Peer` object, `g_peer_mutex` is locked, a copy of the shared pointer is taken, and the lock is released. Individual members of `Peer` are protected by different mutexes that guard related data. The lifetime of the `Peer` object is managed by the shared_ptr refcount. This PR adds the `Peer` object and moves the misbehaving data from `CNodeState` to `Peer`. This allows us to immediately remove 15 `LOCK(cs_main)` instances. For more motivation see bitcoin#19398 ACKs for top commit: laanwj: Code review ACK 8e35bf5 troygiorshev: reACK 8e35bf5 via `git range-diff master 9510938 8e35bf5` theuni: ACK 8e35bf5. jonatack: ACK 8e35bf5 keeping in mind Cory's comment (bitcoin#19607 (comment)) for the follow-up Tree-SHA512: ad84a92b78fb34c9f43813ca3dfbc7282c887d55300ea2ce0994d134da3e0c7dbc44d54380e00b13bb75a57c28857ac3236bea9135467075d78026767a19e4b1

Suggested here: bitcoin#19607 (comment)

bb6a32c [net processing] Move Misbehaving() to PeerManager (John Newbery) aa114b1 [net_processing] Move SendBlockTransactions into PeerManager (John Newbery) 3115e00 [net processing] Move MaybePunishPeerForTx to PeerManager (John Newbery) e662e2d [net processing] Move ProcessOrphanTx to PeerManager (John Newbery) b70cd89 [net processing] Move MaybePunishNodeForBlock into PeerManager (John Newbery) d777835 [net processing] Move ProcessHeadersMessage to PeerManager (John Newbery) 64f6162 [whitespace] tidy up indentation after scripted diff (John Newbery) 58bd369 scripted-diff: [net processing] Rename PeerLogicValidation to PeerManager (John Newbery) 2297b26 [net_processing] Pass chainparams to PeerLogicValidation constructor (John Newbery) 824bbd1 [move only] Collect all private members of PeerLogicValidation together (John Newbery) Pull request description: Continues the work of moving net_processing logic into PeerLogicValidation. See bitcoin/bitcoin#19704 and bitcoin/bitcoin#19607 (comment) for motivation. This PR also renames `PeerLogicValidation` to `PeerManager` as suggested in bitcoin/bitcoin#10756 (review). ACKs for top commit: MarcoFalke: re-ACK bb6a32c only change is rebase due to conflict in struct NodeContext and variable rename 🤸 hebasto: re-ACK bb6a32c, only rebased, and added renaming `s/peer_logic/peerman/` into scripted-diff since my [previous](bitcoin/bitcoin#19791 (review)) review (verified with `git range-diff`). Tree-SHA512: a2de4a521688fd25125b401e5575402c52b328a0fa27b3010567008d4f596b960aabbd02b2d81f42658f88f4365443fadb1008150a62fbcea123fb42d85a2c21

Suggested here: bitcoin#19607 (comment)

Suggested here: - bitcoin#19607 (comment) - bitcoin#19829 (comment)

…t_processing 3002b4a [net processing] Guard m_continuation_block with m_block_inv_mutex (John Newbery) 184557e [net processing] Move hashContinue to net processing (John Newbery) c853ef0 scripted-diff: rename vBlockHashesToAnnounce and vInventoryBlockToSend (John Newbery) 53b7ac1 [net processing] Move block inventory data to Peer (John Newbery) 78040f9 [net processing] Rename nStartingHeight to m_starting_height (John Newbery) 77a2c2f [net processing] Move nStartingHeight to Peer (John Newbery) 717a374 [net processing] Improve documentation for Peer destruction/locking (John Newbery) Pull request description: This continues the work of moving application layer data into net_processing, by moving all block inventory state into the new Peer object added in bitcoin#19607. For motivation, see bitcoin#19398. ACKs for top commit: jonatack: re-ACK 3002b4a per `git diff 9aad3e4 3002b4a` Sjors: Code review re-ACK 3002b4a MarcoFalke: re-ACK 3002b4a 🌓 Tree-SHA512: eb2b474b73b025791ee3e6e41809926b332b48468763219f31638ca390f427632f05902dfc6a2c6bdc1ce47b215782f67874ddbf05b97d77d5897b7e2abfe4d9

Suggested here: - bitcoin#19607 (comment) - bitcoin#19829 (comment)

Summary: ``` We currently have two structures for per-peer data: CNode in net, which should just contain connection layer data (eg socket, send/recv buffers, etc), but currently also contains some application layer data (eg tx/block inventory). CNodeState in net processing, which contains p2p application layer data, but requires cs_main to be locked for access. This PR adds a third struct Peer, which is for p2p application layer data, and doesn't require cs_main. Eventually all application layer data from CNode should be moved to Peer, and any data that doesn't strictly require cs_main should be moved from CNodeState to Peer (probably all of CNodeState eventually). Peer objects are stored as shared pointers in a net processing global map g_peer_map, which is protected by g_peer_mutex. To use a Peer object, g_peer_mutex is locked, a copy of the shared pointer is taken, and the lock is released. Individual members of Peer are protected by different mutexes that guard related data. The lifetime of the Peer object is managed by the shared_ptr refcount. This PR adds the Peer object and moves the misbehaving data from CNodeState to Peer. This allows us to immediately remove 15 LOCK(cs_main) instances. For more motivation see #19398 ``` Backport of core [[bitcoin/bitcoin#19607 | PR19607]]. Depends on D8793. Includes changes from our codebase that don't exist in core. Test Plan: With Clang and Debug: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D8794

a5e15ae scripted-diff: rename ping members (John Newbery) 45dcf22 [net processing] Move ping data fields to net processing (John Newbery) dd2646d [net processing] Move ping timeout logic to net processing (John Newbery) 0b43b81 [net processing] Move send ping message logic into function (John Newbery) 1a07600 [net] Add RunInactivityChecks() (John Newbery) f8b3058 [net processing] Add Peer& arg to MaybeDiscourageAndDisconnect() (John Newbery) Pull request description: This continues the work of moving application layer data into net_processing, by moving all ping data into the new Peer object added in bitcoin#19607. For motivation, see bitcoin#19398. ACKs for top commit: glozow: reACK bitcoin@a5e15ae MarcoFalke: review ACK a5e15ae 🥉 amitiuttarwar: ACK a5e15ae Tree-SHA512: fb84241613d6a6e1f2832fa5378030b5877a02e8308188f57ab545a6eaf2ab731a93abb7dcd3a7f7285bb66700f938096378a8e90cd6a3e6f3309f81d85a344e

0829516 [refactor] Remove unused ForEachNodeThen() template (John Newbery) 09cc66c scripted-diff: rename address relay fields (John Newbery) 76568a3 [net processing] Move addr relay data and logic into net processing (John Newbery) caba7ae [net processing] Make RelayAddress() a member function of PeerManagerImpl (John Newbery) 86acc96 [net processing] Take NodeId instead of CNode* as originator for RelayAddress() (John Newbery) Pull request description: This continues the work of moving application layer data into net_processing, by moving all addr data into the new Peer object added in #19607. For motivation, see #19398. ACKs for top commit: laanwj: Code review ACK 0829516 mzumsande: ACK 0829516, reviewed the code and ran tests. sipa: utACK 0829516 hebasto: re-ACK 0829516 Tree-SHA512: efe0410fac288637f203eb37d1999910791e345872d37e1bd5cde50e25bb3cb1c369ab86b3a166ffd5e06ee72e4508aa2c46d658be6a54e20b4f220d2f57d0a6

1066d10 scripted-diff: rename TxRelay members (John Newbery) 575bbd0 [net processing] Move tx relay data to Peer (John Newbery) 785f55f [net processing] Move m_wtxid_relay to Peer (John Newbery) 3634670 [net] Add CNode.m_relays_txs and CNode.m_bloom_filter_loaded (John Newbery) Pull request description: This continues the work of moving application layer data into net_processing, by moving all tx data into the new Peer object added in #19607. For motivation, see #19398. ACKs for top commit: dergoegge: ACK 1066d10 - This is a good layer separation improvement with no behavior changes. glozow: utACK 1066d10 Tree-SHA512: 0c9d6b8a0a05e2d816b6d6588b7df133842ec960ae67667813422aa7bd8eb5308599c714f3822a98ddbdf364ffab9050b055079277ba4aff24092557ff99ebcc

jnewbery mentioned this pull request Jul 28, 2020

Move remaining application layer data to net processing #19398

Closed

16 tasks

fanquake added the P2P label Jul 28, 2020

jnewbery force-pushed the 2020-07-peer branch from 15c0437 to 3fa130d Compare July 28, 2020 09:29

maflcko reviewed Jul 28, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

troygiorshev reviewed Jul 28, 2020

View reviewed changes

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 Jul 28, 2020

Erlay: bandwidth-efficient transaction relay protocol #18261

Closed

maflcko reviewed Jul 29, 2020

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

jnewbery force-pushed the 2020-07-peer branch 2 times, most recently from 8f8d5b0 to 123bba3 Compare July 29, 2020 13:47

DrahtBot mentioned this pull request Jul 29, 2020

[RFC] Package-relay: sender-initiated #19621

Closed

jnewbery force-pushed the 2020-07-peer branch from 123bba3 to afe3604 Compare July 30, 2020 08:22

jonatack reviewed Jul 30, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

maflcko reviewed Jul 30, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

jnewbery force-pushed the 2020-07-peer branch from afe3604 to 9510938 Compare July 30, 2020 11:40

DrahtBot mentioned this pull request Jul 30, 2020

[net] Cleanup logic around connection types #19316

Merged

ariard reviewed Jul 31, 2020

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 2, 2020

assumeutxo #15606

Closed

18 tasks

jnewbery force-pushed the 2020-07-peer branch from 9510938 to 8fe28ce Compare August 6, 2020 17:14

jonatack reviewed Aug 28, 2020

View reviewed changes

laanwj merged commit 1cf73fb into bitcoin:master Aug 28, 2020

jnewbery deleted the 2020-07-peer branch August 28, 2020 19:22

jnewbery added a commit to jnewbery/bitcoin that referenced this pull request Aug 28, 2020

[net processing] Add comment about destructing Peer objects

283995b

Suggested here: bitcoin#19607 (comment)

jnewbery mentioned this pull request Aug 28, 2020

net processing: Move block inventory state to net_processing #19829

Merged

jnewbery added a commit to jnewbery/bitcoin that referenced this pull request Sep 2, 2020

[net processing] Add comment about destructing Peer objects

415890f

Suggested here: bitcoin#19607 (comment)

jnewbery added a commit to jnewbery/bitcoin that referenced this pull request Sep 3, 2020

[net processing] Add comment about destructing Peer objects

3be8b7e

Suggested here: bitcoin#19607 (comment)

jnewbery added a commit to jnewbery/bitcoin that referenced this pull request Sep 7, 2020

[net processing] Add comment about destructing Peer objects

597a84f

Suggested here: bitcoin#19607 (comment)

jnewbery added a commit to jnewbery/bitcoin that referenced this pull request Sep 8, 2020

[net processing] Add comment about destructing Peer objects

98ce880

Suggested here: bitcoin#19607 (comment)

theuni mentioned this pull request Sep 24, 2020

net processing: Move peer_map to PeerManager #19910

Merged

jnewbery added a commit to jnewbery/bitcoin that referenced this pull request Sep 28, 2020

[net processing] Add comment about destructing Peer objects

85e7deb

Suggested here: bitcoin#19607 (comment)

jnewbery added a commit to jnewbery/bitcoin that referenced this pull request Sep 29, 2020

[net processing] Add comment about destructing Peer objects

36ddcac

Suggested here: bitcoin#19607 (comment)

jnewbery added a commit to jnewbery/bitcoin that referenced this pull request Oct 19, 2020

[net processing] Add comment about destructing Peer objects

d8a2ad4

Suggested here: bitcoin#19607 (comment)

jnewbery added a commit to jnewbery/bitcoin that referenced this pull request Nov 19, 2020

[net processing] Add comment about destructing Peer objects

e679b8d

Suggested here: bitcoin#19607 (comment)

jnewbery added a commit to jnewbery/bitcoin that referenced this pull request Dec 14, 2020

[net processing] Add comment about destructing Peer objects

cd6ae81

Suggested here: bitcoin#19607 (comment)

jnewbery mentioned this pull request Dec 19, 2020

Net: Move ping data to net_processing #20721

Merged

jnewbery added a commit to jnewbery/bitcoin that referenced this pull request Dec 19, 2020

[net processing] Add comment about destructing Peer objects

3be7e55

Suggested here: bitcoin#19607 (comment)

jnewbery added a commit to jnewbery/bitcoin that referenced this pull request Dec 19, 2020

[net processing] Improve documentation for Peer destruction/locking

717a374

Suggested here: - bitcoin#19607 (comment) - bitcoin#19829 (comment)

mjdietzx pushed a commit to mjdietzx/bitcoin that referenced this pull request Dec 26, 2020

[net processing] Improve documentation for Peer destruction/locking

65e3729

Suggested here: - bitcoin#19607 (comment) - bitcoin#19829 (comment)

This was referenced Feb 12, 2021

net/net processing: Move tx inventory into net_processing #21160

Merged

net/net processing: Move addr data into net_processing #21186

Merged

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

[p2p] Add Peer struct for per-peer data in net processing #19607

[p2p] Add Peer struct for per-peer data in net processing #19607

Uh oh!

Conversation

jnewbery commented Jul 28, 2020

Uh oh!

jnewbery commented Jul 28, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

troygiorshev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Jul 29, 2020

Uh oh!

jnewbery commented Jul 29, 2020

Uh oh!

jonatack commented Jul 30, 2020

Uh oh!

jnewbery commented Jul 30, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Jul 30, 2020

Uh oh!

troygiorshev commented Jul 30, 2020

Uh oh!

laanwj commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Jul 30, 2020

Uh oh!

fjahr commented Jul 30, 2020

Uh oh!

jnewbery commented Jul 30, 2020

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

sipa commented Aug 28, 2020

Uh oh!

Uh oh!

DrahtBot commented Jul 28, 2020 •

edited

Loading

laanwj commented Jul 30, 2020 •

edited

Loading