-
Notifications
You must be signed in to change notification settings - Fork 38.7k
Remove fNetworkNode and pnodeLocalHost. #9226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Have I missed something here? I just don't want to be left out of the finds-code-he-doesn't-understand-so-opens-a-pr-to-remove-it club! |
src/main.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should now be if (!pfrom->fInbound) to avoid changing behavior, as it is used to decide whether to call addrman.Connected from DeleteNode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should be addrman.Connected(pnode->addr) on inbound connections too. I could document this better in the commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm perhaps not, okay I reasoned myself out of that. I'll change it.
|
You might also remove pnodeLocalHost as well, since, afaict, it makes fNetworkNode a synonym for !fInbound. |
Matt pointed out to me that this appeared to be doing nothing (except involving itself in data races).
Mostly a legacy of the long removed pub/sub system.
|
Slightly-tested ACK bdb922b |
|
Aww, I was looking forward to nuking a bunch of this useless stuff all at once :) utACK |
|
utACK bdb922b |
|
Lightly tested ACK bdb922b (though I have zero idea what this pnodeLocalHost thing was ever for). |
It seems to have originally been needed for the pubsub mechanism (which was removed in #1036). |
|
@sdaftuar My commit message said! (as Pieter says, it was the 'pub sub' system in the original release of Bitcoin) |
|
Oh wow, I'm surprised this stuck around for so long as it is indeed used nowhere. |
Matt pointed out to me that this appeared to be doing nothing (except involving itself in data races).
Also removes pnodeLocalHost.