query+neutrino: use query dispatcher for GetBlock and GetCFilter #273

ellemouton · 2023-05-05T13:38:12Z

This PR refactors the GetBlock and GetCFilter methods so that they use the
work dispatcher for their queries instead of using the old queryPeers function.
The function is now removed bringing us one step closer to removing all query
logic from the main package.

query_test.go

query.go

guggero

Very nice improvement! Did a first pass just to load some context, didn't see anything that stood out as being problematic. I assume we'll battle test all these improvements mostly indirectly through the lnd itests?

neutrino.go

query.go

ellemouton · 2023-05-08T15:30:19Z

I assume we'll battle test all these improvements mostly indirectly through the lnd itests?

Ah, that's a good point - I should open an LND PR pointing to all these changes so that the neutrino itest suite can be run 👍

EDIT: cool, doing that here for now

guggero

Looks good to me 🎉

query.go

ellemouton · 2023-05-12T14:16:34Z

hmmm - ok so I think the unit-race fail is a flake. Gonna re-run it...

positiveblue

LGTM 💯

I saw that the unit tests were failing in #69 but they all passed locally.

banman/reason.go

query.go

ellemouton · 2023-05-17T12:54:16Z

I saw that the unit tests were failing in ellemouton/lnd#69 but they all passed locally.

ah - let me open one that only points to this and not the others. Just to isolate it a bit.

ellemouton · 2023-05-17T12:57:27Z

ok, I've updated ellemouton/lnd#69 to just point to this PR. fingers crossed for unit tests 🤞

ellemouton · 2023-05-17T13:07:48Z

erg - realised we will need to make an update in btcwallet aswell since it uses the old WorkManager struct which is now an interface 🙃

ellemouton · 2023-05-17T16:07:21Z

ok the LND unit tests seem to consistently fail (at least on GH) when pointing neutrino to this PR. I will need to investigate. I will ping on here once things are working 👍

ellemouton · 2023-05-22T14:23:53Z

alrighty! after searching for waaaayyy too many hours - I found the issue 🙈

The issue is that: the worker manager, as it currently is, doesnt have any form of "maximum number of retries" and so if any query fails, it just reschedules the job so that it can be tried again later. This is not the behaviour that the rescan code expects especially during a re-org. The rescan function wants the query to error so that it can retry itself. The reason we want the calling function to control retries is cause perhaps the query failed due to a re-org and so the actual query itself needs to be changed. So what was happening in the tests that were failing is: a query was started to get a filter for a certain block hash, then a re-org happens, and then the query manager just keeps trying the same query with the old block hash forever.

So now I have added a commit that allows the caller to set the maximum number of retries.

gonna re-request review since a new commit was added (it is the second commit)

LND things now passing: https://github.com/ellemouton/lnd/actions/runs/5046731469/jobs/9052661472?pr=69

ellemouton · 2023-05-23T15:16:44Z

Ok y'all - I think this is finally ready for another look :) The NumRetries and NoRetryMax functional options have been added.

One thing i'm not sure how to deal with and would love a second opinion on: the GetBlock and GetCFilter methods currently take in a set of neutrino.QueryOptions whereas the query.Query method takes in a set of query.QueryOptions. There is not a 1:1 mapping between these 2 sets... So the question is, should we completely change how the query.WorkManager in order to force this 1:1 mapping? or should we just add a comment saying which functional options no longer apply to GetBlock and GetCFilter?

guggero

Very nice, LGTM 🎉

Can we turn ellemouton/lnd#69 into an upstream PR so we can track CI there as well?

query/workmanager.go

guggero · 2023-06-12T10:16:01Z

query/workmanager_test.go

-	wm, workers := startWorkManager(t, numWorkers)
+	workMgr, workers := startWorkManager(t, numWorkers)
+
+	wm, ok := workMgr.(*workManager)


nit: could use require.IsType(t, &workManager{}, workMgr) for a nicer failure message if this ever is false. Also, there's a (pre-existing) typo in the godoc comment of this test function.

Roasbeef · 2023-06-14T00:54:02Z

the GetBlock and GetCFilter methods currently take in a set of neutrino.QueryOptions whereas the query.Query method takes in a set of query.QueryOptions. There is not a 1:1 mapping between these 2 sets... So the question is, should we completely change how the query.WorkManager in order to force this 1:1 mapping? or should we just add a comment saying which functional options no longer apply to GetBlock and GetCFilter?

Need to check out the updated diff, but is it not possible to create a 1:1 mapping between them?

Or is this about documenting a potential breaking API change?

query/workmanager.go

query/interface.go

query/workmanager.go

query/workmanager_test.go

query.go

Roasbeef · 2023-06-14T01:19:41Z

query.go

+
+	// Prepare the query options.
+	queryOpts := []query.QueryOption{
+		query.Encoding(qo.encoding),


At a glance the option mapping here looks to be lossy, but need to dig deeper to see if it'll matter in practice (eg: we don't need things like PeerConnectTimeout here.

Yeah it's defs lossy. See my overall review comment on the trickiness of the mapping between these two sets of
functional options

query.go

Roasbeef · 2023-06-14T01:22:09Z

query.go

@@ -619,23 +745,20 @@ func (s *ChainService) prepareCFiltersQuery(blockHash chainhash.Hash,
 	}
 	bestHeight := int64(bestBlock.Height)

-	qo := defaultQueryOptions()


I think rather than remove the old set of options, we should instead seek to unify them. If we need to add extra options that are relevant to the main package, but not the query package, then we can use a type alias here.

for this method specifically, the old options arent remove, just moved one call back. See where this method is called.

If we need to add extra options that are relevant to the main package, but not the query package, then we can use a type alias here.

can you elaborate on "we can use a type alias here"?

query.go

Roasbeef · 2023-06-14T01:40:55Z

Ah ok, I see the distinction now:

neutrino/query.go

Lines 140 to 151 in d209c5e

    
           // defaultQueryOptions returns a queryOptions set to package-level defaults. 
        
           func defaultQueryOptions() *queryOptions { 
        
           	return &queryOptions{ 
        
           		timeout:            QueryTimeout, 
        
           		numRetries:         uint8(QueryNumRetries), 
        
           		peerConnectTimeout: QueryPeerConnectTimeout, 
        
           		rejectTimeout:      QueryRejectTimeout, 
        
           		encoding:           QueryEncoding, 
        
           		invalidTxThreshold: QueryInvalidTxThreshold, 
        
           		optimisticBatch:    noBatch, 
        
           	} 
        
           }

vs

neutrino/query/interface.go

Lines 35 to 48 in d209c5e

    
           // QueryOption is a functional option argument to any of the network query 
        
           // methods, such as GetBlock and GetCFilter (when that resorts to a network 
        
           // query). These are always processed in order, with later options overriding 
        
           // earlier ones. 
        
           type QueryOption func(*queryOptions) // nolint 
        
           // defaultQueryOptions returns a queryOptions set to package-level defaults. 
        
           func defaultQueryOptions() *queryOptions { 
        
           	return &queryOptions{ 
        
           		timeout:    defaultQueryTimeout, 
        
           		encoding:   defaultQueryEncoding, 
        
           		cancelChan: nil, 
        
           	} 
        
           }

In this commit, the worker manager is adjusted to only retry a request a certain maximum number of times. This is done so that a smooth transition can be made from call sights currently using the neutrino.queryChainServicePeers method to using the query.WorkManager instead so that the same default behaviour can be expected. In order to also cater for call sights currently using the query.WorkManager, a NoRetryMax optional paramater is added that can used to disable the retry maximum.

Introduce a `WorkManager` interface that encapsulates the Dispatcher interface along with `Start` and `Stop` methods. The previous `WorkManager` struct is converted to `workManager` which is an implementation of the new `WorkManager` interface. The `ChainService` struct is then adapted to hold the new interface instead of the struct. This will make testing easier in future.

Use the work dipatcher interface from the query package to make getdata requests instead of using the old queryPeers function.

Rename the `query` variable in `GetCFilter` so that the `query` package can be imported and used cleanly.

Use the work dispatcher query interface instead of the old queryPeers method for making getcfilter requests. This ensures that the queries are made to the most responsive peers.

Since both GetBlock and GetCFilter now make use of the work dispatcher instead of the `queryPeers` function, we can now remove the `queryPeers` and `queryChainServicePeers` functions.

ellemouton

but is it not possible to create a 1:1 mapping between them?

I've tried various things to do this but it always ends up being quite contrived. So for example:

the query dispatcher makes use of a dynamic query timeout meaning that it tries a low timeout and increases it if needed. This also means that this timeout is not exposed to the caller. But the neutrino query options have a Timeout option used to set the static timeout that was used in the now deleted query method. So does this then mean we add a WithStaticQueryTimout option to the query dispatcher and lose the nice dynamic timeout feature there?

Also, just in general: mapping one set of functional options to another is tricky cause it is hard to tell what the user specifically set/requested and what was set by default. So it is hard to know what is ok to override in the original set of options.

Or is this about documenting a potential breaking API change?

So im trying to determine if we should do a whole thing to make the options mappable or if we should rather just add extra documentation around the functional options saying "This one wont apply for GetBlock" or w/e. And then the other option is to break the API completely and have a new set of functional options.

query/interface.go

query/workmanager.go

ellemouton · 2023-06-23T09:07:17Z

query.go

@@ -619,23 +745,20 @@ func (s *ChainService) prepareCFiltersQuery(blockHash chainhash.Hash,
 	}
 	bestHeight := int64(bestBlock.Height)

-	qo := defaultQueryOptions()


for this method specifically, the old options arent remove, just moved one call back. See where this method is called.

query.go

ellemouton · 2023-06-23T13:37:39Z

query.go

+
+	// Prepare the query options.
+	queryOpts := []query.QueryOption{
+		query.Encoding(qo.encoding),


Yeah it's defs lossy. See my overall review comment on the trickiness of the mapping between these two sets of
functional options

ellemouton · 2023-06-23T13:40:06Z

query.go

@@ -619,23 +745,20 @@ func (s *ChainService) prepareCFiltersQuery(blockHash chainhash.Hash,
 	}
 	bestHeight := int64(bestBlock.Height)

-	qo := defaultQueryOptions()


If we need to add extra options that are relevant to the main package, but not the query package, then we can use a type alias here.

can you elaborate on "we can use a type alias here"?

query.go

ellemouton · 2023-06-23T14:04:50Z

@Roasbeef - re-requesting just to get your opinion on how to move forward with the 2 separate/conflicting query option sets

ellemouton · 2023-06-23T14:05:25Z

Can we turn ellemouton/lnd#69 into an upstream PR so we can track CI there as well?

Yeah! done here: lightningnetwork/lnd#7788

lightninglabs-deploy · 2023-07-07T14:40:57Z

@Roasbeef: review reminder
@positiveblue: review reminder

Roasbeef · 2023-07-10T22:34:45Z

So im trying to determine if we should do a whole thing to make the options mappable or if we should rather just add extra documentation around the functional options saying "This one wont apply for GetBlock" or w/e. And then the other option is to break the API completely and have a new set of functional options.

So I think we may be able to unify things by inserting a type param here: there'd be a set of "static" values (say the timeout, etc), then a value T that's inserted to allow the specific package/call site to insert the extra information needed for the error. So something along the lines of: https://golang.design/research/generic-option/

I don't think this should block the rest of this PR series though, as upfront we've been able to identify some option abstraction leaks while ensuring the existing behavior is properly capture as is.

Roasbeef · 2023-07-10T22:35:53Z

So does this then mean we add a WithStaticQueryTimout option to the query dispatcher and lose the nice dynamic timeout feature there?

Agreed that in most cases we do want a dynamic timeout vs a static one. In the past, we had some hard coded timeouts tuned by devs in SF, then those in South America needed to retune things as certain timeouts were no longer valid across the higher latency environment (was mobile also).

Roasbeef

LGTM 🦠

lightninglabs/neutrino#273 Author: Elle Mouton <elle.mouton@gmail.com> Date: Tue May 9 12:33:19 2023 +0200 and btcsuite#884 Author: Olaoluwa Osuntokun <laolu32@gmail.com> Date: Fri Aug 4 11:46:12 2023 -0700

…locksync Plus lightninglabs/neutrino#273 Author: Elle Mouton <elle.mouton@gmail.com> Date: Mon May 22 15:53:49 2023 +0200 chain: PrunedBlockDispatcher bugfix.

lightninglabs/neutrino#273 Author: Elle Mouton <elle.mouton@gmail.com> Date: Tue May 9 12:33:19 2023 +0200 and btcsuite#884 Author: Olaoluwa Osuntokun <laolu32@gmail.com> Date: Fri Aug 4 11:46:12 2023 -0700

…locksync Plus lightninglabs/neutrino#273 Author: Elle Mouton <elle.mouton@gmail.com> Date: Mon May 22 15:53:49 2023 +0200 chain: PrunedBlockDispatcher bugfix.

This was referenced May 5, 2023

multi: introduce a batch filter writer #274

Merged

rescan: use batch filter fetching #275

Merged

neutrino+query+rescan: improve rescan speed #236

Closed

Roasbeef requested review from guggero and positiveblue May 5, 2023 22:56

Roasbeef reviewed May 5, 2023

View reviewed changes

query_test.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 Show resolved Hide resolved

ellemouton force-pushed the useQueryDispatcher branch 2 times, most recently from d853b50 to a182aad Compare May 8, 2023 07:30

guggero reviewed May 8, 2023

View reviewed changes

neutrino.go Outdated Show resolved Hide resolved

query.go Outdated Show resolved Hide resolved

ellemouton force-pushed the useQueryDispatcher branch 3 times, most recently from b3ecf6e to 72c016e Compare May 9, 2023 10:43

ellemouton requested a review from guggero May 9, 2023 10:45

ellemouton force-pushed the useQueryDispatcher branch 3 times, most recently from c1691b6 to 7824721 Compare May 9, 2023 10:56

guggero approved these changes May 12, 2023

View reviewed changes

query.go Outdated Show resolved Hide resolved

query.go Show resolved Hide resolved

ellemouton force-pushed the useQueryDispatcher branch 2 times, most recently from 2aadf9c to 2870613 Compare May 12, 2023 14:02

saubyk assigned ellemouton May 16, 2023

positiveblue approved these changes May 17, 2023

View reviewed changes

banman/reason.go Show resolved Hide resolved

query.go Show resolved Hide resolved

ellemouton force-pushed the useQueryDispatcher branch from 2870613 to cf6d843 Compare May 22, 2023 13:58

ellemouton requested a review from positiveblue May 22, 2023 14:24

ellemouton force-pushed the useQueryDispatcher branch from aa9aec2 to df858a8 Compare May 23, 2023 14:59

positiveblue mentioned this pull request Jun 6, 2023

neutrino: implement parallel header download via header checkpoints #71

Open

guggero approved these changes Jun 12, 2023

View reviewed changes

Roasbeef requested changes Jun 14, 2023

View reviewed changes

ellemouton added 6 commits June 23, 2023 13:40

neutrino+query: use work dispatcher for GetBlock

bfa56f6

Use the work dipatcher interface from the query package to make getdata requests instead of using the old queryPeers function.

query: rename query variable

45bbe53

Rename the `query` variable in `GetCFilter` so that the `query` package can be imported and used cleanly.

neutrino+query: use work dispatcher for GetCFilter

22311cb

Use the work dispatcher query interface instead of the old queryPeers method for making getcfilter requests. This ensures that the queries are made to the most responsive peers.

query+neutrino: remove unused queryPeers function

663cacc

Since both GetBlock and GetCFilter now make use of the work dispatcher instead of the `queryPeers` function, we can now remove the `queryPeers` and `queryChainServicePeers` functions.

ellemouton commented Jun 23, 2023

View reviewed changes

ellemouton force-pushed the useQueryDispatcher branch from df858a8 to 663cacc Compare June 23, 2023 13:43

ellemouton requested a review from Roasbeef June 23, 2023 14:04

ellemouton mentioned this pull request Jun 23, 2023

go.mod: bump neutrino and btcwallet versions lightningnetwork/lnd#7788

Merged

Roasbeef approved these changes Jul 10, 2023

View reviewed changes

Roasbeef merged commit 5aac983 into lightninglabs:master Jul 10, 2023

Chinwendu20 mentioned this pull request May 1, 2024

Use query.Workmanager for cfilter queries #182

Closed

buck54321 mentioned this pull request May 8, 2024

catch spv up with lightninglabs/neutrino dcrlabs/ltcwallet#6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

query+neutrino: use query dispatcher for GetBlock and GetCFilter #273

query+neutrino: use query dispatcher for GetBlock and GetCFilter #273

ellemouton commented May 5, 2023

guggero left a comment

ellemouton commented May 8, 2023 •

edited

Loading

guggero left a comment

ellemouton commented May 12, 2023 •

edited

Loading

positiveblue left a comment

ellemouton commented May 17, 2023

ellemouton commented May 17, 2023

ellemouton commented May 17, 2023

ellemouton commented May 17, 2023

ellemouton commented May 22, 2023 •

edited

Loading

ellemouton commented May 23, 2023

guggero left a comment

guggero Jun 12, 2023

Roasbeef commented Jun 14, 2023

Roasbeef Jun 14, 2023

ellemouton Jun 23, 2023

Roasbeef Jun 14, 2023

ellemouton Jun 23, 2023

ellemouton Jun 23, 2023

Roasbeef commented Jun 14, 2023

ellemouton left a comment

ellemouton Jun 23, 2023

ellemouton Jun 23, 2023

ellemouton Jun 23, 2023

ellemouton commented Jun 23, 2023

ellemouton commented Jun 23, 2023

lightninglabs-deploy commented Jul 7, 2023

Roasbeef commented Jul 10, 2023

Roasbeef commented Jul 10, 2023

Roasbeef left a comment

query+neutrino: use query dispatcher for GetBlock and GetCFilter #273

query+neutrino: use query dispatcher for GetBlock and GetCFilter #273

Conversation

ellemouton commented May 5, 2023

guggero left a comment

Choose a reason for hiding this comment

ellemouton commented May 8, 2023 • edited Loading

guggero left a comment

Choose a reason for hiding this comment

ellemouton commented May 12, 2023 • edited Loading

positiveblue left a comment

Choose a reason for hiding this comment

ellemouton commented May 17, 2023

ellemouton commented May 17, 2023

ellemouton commented May 17, 2023

ellemouton commented May 17, 2023

ellemouton commented May 22, 2023 • edited Loading

ellemouton commented May 23, 2023

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Jun 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Jun 14, 2023

ellemouton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton commented Jun 23, 2023

ellemouton commented Jun 23, 2023

lightninglabs-deploy commented Jul 7, 2023

Roasbeef commented Jul 10, 2023

Roasbeef commented Jul 10, 2023

Roasbeef left a comment

Choose a reason for hiding this comment

ellemouton commented May 8, 2023 •

edited

Loading

ellemouton commented May 12, 2023 •

edited

Loading

ellemouton commented May 22, 2023 •

edited

Loading