This looks somewhat awkward as-is, but it’s a part of the CConnman refactor. I’ve broken it out and PR’d separately because it’s a change of behavior that requires some scrutiny. As part of #8085, the height will be passed into CConnman instead. For reference, it will end up looking more like this: https://github.com/bitcoin/bitcoin/pull/8085/commits/148163c9b1ce04ec9fa9c8ff9a4289520bd92127.
Even without that, though, I think this is an improvement, as it eliminates a dependency on main from net.
Push block height changes from main into net, and pass the current height into CNode at creation time, rather than pulling from main each time (and locking cs_main). Now CNode has no dependency on main for height.
Note that It may also be handy to know later what our original height was for a given node.
This also helps to prevent identity leakage a tiny bit. Before this change, an attacker could theoretically make 2 connections on different interfaces. They would connect fully on one, and only establish the initial connection on the other. Once they receive a new block, they would relay it to your first connection, and immediately commence the version handshake on the second. Since the new block height was reflected immediately, they could attempt to learn whether the two connections were correlated.
This is, of course, incredibly unlikely to work due to the small timings involved and receipt from other senders. But it doesn’t hurt to lock-in nBestHeight at the time of connection, rather than letting the remote choose the time.
