Page MenuHomePhabricator

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

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

Details

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

Event Timeline

Fabien requested review of this revision.Jan 5 2021, 16:10
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
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

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

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