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

server: Always serve known getcfilterv2 filters. #3035

Merged

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Dec 2, 2022

Similar to the recently changed getheaders handling, which also had the same semantics, the current logic for responding to getcfilterv2 includes a check to avoid serving filters whenever the current local chain is not considered to be current.

Like the getheaders case, one less than ideal consequence of not responding is that it can lead to peers appearing to be unresponsive and/or stalled whenever the peer does not consider itself current, which can happen temporarily in a variety of corner cases such as after being unable to communicate with the network for a long time, or in testing scenarios where there are necessarily long periods of time without any new blocks.

This existing behavior for getcfilterv2 handling was inherited from the older bloom filter logic where it mattered because bloom filters could potentially put a substantial strain on the server and also could significantly slow down the initial chain sync.

However, the aforementioned concerns no longer apply to version 2 block filters because they have a fixed creation cost per block, are required to exist for main chain blocks by consensus, and are the same for all peers.

Taken as a whole, this means that serving the filters is relatively cheap and therefore this modifies the semantics to always respond to getcfilterv2 requests, even before the local chain is fully synced.

Similar to the recently changed getheaders handling, which also had the
same semantics, the current logic for responding to getcfilterv2
includes a check to avoid serving filters whenever the current local
chain is not considered to be current.

Like the getheaders case, one less than ideal consequence of not
responding is that it can lead to peers appearing to be unresponsive
and/or stalled whenever the peer does not consider itself current, which
can happen temporarily in a variety of corner cases such as after being
unable to communicate with the network for a long time, or in testing
scenarios where there are necessarily long periods of time without any
new blocks.

This existing behavior for getcfilterv2 handling was inherited from the
older bloom filter logic where it mattered because bloom filters could
potentially put a substantial strain on the server and also could
significantly slow down the initial chain sync.

However, the aforementioned concerns no longer apply to version 2 block
filters because they have a fixed creation cost per block, are required
to exist for main chain blocks by consensus, and are the same for all
peers.

Taken as a whole, this means that serving the filters is relatively
cheap and therefore this modifies the semantics to always respond to
getcfilterv2 requests, even before the local chain is fully synced.
@davecgh davecgh added the wire protocol change Discussion and pull requests regarding items that require changes to the wire protocol. label Dec 2, 2022
@davecgh davecgh added this to the 1.8.0 milestone Dec 2, 2022
Copy link
Member

@matheusd matheusd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now works to serving cfilters to the wallet once the minKnownWork height is reached, as expected.

@davecgh davecgh merged commit 5133ec2 into decred:master Dec 5, 2022
@davecgh davecgh deleted the server_always_respond_to_getcfilterv2 branch December 5, 2022 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wire protocol change Discussion and pull requests regarding items that require changes to the wire protocol.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants