refactor: Remove nMyStartingHeight from CNode/Connman

[maflcko]

CNode and CConnman keep track of the active chain height when CNodes have been created, but apart from serializing the int once (when sending a version message), it is unused. So it can simply be removed in favor of a single int in PeerMan that can do the same.

[jnewbery]

Concept ACK. I have a branch that does almost exactly the same as this.

I don't think we even need to storem_best_height in PeerManager. We can just call ::ChainActive().Height() whenever we want to send a version message.

[maflcko]

just call ::ChainActive().Height() whenever we want to send a version message.

I was thinking that it would be nice (in the future) if most message handling could be done without cs_main.

[jnewbery]

I was thinking that it would be nice (in the future) if most message handling could be done without cs_main.

This would definitely be nice! At the moment, both places we send a version message require cs_main at some point, so we're quite far off at the moment.

I have a preference for not caching the block height in PeerManager since I think it makes isolating and testing behaviour more difficult, but either way, this is clearly a big improvement over the current code.

[DrahtBot]

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• refactor: move net_processing implementation details out of header #20811 (refactor: move net_processing implementation details out of header by ajtowns)
• fuzz: Rework strong and weak net enum fuzzing #20789 (fuzz: Rework strong and weak net enum fuzzing by MarcoFalke)
• net: add RAII socket and use it instead of bare SOCKET #20788 (net: add RAII socket and use it instead of bare SOCKET by vasild)
• net-processing refactoring -- lose globals, move implementation details from .h to .cpp #20758 (net-processing refactoring -- lose globals, move implementation details from .h to .cpp by ajtowns)
• Add I2P support using I2P SAM #20685 (Add I2P support using I2P SAM by vasild)
• fuzz: version handshake #20370 (DRAFT fuzz: Permission flags in net_processing fuzzers by MarcoFalke)
• addrman: Make addrman a top-level component #20228 (addrman: Make addrman a top-level component by jnewbery)
• Erlay: bandwidth-efficient transaction relay protocol #18261 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)

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.

[jnewbery]

Code review ACK fa7afc0

[maflcko]

rebased

[jnewbery]

code review ACK faf54d4

[naumenkogs]

Concept ACK

[jnewbery]

Code review ACK fa8a96a

[theStack]

Nice cleanup!
Code review ACK fa8a96a ☃️

[maflcko]

Rebased due to trivial conflict in adjacent line

[jnewbery]

utACK faaa4f2

[theStack]

re-ACK faaa4f2 ⚓
Checked that the changes since my previous cr ACK were only rebase-related via $ git range-diff fa8a96a9146...faaa4f2b6af, also compiled and ran the tests.