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

BIP-0157: increase max getcfilters request to 1k blocks #857

Merged
merged 1 commit into from Nov 4, 2019

Conversation

@Roasbeef
Copy link
Contributor

Roasbeef commented Oct 25, 2019

In this commit, we effectively revert #699 by allow clients to request
filter for up to 1k consecutive blocks. Testing in the field has shown
that applications are able to reduce perceived latency from syncing to
full functionality after an app has been offline for several days by
batching requests for filters. A value of 100 would mean each additional
day behind adds an additional round trip, resulting in 10s of
seconds of lag after just a few days of being offline. A value of ~1k
allows implementations to catch up with roughly a week's worth of
filters in a single round trip.

In this commit, we effectively revert #699 by allow clients to request
filter for up to 1k consecutive blocks. Testing in the field has shown
that applications are able to reduce perceived latency from syncing to
full functionality after an app has been offline for several days by
batching requests for filters. A value of 100 would mean each additional
day behind adds an additional round trip, resulting in 10s of
seconds of lag after just a few days of being offline. A value of ~1k
allows implementations to catch up with roughly a week's worth of
filters in a single round trip.
@Roasbeef Roasbeef force-pushed the Roasbeef:bip-157-filter-request-limit branch from 094bd71 to 898559f Oct 25, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 4, 2019

@luke-jr luke-jr merged commit a758915 into bitcoin:master Nov 4, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@harding

This comment has been minimized.

Copy link
Member

harding commented Nov 10, 2019

How does this interact with Bitcoin Core's maximum sizes for P2P messages? According to the reverted #699, 100x filters produces messages about 2MB in size, so I assume 1000x filters would be messages about 20MB in size. However:

$ cd bitcoin
$ git grep static.*MAX_SIZE
src/serialize.h:static const unsigned int MAX_SIZE = 0x02000000;

$ git grep static.*MAX_PROTOCOL_MESSAGE_LENGTH
src/net.h:static const unsigned int MAX_PROTOCOL_MESSAGE_LENGTH = 4 * 1000 * 1000;

Max size is 32 MiB, which seems pretty close to the current expected size of 20MB. Is there a chance that larger blocks (e.g. more segwit txes) or just blocks with lots of outputs (e.g. increased payment batching) could inflate the typical filter size by ~50% so that this results in too-large messages?

(The MAX_PROTOCOL_MESSAGE_LENGTH of 4 MB seems to be only for messages Bitcoin Core accepts, so it isn't relevant to BIP157 unless Bitcoin Core decides to totally detach its wallet from the full node. Still, I wanted to mention it here in case someone knows of it being used somewhere else.)

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Nov 11, 2019

For anti-DoS reasons, there's no way a full node will respond to a few-byte message with a 32MB message. I assume the BIP allows Bitcoin Core to just respond with fewer filters (which it obviously will).

@harding

This comment has been minimized.

Copy link
Member

harding commented Nov 11, 2019

@TheBlueMatt that makes sense, but I think that wouldn't be compliant with the BIP as written. This PR allows a client to request up to 1000 block filters. If I'm reading correctly, the node "MUST" then provide all the requested filters:

The receiving node MUST respond to valid requests by sending one cfilter message for each block in the requested range, sequentially in order by block height.

@harding

This comment has been minimized.

Copy link
Member

harding commented Nov 11, 2019

Sorry, I think I misunderstood BIP157. I thought it was working like getheaders and headers where the results would be bundled into a single response. Instead, each cfilter message seems to be separate, so each message will be under the MAX_SIZE even though getcfilters may be able to request more than 32 MiB of data at once (spread out over 1,000 P2P messages).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.