refactor: Remove nMyStartingHeight from CNode/Connman #20649

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2012-netNoMyHeight changing 12 files +33 −55
  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: contributor

    <!--4a62be1de6b64f3ed646cdc7932c8cf5-->

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

  8. DrahtBot cross-referenced this on Dec 14, 2020 from issue doc: Move addr relay comment in net to correct place by MarcoFalke
  9. DrahtBot commented at 8:12 PM on December 14, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  10. DrahtBot cross-referenced this on Dec 14, 2020 from issue fuzz: Avoid time-based "non-determinism" in fuzzing harnesses by using mocked GetTime() by practicalswift
  11. DrahtBot cross-referenced this on Dec 14, 2020 from issue refactor, net: Increase CNode data member encapsulation by hebasto
  12. DrahtBot cross-referenced this on Dec 14, 2020 from issue fuzz: version handshake by MarcoFalke
  13. DrahtBot cross-referenced this on Dec 14, 2020 from issue net: use std::chrono throughout maxOutbound logic by fanquake
  14. DrahtBot cross-referenced this on Dec 14, 2020 from issue addrman: Make addrman a top-level component by jnewbery
  15. DrahtBot cross-referenced this on Dec 14, 2020 from issue net: assert CNode::m_inbound_onion is inbound in ctor, add getter, unit tests by jonatack
  16. jnewbery commented at 10:54 AM on December 15, 2020: member

    Code review ACK fa7afc076328ef3fbc56b51f62f7afae24701dae

  17. DrahtBot added the label Needs rebase on Dec 15, 2020
  18. MarcoFalke force-pushed on Dec 15, 2020
  19. DrahtBot removed the label Needs rebase on Dec 15, 2020
  20. MarcoFalke commented at 9:36 AM on December 16, 2020: member

    rebased

  21. jnewbery commented at 10:08 AM on December 16, 2020: member

    code review ACK faf54d4af483a5e107d47b16cfa47cdeb815e723

  22. MarcoFalke force-pushed on Dec 17, 2020
  23. DrahtBot cross-referenced this on Dec 17, 2020 from issue Add I2P support using I2P SAM by vasild
  24. naumenkogs commented at 8:59 AM on December 18, 2020: member

    Concept ACK

  25. jnewbery commented at 10:33 AM on December 18, 2020: member

    Code review ACK fa8a96a914639ecda7f5d8c522dc0339815efef5

  26. DrahtBot cross-referenced this on Dec 19, 2020 from issue tree-wide: De-globalize ChainstateManager by dongcarl
  27. DrahtBot cross-referenced this on Dec 20, 2020 from issue p2p: standardize outbound full/block relay connection type naming by jonatack
  28. DrahtBot cross-referenced this on Dec 23, 2020 from issue net-processing refactoring -- lose globals, move implementation details from .h to .cpp by ajtowns
  29. DrahtBot cross-referenced this on Dec 25, 2020 from issue Erlay: bandwidth-efficient transaction relay protocol by naumenkogs
  30. theStack approved
  31. theStack commented at 2:34 PM on December 28, 2020: contributor

    Nice cleanup! Code review ACK fa8a96a914639ecda7f5d8c522dc0339815efef5 ☃️

  32. DrahtBot cross-referenced this on Dec 28, 2020 from issue net: add RAII socket and use it instead of bare SOCKET by vasild
  33. DrahtBot cross-referenced this on Dec 29, 2020 from issue fuzz: Rework strong and weak net enum fuzzing by MarcoFalke
  34. DrahtBot cross-referenced this on Dec 31, 2020 from issue refactor: move net_processing implementation details out of header by ajtowns
  35. DrahtBot added the label Needs rebase on Jan 2, 2021
  36. refactor: Remove nMyStartingHeight from CNode/Connman faaa4f2b6a
  37. MarcoFalke force-pushed on Jan 2, 2021
  38. MarcoFalke commented at 9:42 AM on January 2, 2021: member

    Rebased due to trivial conflict in adjacent line

  39. jnewbery commented at 9:44 AM on January 2, 2021: member

    utACK faaa4f2b6af4ce249fc4809a030240afa45b1a33

  40. theStack approved
  41. theStack commented at 11:01 AM on January 2, 2021: contributor

    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.

  42. DrahtBot removed the label Needs rebase on Jan 2, 2021
  43. MarcoFalke merged this on Jan 2, 2021
  44. MarcoFalke closed this on Jan 2, 2021
  45. MarcoFalke deleted the branch on Jan 2, 2021
  46. sidhujag referenced this in commit 2c8996ffbb on Jan 2, 2021
  47. Fabcien referenced this in commit 21ae0d505a on Jan 25, 2022
  48. bitcoin locked this on Aug 16, 2022
Contributors
MarcoFalkejnewberyDrahtBotnaumenkogstheStack
Labels
RefactoringP2P
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:53 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me