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, message='getdata message size = 50001'
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

[portlandhodl]

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

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/25832.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	laanwj, vasild, sipa
Concept ACK	dergoegge, i-am-yuvi
Stale ACK	jb55, virtu, maflcko

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:

• #30988 (Split CConnman by vasild)
• #30381 ([WIP] net: return result from addnode RPC by willcl-ark)

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

[laanwj]

Both CI errors are related to mempool synchronization. It seems very unlikely to be caused by the changes in this PR.

[laanwj]

Code review ACK 6eb94b3082e6ff6882e27ee98abb3c79f34abbe0

[0xB10C]

Rebased and addressed #25832 (comment)

[laanwj]

re-ACK e3622a9

[vasild]

ACK e3622a9

[sipa]

utACK e3622a9

[sipa]

It's not very nice to expose this internal value in the interface, if we want it to be stable. What about using the BIP155 network ID instead?

[0xB10C]

Using BIP155 network IDs is a good idea! Planning to address #25832 (comment) in a followup.

[vasild]

CNetAddr::GetBIP155Network() might be useful.

[0xB10C]

I implemented both the BIP155 NetworkID approach (0xB10C@72133f3) and the Network as string approach from #25832 (comment) (0xB10C@39102d4).

The BIP155 approach is a bit more involved with making GetBIP155Network() public, introducing a ConnectedThroughBIP155Network() helper, and passing 0 (or something else?) for NET_INTERNAL as there is no BIP155 equivalent. With this approach, NET_UNROUTABLE isn't exposed - e.g. 127.0.0.1 will get the BIP155 NetworkID for IPv4.

The network name as string approach exposes NET_UNROUTABLE as not_publicly_routable and NET_INTERNAL as internal. We have these in -netinfo too.

I have a slight preference on BIP155 NetworkIDs over keeping-as-is or network name as string.

[laanwj]

I have a slight preference on BIP155 NetworkIDs over keeping-as-is or network name as string.

Agree. Strings are sensitive to typos, and require extra effort for matching when doing statistics. i think even the current enumeration is better than that. BIP155 is a good suggestion, but as you have to make up non-standard values, and handle NET_UNROUTABLE differently i'm not entirely sure.

The purpose of tracing is to gain insight into internal state so logging the actual values makes more sense than thinking up a new convention. The internal enumeration is likely stable enough to warrant being a (semi-)stable interface.

[sipa]

The BIP155 approach is a bit more involved with making GetBIP155Network() public, introducing a ConnectedThroughBIP155Network() helper, and passing 0 (or something else?) for NET_INTERNAL as there is no BIP155 equivalent.

Given that BIP155 network IDs are uint8_t's, I think it suffices to pick a value outside of that range for unreachable. For example, 0x100, or 0xffffffff for a uint32_t, or -1 for a signed integer type.

Using 0 works too of course, and I guess it won't get assigned in future extensions, but using something outside of the range might be even more future-proof.

[0xB10C]

Agree. Strings are sensitive to typos, and require extra effort for matching when doing statistics. i think even the current enumeration is better than that. BIP155 is a good suggestion, but as you have to make up non-standard values, and handle NET_UNROUTABLE differently i'm not entirely sure.
The purpose of tracing is to gain insight into internal state so logging the actual values makes more sense than thinking up a new convention. The internal enumeration is likely stable enough to warrant being a (semi-)stable interface.

I tend to agree. A few toughs:

• Mapping the internal Network enum to other values makes it harder to go from tracepoint output/observation to Bitcoin Core internals again
• Using a third (1. Network enum, 2. BIP155) protocol "BIP155 extended for tracepoint interface to handle Bitcoin Core internal values not present in BIP155" seems weird, confusing, and not worth it (?). If mapping to BIP155 would have been more straight forward, I'd have been happy to use it.
• Personally, I'd favor to avoid changes/functions (like ConnectedThroughBIP155Network(), making GetBIP155Network() public) that are only used by or useful for tracepoints.
• The tracepoint interface will inherently expose internals. It won't ever be stable in a way the RPC interface is stable. If we remove or refactor something, the affected tracepoints will need to change. This happened in the past with, for example, removing the misbehaving score (tracing: network connection tracepoints #25832 (comment)) after net_processing: make any misbehavior trigger immediate discouragement #29575 and cluster mempool: Implement changeset interface for mempool #31122 (comment). If the internals change, the tracepoints and their interface needs to change too. Though, this could be better documented in https://github.com/bitcoin/bitcoin/blob/master/doc/tracing.md#semi-stable-api

Unless there are strong opinions on needing a follow up, I'll plan to leave this as is.

[0xB10C]

This should have been

- assert_equal(0, closed_connection.conn.network)
+ assert_equal(NETWORK_TYPE_UNROUTABLE, closed_connection.conn.network)