tracing: network connection tracepoints

[0xB10C]

This adds five new tracepoints with documentation and tests for network connections:

• established connections with net:inbound_connection and net:outbound_connection
• closed connections (both closed by us or by the peer) with net:closed_connnection
• inbound connections that we choose to evict with net:evicted_inbound_connection
• connections that are misbehaving and punished with net:misbehaving_connection

I've been using these tracepoints for a few months now to monitor connection lifetimes, re-connection frequency by IP and netgroup, misbehavior, peer discouragement, and eviction and more. Together with the two existing P2P message tracepoints they allow for a good overview of local P2P network activity. Also sort-of addresses #22006 (comment).

I've been back and forth on which arguments to include. For example, net:evicted_connection could also include some of the eviction metrics like e.g. last_block_time, min_ping_time, ... but I've left them out for now. If wanted, this can be added here or in a follow-up. I've tried to minimize a potential performance impact by measuring executed instructions with gdb where possible (method described here). I don't think a few hundred extra instructions are too crucial, as connection opens/closes aren't too frequent (compared to e.g. P2P messages). Note: e.g. CreateNodeFromAcceptedSocket() usually executes between 80k and 90k instructions for each new inbound connection.

tracepoint	instructions
net:inbound_connection	390 ins
net:outbound_connection	between 700 and 1000 ins
net:closed_connnection	473 ins
net:evicted_inbound_connection	not measured; likely similar to net:closed_connnection
net:misbehaving_connection	not measured

Also added a bpftrace (tested with v0.14.1) log_p2p_connections.bt example script that produces output similar to:

Attaching 6 probes...
Logging opened, closed, misbehaving, and evicted P2P connections
OUTBOUND conn to 127.0.0.1:15287: id=0, type=block-relay-only, network=0, total_out=1
INBOUND conn from 127.0.0.1:45324: id=1, type=inbound, network=0, total_in=1
MISBEHAVING conn id=1, score_before=0, score_increase=20, message='getdata message size = 50001', threshold_exceeded=false
CLOSED conn to 127.0.0.1:15287: id=0, type=block-relay-only, network=0, established=1231006505
EVICTED conn to 127.0.0.1:45324: id=1, type=inbound, network=0, established=1612312312

[jb55]

nice, utACK 7252c60

[russeree]

Just reporting in that everything is functional. Console log looks correct.

Tests Passed

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	virtu
Concept ACK	dergoegge
Stale ACK	jb55

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)

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.

[0xB10C]

Note to self: might be worth checking if we can also pass AS information and not only the net_group.

[0xB10C]

Note to self: might be worth checking if we can also pass AS information and not only the net_group.

This is possible by accessing the CNodeStats, but don't think we should do that in the tracepoint. We have the IP and can lookup the AS in a tracing scirpt, if necessary.

---

Fixed up the linter complains and rebased. This is ready for further review.

[dergoegge]

Concept ACK

[0xB10C]

The linter started complaining about "E275 missing whitespace after keyword". Fixed this up in the individual commits.

ac438b1 -> 6de4fc7 (2022-05-connection-tracepoints.pr.1 -> 2022-05-connection-tracepoints.pr.2; compare)

[virtu]

ACK 6de4fc7

Code reviewed tracepoints, tests, and demo script. Tracepoint placement looks good, so do tests. Successfully ran functional tests and the demo script.

Some points:

• Any reason why only inbound_connection and outbound_connection explicitly cast all arguments in the demo script?
• I think some of the format specifiers in the demo script are lightly off, leading to outputs with negative netgroup values on my system. I believe it should be %lld for int64 and %llu for uint64.
• As brought up previously, it would be nice to switch from nKeyedNetGroup to netgroup ids that are consistent over time

[jb55]

crACK af20707

(although I noticed the mispelling is still there)

[0xB10C]

Hm the CI failure of previous releases, qt5 dev package and depends packages, DEBUG (CI job used to find silent-merge conflicts (?)) four days ago seems weird. Didn't see any conflicts when rebasing.

Anyway, rebased. Sorry for invalidating your ACK again, @jb55. Fixed the discurage -> discourage misspelling. I thought I did it in an earlier push.

[jb55]

On Thu, Jan 18, 2024 at 02:03:35AM -0800, 0xB10C wrote: Hm the CI failure of `previous releases, qt5 dev package and depends packages, DEBUG` (CI job used to find silent-merge conflicts (?)) four days ago seems weird. Didn't see any conflicts when rebasing. Anyway, rebased. Sorry for invalidating your ACK again, @jb55. Fixed the discurage -> discourage misspelling. I thought I did it in an earlier push.

sorry I haven't been able to sit down and actually test this yet. The changes look good though!

[0xB10C]

Can we re-trigger the Cirrus CI / ASan + LSan + UBSan + integer, no depends, USDT job? The failing wallet_send.py --descriptors test looks unrelated.

[laanwj]

It's slightly inconsistent to pass this parameter as int, while the other enum (Connection Type) gets passed as string. Might similarly pass GetNetworkName(...).c_str(). Especially after #26593 which would remove the call overhead in the happy path.
No strong opinion though, I think the integer value is easier to use if you're going to make a histogram or such. My personal preference is also string-less APIs. But it's less consistent.

[0xB10C]

I fully agree what you've said. I'm leaving as is for now. If #26593 is merged and I retouch this, I'll reconsider changing this. I'll leave this as "unresolved" for now.

[virtu]

ACK 996029b (modulo comment and this)

Successfully ran log_p2p_connections.bt over the weekend on both x86_64/clang and aarch64/gcc.

Extended functional test (interface_usdt_net.py) equally passed on both machines.

[virtu]

LGTM! ACK 8109319

[0xB10C]

note that #29575 removes the misbehavior score (which is exposed via the net:misbehaving_connection tracepoint). If/when #29575 is merged, the misbehaving_connection tracepoint will change.