-
Notifications
You must be signed in to change notification settings - Fork 38.6k
test: Fix NODE_NETWORK_LIMITED_MIN_BLOCKS disconnection #21230
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
The context manager was not even created, so previously it did not check the debug log
jonasschnelli
left a comment
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.
utACK fa24247 - thanks for fixing.
|
Code changes look good, but I don't understand what the problem is or how the fix works. I'm seeing the feature_blockfilterindex_prune.py failure during the sync_all() call https://cirrus-ci.com/task/5429627528151040?command=ci#L3364 and a "[ProcessGetBlockData] Ignore block request below NODE_NETWORK_LIMITED threshold from peer=0" message in the logs. But I don't understand what the log message means means or why replacing one generate/sync sequence with two would fix the problem. I also don't understand why this error seems to only happen on one platform, and I can't tell from the bug report whether this is happens reliably is some kind of race condition. |
ryanofsky
left a comment
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.
Code review ACK fa24247 with caveat above that I don't really understand the problem or fix. But the cleanups look good and the fix does seem perfectly safe. More description would be welcome!
If you look at the next line after the logprint, you will see that this is the reason for disconnection: LogPrint(BCLog::NET, "Ignore block request below NODE_NETWORK_LIMITED threshold from peer=%d\n", pfrom.GetId());
//disconnect node and prevent it from stalling (would otherwise wait for the missing block)
pfrom.fDisconnect = true;If you then look up This is a race condition because a node that "immediately" requests the block that has been announced to it won't request a block that is 288 blocks "old". Thus it won't hit the disconnect. |
|
Very helpful explanation! Thanks! I didn't know there was a disconnection, and didn't realize ignoring request from a peer might also involve disconnecting the peer. |
Fix several bugs. Also, fix #21227