refactor: Remove nMyStartingHeight from CNode/Connman #20649

1. 
   MarcoFalke commented at 11:27 am on December 14, 2020: member

   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.
2. MarcoFalke added the label Refactoring on Dec 14, 2020
3. MarcoFalke added the label P2P on Dec 14, 2020
4. 
   jnewbery commented at 11:34 am on December 14, 2020: member

   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.

5. 
   MarcoFalke commented at 11:47 am on December 14, 2020: member

   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.

6. 
   jnewbery commented at 12:51 pm on December 14, 2020: member

   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.

7. 
   DrahtBot commented at 7:12 pm on December 14, 2020: member

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

8. 
   DrahtBot commented at 8:12 pm on December 14, 2020: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #20811 (refactor: move net_processing implementation details out of header by ajtowns)
   • #20789 (fuzz: Rework strong and weak net enum fuzzing by MarcoFalke)
   • #20788 (net: add RAII socket and use it instead of bare SOCKET by vasild)
   • #20758 (net-processing refactoring – lose globals, move implementation details from .h to .cpp by ajtowns)
   • #20685 (Add I2P support using I2P SAM by vasild)
   • #20370 (DRAFT fuzz: Permission flags in net_processing fuzzers by MarcoFalke)
   • #20228 (addrman: Make addrman a top-level component by jnewbery)
   • #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.

9. 
   jnewbery commented at 10:54 am on December 15, 2020: member

   Code review ACK fa7afc076328ef3fbc56b51f62f7afae24701dae
10. DrahtBot added the label Needs rebase on Dec 15, 2020
11. MarcoFalke force-pushed on Dec 15, 2020
12. DrahtBot removed the label Needs rebase on Dec 15, 2020
13. 
    MarcoFalke commented at 9:36 am on December 16, 2020: member

    rebased
14. 
    jnewbery commented at 10:08 am on December 16, 2020: member

    code review ACK faf54d4af483a5e107d47b16cfa47cdeb815e723
15. MarcoFalke force-pushed on Dec 17, 2020
16. 
    naumenkogs commented at 8:59 am on December 18, 2020: member

    Concept ACK
17. 
    jnewbery commented at 10:33 am on December 18, 2020: member

    Code review ACK fa8a96a914639ecda7f5d8c522dc0339815efef5
18. theStack approved
19. 
    theStack commented at 2:34 pm on December 28, 2020: member

    Nice cleanup! Code review ACK fa8a96a914639ecda7f5d8c522dc0339815efef5 ☃️
20. DrahtBot added the label Needs rebase on Jan 2, 2021
21. refactor: Remove nMyStartingHeight from CNode/Connman faaa4f2b6a
22. MarcoFalke force-pushed on Jan 2, 2021
23. 
    MarcoFalke commented at 9:42 am on January 2, 2021: member

    Rebased due to trivial conflict in adjacent line
24. 
    jnewbery commented at 9:44 am on January 2, 2021: member

    utACK faaa4f2b6af4ce249fc4809a030240afa45b1a33
25. theStack approved
26. 
    theStack commented at 11:01 am on January 2, 2021: member

    re-ACK faaa4f2b6af4ce249fc4809a030240afa45b1a33 ⚓ 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.
27. DrahtBot removed the label Needs rebase on Jan 2, 2021
28. MarcoFalke merged this on Jan 2, 2021
29. MarcoFalke closed this on Jan 2, 2021

    ---

30. MarcoFalke deleted the branch on Jan 2, 2021
31. sidhujag referenced this in commit 2c8996ffbb on Jan 2, 2021
32. Fabcien referenced this in commit 21ae0d505a on Jan 25, 2022
33. DrahtBot locked this on Aug 16, 2022

Contributors
MarcoFalke jnewbery DrahtBot naumenkogs theStack

Labels
Refactoring P2P