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

Batch filter fetching #134

Merged
merged 10 commits into from Apr 18, 2019

Conversation

Projects
None yet
4 participants
@halseth
Copy link
Contributor

commented Mar 15, 2019

This PR implements batch filter fetching. This is achieved by giving the callers of GetCFilter the option to make a "optimistic batch query", where the filters following the requested filters are also fetched, and added to the filter cache.

I ran this on mainnet to find cache size needed to keep a whole filter batch, and a size of 30 MB could hold at minimum 1483 elements (tested on filters at height > 515110), which should give us some wiggle room.

@halseth halseth force-pushed the halseth:batch-filter-fetching branch 2 times, most recently from 700ea42 to b6b1a19 Mar 15, 2019

@coveralls

This comment has been minimized.

Copy link

commented Mar 15, 2019

Coverage Status

Coverage decreased (-0.2%) to 68.53% when pulling d7eb617 on halseth:batch-filter-fetching into 72e8054 on lightninglabs:master.

@wpaulino
Copy link
Member

left a comment

Looks pretty good to me on first pass! Would say I was expecting a more abstracted batching interface where we can batch requests for different non-consecutive height ranges, but the approach implemented here seems to work well assuming that we'll be doing incremental linear scans. One issue I see with the current approach however, is that lnd sometimes performs scans backwards, like within the ChainNotifier when detecting channel spends. In this case, the current approach would not work unless we implement some other kind of heuristic.

Show resolved Hide resolved headerfs/file.go
Show resolved Hide resolved headerfs/index.go
Show resolved Hide resolved query.go Outdated
Show resolved Hide resolved query.go Outdated
Show resolved Hide resolved query.go Outdated
Show resolved Hide resolved query.go
Show resolved Hide resolved query.go
Show resolved Hide resolved headerfs/file.go Outdated
Show resolved Hide resolved headerfs/file.go Outdated
Show resolved Hide resolved cache/cache.go
Show resolved Hide resolved neutrino.go Outdated
Show resolved Hide resolved query.go Outdated
@@ -749,14 +749,8 @@ func (s *ChainService) GetCFilter(blockHash chainhash.Hash,
return nil, fmt.Errorf("unknown filter type: %v", filterType)
}

// Only get one CFilter at a time to avoid redundancy from mutliple

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Apr 6, 2019

Member

Without this mutex, we'll have multiple concurrent requests for the same set of filters.

Show resolved Hide resolved query.go Outdated
Show resolved Hide resolved query.go
Show resolved Hide resolved query.go Outdated
Show resolved Hide resolved query.go

@halseth halseth force-pushed the halseth:batch-filter-fetching branch 8 times, most recently from 5678742 to b6c179f Apr 11, 2019

@halseth

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Comments addressed, PTAL @Roasbeef @wpaulino

@wpaulino
Copy link
Member

left a comment

Nice refactor from the previous version! Will begin testing this. No major comments other than one left inline about reverse batches.

Show resolved Hide resolved headerfs/file.go Outdated
Show resolved Hide resolved headerfs/file.go Outdated
Show resolved Hide resolved query.go
Show resolved Hide resolved query.go Outdated
Show resolved Hide resolved query.go
Show resolved Hide resolved query.go
Show resolved Hide resolved query.go Outdated
Show resolved Hide resolved query.go Outdated
Show resolved Hide resolved query.go Outdated
Show resolved Hide resolved query.go Outdated

halseth added some commits Apr 11, 2019

query: define cfiltersQuery
cfiltersQuery is a struct that encapsulates all information necessary to
request CFilters and handle the received responses.
cache+query: add evicted return value to cache.Put
We add a return value to the cache's Put method, to indicate whether
items needed to be evicted to add the new one. This is useful in
scenarios where we fetch a series of items (like a batch of filters)
that we add to the cache. If we need to evict items to fit the items,
while the number of items in the cache is less than the batch size, it
indicates that the full cache cannot hold all the items in the batch,
and we should decrease the batch size.

@halseth halseth force-pushed the halseth:batch-filter-fetching branch from 2be9b2d to d7eb617 Apr 16, 2019

@Roasbeef
Copy link
Member

left a comment

LGTM 💈

Still not super satisfied with portions of the new code, but I think we'll defer a greater refactoring in that area until we re-write the batch query interface all together.


// handleCFiltersRespons is called every time we receive a response for the
// GetCFilters request.
func (s *ChainService) handleCFiltersResponse(q *cfiltersQuery,

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Apr 18, 2019

Member

Doesn't need to be a method on the ChainService.

Show resolved Hide resolved query.go

@Roasbeef Roasbeef merged commit be81df3 into lightninglabs:master Apr 18, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.2%) to 68.53%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.