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.

Sign up for GitHub

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

Jump to bottom

Refactor and cleanup custodian events cache handling #756

Merged
merged 2 commits into from
Jan 10, 2024
Merged

Refactor and cleanup custodian events cache handling #756

merged 2 commits into from
Jan 10, 2024

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Jan 4, 2024

This PR makes the following changes:

  1. Refactor method Custodian.receiveProof such that it can be executed without a new goroutine (I see this as a simplification: putting all goroutine/sync code in the same scope).
  2. Makes the custodian's event cache thread safe.
  3. We then use the new thread safe event cache to cleanup events from within the courier proof retrieval goroutine.
  4. Simplify the code by separating the event completion code from the mapProofToEvent method. In this way, mapProofToEvent only retrieves an event related to a given proof, and nothing more. Then, all event completion code is in Custodian.setReceiveCompleted.

@ffranr
tapgarden: refactor Custodian.receiveProof so that it's sync agnostic
41e22b9 
This commit refactors the method `Custodian.receiveProof` such that it's
sync/goroutine agnostic (can be run with/without a goroutine). This
change moves all goroutine wait group management code to the point where
the goroutine is started.

This commit modifies `receiveProof` such that it returns an error rather
than writes an error directly to log. This means that we can pass the
error to the custodian's error channel.
@ffranr ffranr self-assigned this Jan 4, 2024
@ffranr ffranr requested review from guggero and GeorgeTsagk January 4, 2024 17:42
guggero
guggero reviewed Jan 5, 2024
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Thanks for the refactor!
I think the first commit definitely makes sense. But the rest IMO make the already hard to follow flow even trickier...
What do you think of my suggestion to instead just feed the new proof into the main event loop to assert cleanup of the events?

tapgarden/custodian.go Outdated Show resolved Hide resolved
tapgarden/custodian.go Outdated Show resolved Hide resolved
@ffranr ffranr force-pushed the custodian-events-cleanup branch from 7037869 to f7e5951 Compare January 5, 2024 15:10
@ffranr
Copy link
Contributor Author

ffranr commented Jan 5, 2024 •
edited
Loading

Thanks for the refactor! I think the first commit definitely makes sense. But the rest IMO make the already hard to follow flow even trickier... What do you think of my suggestion to instead just feed the new proof into the main event loop to assert cleanup of the events?

Thanks for the review! I've gone with your idea. Reverted c.events and used the proof channel.

I'm still keen on the mapProofToEvent -> handleNewProof + mapProofToEvent + setReceiveCompleted refactor. I think mapProofToEvent is simplest if it only maps to an event, without side effects. Let me know what you think.

@ffranr ffranr requested a review from guggero January 5, 2024 15:17
@ffranr ffranr force-pushed the custodian-events-cleanup branch from f7e5951 to f2e8beb Compare January 5, 2024 15:46
@ffranr
tapgarden: handle proof received from courier using subscription channel
58d4baa 
This commit ensures that a proof received within a proof courier
goroutine is fed into the proof subscription channel. This ensures that
any event associated with the new proof will be cleaned up.
@ffranr ffranr force-pushed the custodian-events-cleanup branch from f2e8beb to 58d4baa Compare January 5, 2024 15:47
@ffranr
Copy link
Contributor Author

ffranr commented Jan 5, 2024

I've taken a look at #726 and I've reduced the scope of this PR so that I don't get in its way: https://github.com/lightninglabs/taproot-assets/pull/726/files#diff-47500066f2e4b70c65d3c7777f4478203365bdfda9f82ae6602e9e853792efeb

In other words, in this PR I've reverted my modifications to mapProofToEvent.

guggero
guggero approved these changes Jan 9, 2024
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Very nice, LGTM 🎉

GeorgeTsagk
GeorgeTsagk approved these changes Jan 10, 2024
View reviewed changes
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

Nice clean-up, LGTM! 🥕

@guggero guggero added this pull request to the merge queue Jan 10, 2024
Merged via the queue into main with commit 76cef1b Jan 10, 2024
14 checks passed
@jharveyb jharveyb deleted the custodian-events-cleanup branch January 10, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guggero guggero guggero approved these changes

@GeorgeTsagk GeorgeTsagk GeorgeTsagk approved these changes

Assignees

@ffranr ffranr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ffranr @guggero @GeorgeTsagk