Skip to content

Conversation

yyforyongyu
Copy link
Member

@yyforyongyu yyforyongyu commented Aug 29, 2022

This PR adds the ability to customize sweeper config, which is needed to set up special cases for our itest. This can also be useful to customize the sweeper behavior.

We may also consider making a RPC server for sweeper to have better control over the sweeping process.

Copy link
Collaborator

@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 refactor!
Just a few thoughts around the safety aspect of this.

@Crypt-iQ
Copy link
Collaborator

Crypt-iQ commented Aug 29, 2022

Can we make these configurable only with like an itest build flag? I feel like users will footgun themselves if they can configure something like DefaultMaxSweepAttempts. If this particular limit is reached, then the contract resolvers will "give up" until a restart, which can be dangerous if the limit is low

@Roasbeef
Copy link
Member

Nice, I've run into some issues w/ this in the past when a sweep actually comes after some target event, and then needs to backoff, which causes a test failure or some non-deterministic behavior.

@Roasbeef Roasbeef added this to the v0.16.0 milestone Aug 29, 2022
@yyforyongyu
Copy link
Member Author

yyforyongyu commented Aug 29, 2022

Can we make these configurable only with like an itest build flag? I feel like users will footgun themselves if they can configure something like DefaultMaxSweepAttempts. If this particular limit is reached, then the contract resolvers will "give up" until a restart, which can be dangerous if the limit is low

Probably shouldn't put more changes than needed in this PR, so the other config options are now removed. Also does this mean DefaultMaxSweepAttempts of 10 is sometimes not enough?

Still thinking about the RPC server, or at least creating a new method to allow fetching the UtxoSweeper.pendingInputs. This way we could do more explicit checks over the sweeping txes rather than relying on checking the mempool. A future PR tho.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🐊

@Roasbeef
Copy link
Member

Needs a rebase!

@yyforyongyu
Copy link
Member Author

Needs a rebase!

Done!

@guggero guggero merged commit 9d04b0c into lightningnetwork:master Sep 1, 2022
@yyforyongyu yyforyongyu deleted the sweepr-config branch September 1, 2022 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants