This PR is an improvement proposal to solve the problem of issue #22898 "There are cases where the peer status is not updated forever".
I used this line: https://github.com/bitcoin/bitcoin/blob/ecf580e40f607091a5e5d5fff9865237e037ea7f/src/net_processing.cpp#L4605 because it is meaningless (always false).
fFoundStartingHeader is not necessarily false because it could be made true inside the for (const uint256& hash : peer->m_blocks_for_headers_relay) { loop, right?
Also, if you were thinking it's always false, it would just make sense to drop fFoundStartingHeader variable altogether? After your change it's never being accessed anyway.
OK. Normally fForceSendHeaders is always true when adding a new block. It's hard to imagine an exception, but I reverted fFoundStartingHeader because something special might happen.
@naumenkogs The current code is designed not to send headers to peers from that we received new blocks or headers. Is this specification intentional? If so, why? I think this is probably the idea that the peer is already aware of the new block and doesn't need to be notified again. However, as long as the node has peer status information, I think it should always be correct. The current code is inappropriate in that sense. What do you think?
Sorry, I'm trying to help but I'm completely lost in your code and your intentions. I'm looking at the current master code, and it does make sense to me, so I'm not sure what you're trying to change in it. Your issue, despite being very elaborate, is also confusing.
@naumenkogs Did you see issue #22898 ? Do you think the test results are as expected and it is fine? Do you say it's correct not to notify our status to peers who already have a new block? I'm not saying that the current code is absolutely wrong. But if it's correct to notify our status to all connected peers, then the current code is wrong.
I built and tested v22.0 with this PR and the peer status has been improved to always show the correct status.
Please answer my question simply. However, it's hard to get your answer, so today I'll try another method to explain the problem.
This is the line I think is the problem with the current code:
} else if (PeerHasHeader (& state, pindex)) {
https://github.com/bitcoin/bitcoin/blob/5c0f46ca46e23a161649b5150baa01020dc85e48/src/net_processing.cpp#L4608
This if statement is true for peers that the node has already received a new block or header. So the node does not send headers to that peer. Therefore, the peer does not update the status of this node.
Do you think this behavior of the node is correct?
@naumenkogs Are you fundamentally misunderstanding? I don't think you can't understand this simple problem. Please consider it a little more seriously.
I think there's a language barrier or other communication problem here.
Is this an issue in the GUI that is fixed with a patch to net_processing.cpp?
Closing for now due to inactivity. It might be good to explain the problem that is fixed with clear steps to reproduce, starting from scratch, or maybe even as a (functional) test.
Let us know when this is done, so that this pull can be reopened.