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

Tests Passed

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.

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.

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

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.

Concept ACK

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)

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

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

CNetAddr::GetBIP155Network() might be useful.

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.

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.

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.

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.