Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Feb 18, 2021

Fix several bugs. Also, fix #21227

MarcoFalke added 3 commits February 18, 2021 20:43
The context manager was not even created, so previously it did not check the debug log
@fanquake fanquake added the Tests label Feb 18, 2021
Copy link
Contributor

@jonasschnelli jonasschnelli left a 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.

@ryanofsky
Copy link
Contributor

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.

Copy link
Contributor

@ryanofsky ryanofsky left a 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!

@jonasschnelli jonasschnelli merged commit 9f3ffa2 into bitcoin:master Feb 19, 2021
@maflcko maflcko deleted the 2102-testFix branch February 19, 2021 08:11
@maflcko
Copy link
Member Author

maflcko commented Feb 19, 2021

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.

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 NODE_NETWORK_LIMITED_MIN_BLOCKS you will see that it is 288 blocks. Thus, the condition shouldn't be hit when less than 288 blocks (from the tip) are synced. (250 is less than 288)

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.

@ryanofsky
Copy link
Contributor

ryanofsky commented Feb 19, 2021

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.

@bitcoin bitcoin locked and limited conversation to collaborators Feb 19, 2021
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.

feature_blockfilterindex_prune.py is consistently failing on master for macOS CI
4 participants