Differential D8794

[p2p] Add Peer struct for per-peer data in net processing
ClosedPublic
Actions

Authored by Fabien on Jan 5 2021, 16:10.

Details

Reviewers

majcosta

Group Reviewers

Restricted Project

Commits

rABCb6a215da6d3c: [p2p] Add Peer struct for per-peer data in net processing

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

Diff Detail

Repository

rABC Bitcoin ABC

Branch

PR19607

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 14589
Build 29139: Build Diff	lint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang
Build 29138: arc lint + arc unit

Event Timeline

Fabien created this revision.Jan 5 2021, 16:10

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 5 2021, 16:10

Fabien requested review of this revision.Jan 5 2021, 16:10

teamcity edited the summary of this revision. (Show Details)Jan 5 2021, 16:11

Harbormaster completed remote builds in B14575: Diff 26638.Jan 5 2021, 16:24

Fabien added a child revision: D8795: [net/net processing] check banman pointer before dereferencing.Jan 5 2021, 16:50

majcosta requested changes to this revision.Jan 5 2021, 21:23

majcosta added a subscriber: majcosta.

majcosta added inline comments.

src/net_processing.cpp
1261–1265 ↗	(On Diff #26638)	I don't think anywhere else in the code we use brackets around individual cases, except where we need a scope for LOCK(...) Perhaps its best to stick to the original PR and get rid of those for consistency? Ditto throughout.
src/test/denialofservice_tests.cpp
259 ↗	(On Diff #26638)	PR19464 is missing, I'll do it

This revision now requires changes to proceed.Jan 5 2021, 21:23

Fabien added inline comments.Jan 6 2021, 08:37

src/net_processing.cpp
1261–1265 ↗	(On Diff #26638)	Indeed, I did it for the DoS test already, better do it everywhere

Remove unnecessary brackets

Harbormaster completed remote builds in B14589: Diff 26657.Jan 6 2021, 09:05

Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Failed tests logs:

====== Bitcoin ABC functional tests: wallet_descriptor.py ======

------- Stdout: -------
2021-01-06T09:00:56.900000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20210106_085858/wallet_descriptor_132
2021-01-06T09:00:57.414000Z TestFramework (INFO): Making a descriptor wallet
2021-01-06T09:00:57.502000Z TestFramework (INFO): Checking wallet info
2021-01-06T09:00:57.505000Z TestFramework (INFO): Test that getnewaddress and getrawchangeaddress work
2021-01-06T09:00:58.326000Z TestFramework (INFO): Test sending and receiving
2021-01-06T09:00:58.430000Z TestFramework (INFO): Test disabled RPCs
2021-01-06T09:00:58.517000Z TestFramework (INFO): Test encryption
2021-01-06T09:00:59.070000Z TestFramework (INFO): Test that getnewaddress still works after keypool is exhausted in an encrypted wallet
2021-01-06T09:01:09.007000Z TestFramework (INFO): Test that unlock is needed when deriving only hardened keys in an encrypted wallet
2021-01-06T09:01:09.116000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 120, in main
    self.run_test()
  File "/work/test/functional/wallet_descriptor.py", line 129, in run_test
    "active": True
  File "/work/test/functional/test_framework/coverage.py", line 48, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/work/test/functional/test_framework/authproxy.py", line 159, in __call__
    raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Error: Please enter the wallet passphrase with walletpassphrase first. (-13)
2021-01-06T09:01:09.167000Z TestFramework (INFO): Stopping nodes
2021-01-06T09:01:09.321000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20210106_085858/wallet_descriptor_132
2021-01-06T09:01:09.321000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20210106_085858/wallet_descriptor_132/test_framework.log
2021-01-06T09:01:09.322000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20210106_085858/wallet_descriptor_132' to consolidate all logs

Each failure log is accessible here:
Bitcoin ABC functional tests: wallet_descriptor.py

majcosta accepted this revision.Jan 6 2021, 09:58

This revision is now accepted and ready to land.Jan 6 2021, 09:58

Rebase

Harbormaster completed remote builds in B14596: Diff 26681.Jan 6 2021, 12:41

Closed by commit rABCb6a215da6d3c: [p2p] Add Peer struct for per-peer data in net processing (authored by John Newbery <john@johnnewbery.com>, committed by Fabien). · Explain WhyJan 6 2021, 12:43

This revision was automatically updated to reflect the committed changes.

Fabien added a commit: rABCb6a215da6d3c: [p2p] Add Peer struct for per-peer data in net processing.

Fabien mentioned this in D8862: [avalanche] Initiate avalanche when the service bit is present and we have a stake..Jan 11 2021, 10:34