Skip to content

refactor: Remove nMyStartingHeight from CNode/Connman#20649

Merged
maflcko merged 1 commit intobitcoin:masterfrom
maflcko:2012-netNoMyHeight
Jan 2, 2021
Merged

refactor: Remove nMyStartingHeight from CNode/Connman#20649
maflcko merged 1 commit intobitcoin:masterfrom
maflcko:2012-netNoMyHeight

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Dec 14, 2020

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
Copy link
Contributor

jnewbery commented Dec 14, 2020

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
Copy link
Member Author

maflcko commented Dec 14, 2020

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
Copy link
Contributor

jnewbery commented Dec 14, 2020

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
Copy link
Contributor

DrahtBot commented Dec 14, 2020

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

@DrahtBot DrahtBot mentioned this pull request Dec 14, 2020
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 14, 2020 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

This was referenced Dec 14, 2020
Merged
Merged
Merged
Merged
Merged
Merged
@jnewbery
Copy link
Contributor

jnewbery commented Dec 15, 2020

Code review ACK fa7afc076328ef3fbc56b51f62f7afae24701dae

@maflcko
Copy link
Member Author

maflcko commented Dec 16, 2020

rebased

@jnewbery
Copy link
Contributor

jnewbery commented Dec 16, 2020

code review ACK faf54d4af483a5e107d47b16cfa47cdeb815e723

@DrahtBot DrahtBot mentioned this pull request Dec 17, 2020
Merged
@naumenkogs
Copy link
Contributor

naumenkogs commented Dec 18, 2020

Concept ACK

@jnewbery
Copy link
Contributor

jnewbery commented Dec 18, 2020

Code review ACK fa8a96a

This was referenced Dec 19, 2020
Closed
Closed
Closed
Closed
theStack
theStack approved these changes Dec 28, 2020
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup!
Code review ACK fa8a96a ☃️

@DrahtBot DrahtBot mentioned this pull request Dec 28, 2020
Merged
This was referenced Dec 29, 2020
Merged
Merged
@maflcko maflcko force-pushed the 2012-netNoMyHeight branch from fa8a96a to faaa4f2 Compare January 2, 2021 09:25
@maflcko
Copy link
Member Author

maflcko commented Jan 2, 2021

Rebased due to trivial conflict in adjacent line

@jnewbery
Copy link
Contributor

jnewbery commented Jan 2, 2021

utACK faaa4f2

theStack
theStack approved these changes Jan 2, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@maflcko maflcko merged commit 6f97172 into bitcoin:master Jan 2, 2021
@maflcko maflcko deleted the 2012-netNoMyHeight branch January 2, 2021 12:08
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 2, 2021
Merge bitcoin#20649: refactor: Remove nMyStartingHeight from CNode/Co…
2c8996f 
…nnman
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 25, 2022
@Fabcien
refactor: Remove nMyStartingHeight from CNode/Connman
21ae0d5 
Summary:
```
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.
```

Backport of [[bitcoin/bitcoin#20649 | core#20649]].

Depends on D10884.

Ref T1696.

Test Plan:
  ninja all check-all
  ninja bitcoin-fuzzers

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Subscribers: PiRK

Maniphest Tasks: T1696

Differential Revision: https://reviews.bitcoinabc.org/D10885
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@theStack theStack theStack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maflcko @jnewbery @DrahtBot @naumenkogs @theStack