-
Notifications
You must be signed in to change notification settings - Fork 38
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
Queries With No Blocks #146
base: main
Are you sure you want to change the base?
Conversation
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.
This looks mostly good, but (as we discussed elsewhere) the behavior when two active requests share blocks and one of them ignores blocks needs to be resolved.
linkTracker := prs.getLinkTracker(requestID) | ||
isUnique = linkTracker.BlockRefCount(link) == 0 | ||
_, noBlockRequest := prs.noBlockRequests[requestID] | ||
isUnique = linkTracker.BlockRefCount(link) == 0 && !noBlockRequest |
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.
isUnique
is now misleading. shouldSend
?
}) | ||
require.NoError(t, err) | ||
|
||
assertSentNotOnWire(t, bd1, blks[0]) |
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.
Could we change this to assertNotSentOnWire? I assume this tests that the block is not sent at all, instead of it being sent but not on the wire like the name suggests.
return nil | ||
}) | ||
require.NoError(t, err) | ||
fph.AssertResponses(expectedResponses{ |
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.
Should there be an assertion that the blks[0] was sent? I would expect that it was since IgnoreAllBlocks
has not been called for requestID2, and I assume that's the point of this second transaction. Some comments on what the different sections of this test are trying to achieve would be helpful.
No description provided.