-
Notifications
You must be signed in to change notification settings - Fork 2.2k
sweep+rpc+cmd/lncli: expose pending input sweeps over RPC + CLI #3089
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
Conversation
PTAL @joostjager. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, just a few cosmetics related comments
lnrpc/rpc.proto
Outdated
sweep transaction of the output. | ||
*/ | ||
uint32 next_broadcast_height = 6 [json_name = "next_broadcast_height"]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we circle back and add retargeting, it may be useful to expose what the next fee rate would be.
sweep/sweeper.go
Outdated
for _, pendingInput := range s.pendingInputs { | ||
// Only the exported fields are set, as we expect the response | ||
// to only be consumed externally. | ||
pendingInputs = append(pendingInputs, &PendingInput{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same thing with creating a snapshot in mission control and introduced separate exported structs for the snapshot. I think it is cleaner. (non blocking)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that was how the initial design of the PR approached it, but I opted for reusing the same struct to result in less code, though the difference is not much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, works as advertised!
LGTM contingent in unifying the byte reversal (in lncli
as we do elsewhere) 🌊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
straightforward diff, some high level comments
@wpaulino needs rebase |
PTAL @Roasbeef @joostjager. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and found that it works. Only minor comment is that the field name sat_per_byte
remains a little vague without reading the comment. But don't know if last_sat_per_byte
would be any better.
This will serve useful when exposing the pending inputs over RPC, since we currently don't keep track of the last fee rate used for an input.
Agreed. This is why Rebased to address a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ✅
In this PR, we expose the set of pending inputs that the UtxoSweeper is currently attempting to sweep. This will serve as preparatory work for allowing users to perform fee bumps. Due to the UtxoSweeper's asynchronous nature, we are not able to quickly report on whether a fee bump succeeded or not, so we resort towards exposing its internal fee rate bucketing system over RPC to give users an indication of what's currently being swept and with what fee rate.
I opted to only expose the sweeping fee rate for each bucket, along with all of the inputs within each bucket and their individual fee rates for now. Looking for feedback on whether we could be exposing this information better.
Depends on #3026.