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

all: Refactor for new subscription API #72

Merged
merged 3 commits into from Mar 25, 2024
Merged

all: Refactor for new subscription API #72

merged 3 commits into from Mar 25, 2024

Conversation

lukechampine
Copy link
Member

This is mostly in a good place, but there is one major sticking point: wallet.NewManager.

With the old API, when a call to cm.AddBlocks returns, all of the subscribers will have finished processing the new update(s). With the new API, this is not true; the processing happens asynchronously. This broke some tests which expected that, after calling AddBlocks, they could immediately observe the effects of those blocks in wallet state. Now, they can't, so we need a way for the test to wait until processing has finished. In most of the tests, I worked around this; see the syncDB helper I added. But that approach doesn't work when you have a real wallet manager running. For now, I faked the waiting with time.Sleep, just to see if that makes the tests pass (it does), but clearly we need a better solution.

The whole asynchronous processing thing really bugs me, so much so that I tweaked the new API to release the chain manager's lock before calling the onReorg listener functions. This means it's now legal for your listener function to call chain manager methods -- meaning, you can implement subscription "the old fashioned way" and have your onReorg function directly call UpdatesSince in a loop until it's caught up. Effectively this brings us back to the same situation as before, except that the manager is no longer locked, so you have a bit more freedom. (And of course, you can still do async processing if you want.)

But the other big wrinkle here is the wallet manager's Subscribe method. Previously, it unsubscribed from the chain manager, then resubscribed at a lower height. Firstly, it's not clear to me why this would work; won't you end up with duplicate state? But more importantly, we need to nail down how this action ought to interact with the wallet manager's ongoing subscription in the new API. In my PR code, we hold the wallet manager's lock while syncing, so Subscribe effectively pauses the background subscription until it's finished. Not exactly pretty, but it gets the job done. Is there a better way? That's what I'm not sure about.

@n8maninger
Copy link
Member

n8maninger commented Mar 11, 2024

I faked the waiting with time.Sleep

That's how hostd and renterd tests work as well. It's not great, but it works (most of the time).

Previously, it unsubscribed from the chain manager, then resubscribed at a lower height. Firstly, it's not clear to me why this would work; won't you end up with duplicate state?

State can't be duplicated in the SQLite store. If a subscriber tries to create an element that already exists, it silently fails and continues processing. Same for deletion/spending. If the element doesn't exist, it silently fails and continues processing.

In my PR code, we hold the wallet manager's lock while syncing, so Subscribe effectively pauses the background subscription until it's finished. Not exactly pretty, but it gets the job done. Is there a better way? That's what I'm not sure about.

In the old API, we could rely on the chain manager locking to guarantee all updates would be seen. This new API may cause problems with that assumption if the scanner is interupted half way through and ends up skipping a revert. I think there are a few ways we could solve that if my understanding is correct.

@n8maninger
Copy link
Member

I rebased this to get rid of the CI errors

@n8maninger n8maninger merged commit 932c0d9 into master Mar 25, 2024
8 checks passed
@n8maninger n8maninger deleted the subscription branch March 25, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants