Skip to content
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

[WIP] Don't effectively blacklist pruned nodes. #8572

Closed
wants to merge 1 commit into from

Conversation

rebroad
Copy link
Contributor

@rebroad rebroad commented Aug 24, 2016

If upon connecting it's found that the node doesn't have the blocks we need, we can always disconnect then, but unless we give the node a chance to become a full-node, it may forever be forgotten about (until it changes it's IP address).

Also, even pruned nodes provide much use for block propagation and it's only during IBD that we're needing historical blocks anyway. Perhaps in this situation it is reasonable to skip nodes that were once seen to be pruning (but may no longer be).

@sipa
Copy link
Member

sipa commented Aug 24, 2016

As pointed out elsewhere, we first need a means to distinguish pruned nodes from nodes that don't relay/verify at all.

@rebroad
Copy link
Contributor Author

rebroad commented Aug 24, 2016

@sipa Why "first"? Yes, I agree that we need this, but how is banning useful pruning nodes a way forward?

And anyway, there's nothing to stop a node advertising as NODE_NETWORK yet not relaying/verifying, so simply banning nodes that don't set NODE_NETWORK is 0% reliable, isn't it?

@sipa
Copy link
Member

sipa commented Aug 24, 2016 via email

@pstratem
Copy link
Contributor

Also this commit should just be change the requires services flags, not removing the check.

@rebroad
Copy link
Contributor Author

rebroad commented Aug 24, 2016

@pstratem I thought of that - an equally valid place to change it, IMHO

@sipa Where is the right place to have that discussion? I guess the other place to "fix" this is to change it so that pruning nodes still advertise as NODE_NETWORK. Perhaps that is a better solution, and then upon connection the node can establish how far back its blocks go. Storing the services bits in the address database was a nice idea - perhaps it can also store additional services which aren't also stored in the services version messages, such as whether a node is pruned, and use this bit instead of the NODE_NETWORK bit currently being used. If this is an ok idea, I can go ahead and create a pull for this functionality (unless someone else wants to do it).

I'm currently working on some other changes which can better determine what value a node is providing, such as whether it's giving usful txs, speedily delivering blocks etc, and then could use this in determining when to evict outgoing connections (in a similar way to the incoming connections evictions but based on different parameters). I'd ideally like to be able to display some of these metrics in the Peers Debug pane, but I'm not up to speed enough on Qt to program this and anyway, I think a webpage would be a better way to display it as that could also be served by bitcoind too.

@sipa
Copy link
Member

sipa commented Aug 24, 2016

@rebroad Pruning nodes can't advertize NODE_NETWORK, because NODE_NETWORK implies they can serve old blocks.

We can have a discussion on the mailing list about new flags or means to advertize what ranges a node has available for download. I believe this PR is in error though - it tries to incorrectly change something that is by design. NACK.

@gmaxwell
Copy link
Contributor

"banning useful pruning nodes a way forward", please be more careful with your comments. It doesn't ban anything here, it just doesn't connect out to them. I can envision the headlines now, "bitcoin core bans pruned nodes!"-- it's just not so. :)

As mentioned in the other PR we need a service type for nodes that relay near-tip that defines how much they must retain, at a minimum.

@isle2983
Copy link
Contributor

Nit - this change leaves the declaration of REQUIRED_SERVICES in src/net.h behind as dead code. It should probably be removed too.

@rebroad
Copy link
Contributor Author

rebroad commented Aug 26, 2016

@sipa meanings change over time. NETWORK_NODE used to mean that this node was worth connecting to for obtaining blocks. It can still mean this, but by making pruned nodes not have this flag, it is changing the meaning. Before pruning nodes existed, no one was defining NETWORK_NODE as specifically as "a node that provides blocks all the way from the latest block to the genesis block" because it was understood that NETWORK_NODEs were the nodes to connect to to get blocks. The meaning needs to stay up to date just as much as the code does. As things evolve, ambiguity often slips in, and I think it's important to be careful how we redefine things as part of that evolution.

@gmaxwell noted. I meant to include the word "effectively" (I thought I did). It doesn't use the ban function, but by omiting them form the advertised addresses shared among nodes, they are effectively banished, wouldn't you agree?

@rebroad rebroad changed the title Don't effectively blacklist pruned nodes. [WIP] Don't effectively blacklist pruned nodes. Aug 26, 2016
If upon connecting it's found that the node doesn't have the blocks we need, we can always disconenct then, but unless we give the node a chance to become a full-node, it will forever be forgotten about. Also, even pruned nodes provide much use for block propagation and it's only during IBD that we're needing historical blocks anyway. Perhaps in this situation it is reasonable to skip nodes that were once seen to be pruning (but may no longer be).
@rebroad
Copy link
Contributor Author

rebroad commented Aug 26, 2016

@pstratem @isle2983 Have removed REQUIRED_SERVICES variable now, as it was providing no value other than moving the logic away from where it was needed (which I'd argue was a negative value). I've also made it so that the logic about requiring NETWORK_NODE is back, but now only during IBD, afterwhich, pruned nodes will also be selected - obviously they need to be in the address list for this to happen, so the other code removal remains in order for this to be possible.

@rebroad rebroad changed the title [WIP] Don't effectively blacklist pruned nodes. Don't effectively blacklist pruned nodes. Aug 26, 2016
@sipa
Copy link
Member

sipa commented Aug 26, 2016 via email

@sipa
Copy link
Member

sipa commented Aug 26, 2016

NODE_NETWORK is and remains required for connections. We don't ever want to waste a connection slot on a node that does not even claim to relay blocks and transactions.

We can discuss how to introduce service flags to indicate lack of archive functionality, but this is not the way.

NACK.

@sipa
Copy link
Member

sipa commented Aug 26, 2016

See the discussion about service bits for pruned nodes here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2013-April/thread.html#2461

@rebroad rebroad changed the title Don't effectively blacklist pruned nodes. [WIP] Don't effectively blacklist pruned nodes. Aug 28, 2016
@rebroad
Copy link
Contributor Author

rebroad commented Aug 28, 2016

@sipa Thanks for the consideration you gave this. I'm in agreement that the code needs refining as we need to ensure a minimum number of NODE_NETWORKS nodes to connect to, which these changes would not guarantee, admittedly. Have changed to [WIP], as I'm working on refining this, although perhaps closing it is better for now...

The work I refer to is to make use of pruned nodes better than we currently do - i.e. connecting to non NODE_NETWORK nodes (in addition to a minimal number of NODE_NETWORK nodes) but not making so many assumptions about the service bits - i.e. testing both NODE_NETWORK and non-NODE_NETWORK nodes to see what services they are providing. I realise in the future that the services available will become more clearly advertised but I still prefer to make the code assume as little as possible - I even have a pull request in the pipeline that makes nodes almost totally ignore the nVersion of a node for these same non-assumptive ethics.

@rebroad rebroad closed this Aug 28, 2016
@rebroad rebroad deleted the Don'tBanPrunedNodes branch April 18, 2020 07:02
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants