Skip to content

Conversation

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Nov 27, 2016

Matt pointed out to me that this appeared to be doing nothing (except involving itself in data races).

Also removes pnodeLocalHost.

@gmaxwell
Copy link
Contributor Author

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

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Nov 27, 2016

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.
@gmaxwell gmaxwell changed the title Remove fNetworkNode. Remove fNetworkNode and pnodeLocalHost. Nov 27, 2016
@fanquake fanquake added the P2P label Nov 27, 2016
@TheBlueMatt
Copy link
Contributor

Slightly-tested ACK bdb922b

@theuni
Copy link
Member

theuni commented Nov 27, 2016

Aww, I was looking forward to nuking a bunch of this useless stuff all at once :)

utACK

@sipa
Copy link
Member

sipa commented Nov 27, 2016

utACK bdb922b

@sdaftuar
Copy link
Member

Lightly tested ACK bdb922b (though I have zero idea what this pnodeLocalHost thing was ever for).

@sipa
Copy link
Member

sipa commented Nov 28, 2016

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).

@gmaxwell
Copy link
Contributor Author

@sdaftuar My commit message said! (as Pieter says, it was the 'pub sub' system in the original release of Bitcoin)

@laanwj
Copy link
Member

laanwj commented Nov 29, 2016

Oh wow, I'm surprised this stuck around for so long as it is indeed used nowhere.
Good find!
utACK bdb922b

@sipa sipa merged commit bdb922b into bitcoin:master Dec 1, 2016
sipa added a commit that referenced this pull request Dec 1, 2016
bdb922b Remove pnodeLocalHost. (Gregory Maxwell)
083f203 Remove fNetworkNode. (Gregory Maxwell)
codablock pushed a commit to codablock/dash that referenced this pull request Jan 17, 2018
bdb922b Remove pnodeLocalHost. (Gregory Maxwell)
083f203 Remove fNetworkNode. (Gregory Maxwell)
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
bdb922b Remove pnodeLocalHost. (Gregory Maxwell)
083f203 Remove fNetworkNode. (Gregory Maxwell)
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 25, 2019
bdb922b Remove pnodeLocalHost. (Gregory Maxwell)
083f203 Remove fNetworkNode. (Gregory Maxwell)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants