Add OutputSweeper utility persisting and sweeping spendable outputs - #2825
Conversation
|
Note Reviews PausedUse the following commands to manage reviews:
WalkthroughThe codebase introduces an Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- lightning/src/util/mod.rs (1 hunks)
- lightning/src/util/persist.rs (1 hunks)
- lightning/src/util/sweep.rs (1 hunks)
Additional comments: 15
lightning/src/util/mod.rs (1)
- 24-24: The addition of the
sweepmodule is noted and appears to be correctly placed within theutilmodule structure.lightning/src/util/sweep.rs (13)
7-10: The module documentation for
OutputSweeperclearly explains its purpose and functionality.11-23: The imports and dependencies for the
OutputSweeperare correctly listed and seem appropriate for the functionality described.30-31: The constant
DEFAULT_REGENERATE_SPEND_THRESHOLDis well-documented and its purpose is clear.33-68: The
TrackedSpendableOutputstruct is well-defined with clear documentation for each field. It encapsulates the state of a spendable output.70-108: The methods
to_watched_outputandis_spent_inwithinTrackedSpendableOutputare logically sound and well-implemented.110-119: The implementation of
impl_writeable_tlv_based!forTrackedSpendableOutputis correct and ensures that the struct can be serialized and deserialized.121-288: The
OutputSweeperstruct and its constructor methods are well-documented and structured. The use of generics and trait bounds is appropriate, and the code is clean and maintainable.291-341: The method
track_spendable_outputsis logically sound, with clear handling of theexclude_static_outputsflag. Error handling with logging is also correctly implemented.343-346: The method
tracked_spendable_outputsprovides a simple and effective way to retrieve the current state of tracked outputs.348-428: The method
rebroadcast_if_necessarycontains logic to rebroadcast transactions as needed. The logic is sound, but it's important to ensure that the error handling in the persistence calls does not inadvertently swallow errors that should cause a failure in the calling code.Verification successful
The
persist_infomethod in theOutputSweeperutility logs errors encountered during the persistence process and then returns those errors to the caller. This means that the method does not swallow errors; instead, it allows the calling code to handle the error as deemed appropriate. This addresses the concern raised in the review comment regarding error handling.* 430-485: The method `prune_confirmed_outputs` and `persist_info` are well-implemented with appropriate error handling and logging.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the error handling in the persistence calls of rebroadcast_if_necessary does not swallow critical errors. # This script is a placeholder for the actual verification process, which would involve a deeper review of the error handling strategy. echo "Review the error handling strategy for persistence calls in rebroadcast_if_necessary."Length of output: 180
Script:
#!/bin/bash # Examine the implementation of `self.persist_info` to understand how errors are handled. ast-grep --lang rust --pattern 'impl OutputSweeper { $$$ fn persist_info($_: &$_) -> $_ { $$$ } $$$ }'Length of output: 116
Script:
#!/bin/bash # Use rg to search for the persist_info method within the OutputSweeper implementation. rg "fn persist_info"Length of output: 134
Script:
#!/bin/bash # Extract the full implementation of the persist_info method from the sweep.rs file. rg -A 10 "fn persist_info" lightning/src/util/sweep.rsLength of output: 399
Script:
#!/bin/bash # Extend the rg command to capture more lines following the persist_info method to see the complete error handling logic. rg -A 20 "fn persist_info" lightning/src/util/sweep.rsLength of output: 636
488-589: The implementations of the
ListenandConfirmtraits forOutputSweeperare correct and handle block connection/disconnection and transaction confirmation/unconfirmation appropriately.613-651: The function
read_spendable_outputsand the helperid_to_hex_stringare correctly implemented and handle errors appropriately.lightning/src/util/persist.rs (1)
- 68-77: The addition of the constants
SPENDABLE_OUTPUT_INFO_PERSISTENCE_PRIMARY_NAMESPACEandSPENDABLE_OUTPUT_INFO_PERSISTENCE_SECONDARY_NAMESPACEis consistent with the existing pattern for defining namespaces for persistence. These constants are used to namespace the persistence ofTrackedSpendableOutputs by theOutputSweeper. The primary namespace is set to"spendable_outputs", and the secondary namespace is an empty string, which is a valid and common pattern in this codebase.
0df5bcc to
da27965
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- lightning/src/util/mod.rs (1 hunks)
- lightning/src/util/persist.rs (1 hunks)
- lightning/src/util/sweep.rs (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- lightning/src/util/mod.rs
- lightning/src/util/persist.rs
- lightning/src/util/sweep.rs
da27965 to
ed9f8bb
Compare
|
@coderabbitai pause |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2825 +/- ##
==========================================
- Coverage 89.42% 89.18% -0.25%
==========================================
Files 117 118 +1
Lines 96290 96920 +630
Branches 96290 96920 +630
==========================================
+ Hits 86109 86437 +328
- Misses 7962 8240 +278
- Partials 2219 2243 +24 ☔ View full report in Codecov by Sentry. |
dc0340e to
c5052e2
Compare
|
Added first basic testcase in the |
29b4a7a to
0626f34
Compare
jbesraa
left a comment
jbesraa
left a comment
There was a problem hiding this comment.
Concept ACK
and proposed to review club lightningdevkit/ldk-review-club#28
0626f34 to
ca91946
Compare
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Basically LGTM, I assume this is already in ldk-node so there's no use commenting on particular storage format?
| self.outputs.lock().unwrap().clone() | ||
| } | ||
|
|
||
| fn rebroadcast_if_necessary(&self) { |
There was a problem hiding this comment.
Can we make the broadcasting optional here so that users can use the utility to just spend when they need to spend? Would require just letting the user skip this and then refactoring the spend logic to take an extra (few) output(s) when called manually.
There was a problem hiding this comment.
I'm not quite sure what you're suggesting. What would be the benefit of skipping the broadcast, and how would it then be different from KeyManager::spend_spendable_output? Given that broadcasting is always fallible without us getting feedback until we see a confirmation, what would be a scenario where rebroadcasting every block isn't desired? Also, what do you mean with 'extra few outputs'?
There was a problem hiding this comment.
how would it then be different from KeyManager::spend_spendable_output?
Mostly just that there would be a utility to handle the storage for you. I agree its not a big difference and certainly anyone managing their own storage can just do that.
Given that broadcasting is always fallible without us getting feedback until we see a confirmation, what would be a scenario where rebroadcasting every block isn't desired? Also, what do you mean with 'extra few outputs'?
I didn't mean not rebroadcasting, only that it'd be nice to let users delay broadcasting until they want to do some other onchain transaction and can just include the sweeping then.
| } | ||
|
|
||
| if let Some(latest_broadcast_height) = output_info.latest_broadcast_height { | ||
| // Re-generate spending tx after regenerate_spend_threshold, rebroadcast |
There was a problem hiding this comment.
If we end up with some output that got claimed another way, won't this mean all our future spends are gonna get stuck?
There was a problem hiding this comment.
Intermittently yes, but we'll still pick up on any confirmed spends from which point on we wouldn't try to include the outputs in the spending transaction regeneration. (Note that in Confirm::transactions_confirmed here we don't match on the txid but check for any confirmed transaction whether it spends one of our tracked outputs).
We'd then also prune the 'stuck' outputs once the (potentially third-party) spend is sufficiently confirmed.
If that's not acceptable we could of course fall back to individually retry to spend & broadcast outputs that previously failed, however, I'd rather not go that route if it's not really necessary as generating one spend per output is really wasteful fee-wise.
Yes, essentially. We could of course always find some migration steps if it would be really necessary though. Is there anything in particular that stands out to you? |
ca91946 to
f418234
Compare
|
Rebased on main to resolve minor conflict. |
f418234 to
3db8480
Compare
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Needs another rebase, sorry about that. Basically LGTM, I was just wondering if I should spend time trying to puzzle over a simpler (read: less stateful) design or if this is kinda fixed and we should just ship it as long as its good enough (it more than is).
Alright. Actually I had originally started out with keeping more state and tracking in a proper STM, which had some benefits w.r.t. explicitness of transitions and would give the user access to complimentary information, but that was reduced during the review on LDK Node. If there's no major concern it's probably best to keep it as is (read: not unnecessarily break compat with LDK Node's sweeper). |
3db8480 to
eff8db9
Compare
|
Rebased on |
f107d77 to
dd09907
Compare
| ) -> Result<Self, DecodeError> { | ||
| let (broadcaster, chain_data_source, kv_store, logger, spend_outputs_callback) = args; | ||
| let state = SweeperState::read(reader)?; | ||
| let best_block = state.best_block; |
There was a problem hiding this comment.
I wonder if we should enforce this instead by defining a trait for returning the BestBlock(or even add a best_block method to chain::Listen / chain::Confirm) such that our utilities query it rather than relying on the user providing it paired with the given type. Returning it as part of Readable doesn't prevent the user from doing the wrong thing.
| if i == num_blocks { | ||
| node.node.best_block_updated(&header, height); | ||
| node.chain_monitor.best_block_updated(&header, height); | ||
| node.sweeper.best_block_updated(&header, height); |
There was a problem hiding this comment.
Are we only testing this using lightning-background-processor because our functional tests utilities don't allow additional chain::Listen/chain::Confirm implementations?
There was a problem hiding this comment.
This is one reason, but also as we require a 'real' KeysManager allowing to spend the outputs. Essentially we require most of the core components of a Lightning node, and our BP tests seems to be the only place where all come together?
986bfcd to
2c8fffc
Compare
|
Now updated to
|
3ef6e21 to
4dfcfc0
Compare
| state_lock.best_block = BestBlock::new(header.block_hash(), height); | ||
| self.prune_confirmed_outputs(&mut *state_lock); |
There was a problem hiding this comment.
Maybe not worth it, but could use the result of these to determine if persistence can be skipped. Or do we always need to persist when the best block is updated?
There was a problem hiding this comment.
Hmm, yeah, previously I left this one alone as I assumed we'd want to persist the BestBlock. Whether we could do that lazily might be debatable. Note it would only safe a single persist call on each block connect if we don't have any outputs that are currently broadcasted, as otherwise we'd always update latest_broadcast_height anyways.
If we want to do it lazily, we might need to consider introducing an interval in which we force persisting the best block, as otherwise long-running nodes with low traffic might not persist the BestBlock for a long time, and we'd then force them to (unnecessarily) resync a lot of chain data on restart.
4dfcfc0 to
e362dac
Compare
commented
Apr 17, 2024
|
Addressed feedback and rebased to resolve minor conflicts. |
| /// Will be `None` if no `channel_id` was given to [`OutputSweeper::track_spendable_outputs`] | ||
| pub channel_id: Option<ChannelId>, | ||
| /// The current status of the output spend. | ||
| pub status: OutputSpendStatus, |
There was a problem hiding this comment.
Hmm, so I can't say I'm a huge fan of all the internal state being publicly exposed, it means we don't really do any validation at all of what the user tells us the state of an output is and all the debug assertions in this file are reachable if the user simply provides some garbage.
If we want to expose them all so that users can inspect using tracked_spendable_outputs maybe we should make from_outputs #[doc(hidden)] so that users are discouraged from passing invalid state that we can't verify?
There was a problem hiding this comment.
Hmm, so I can't say I'm a huge fan of all the internal state being publicly exposed, it means we don't really do any validation at all of what the user tells us the state of an output is and all the debug assertions in this file are reachable if the user simply provides some garbage.
If we want to expose them all so that users can inspect using
tracked_spendable_outputsmaybe we should makefrom_outputs#[doc(hidden)]so that users are discouraged from passing invalid state that we can't verify?
I think we def. want to expose the TrackedSpendableOutputs to allow users insight into what state their funds are in. Currently (and otherwise going forward), the funds just disappear and reappear at some magic time in the user's on-chain wallet if everything goes according to plan. To make this more transparent, I think it's important to give full insight into what's happening (and even allow them to grab the Transaction and broadcast it out-of-band if something goes wrong). This is one reason why I previously was hesitant to make too many assumptions about what status a transaction is in, if we don't have explicit data on it.
That said, if you're worried users would feed us bogus data, we can probably drop from_outputs from the API now that we implemented ReadableArgs on OutputSweeper. Now added a fixup doing so.
There was a problem hiding this comment.
That said, if you're worried users would feed us bogus data, we can probably drop from_outputs from the API now that we implemented ReadableArgs on OutputSweeper. Now added a fixup doing so.
Don't you need it for ldk-node?
There was a problem hiding this comment.
Don't you need it for ldk-node?
By now I kind of gave up on the idea of reconstructing the new data types from the old persisted state and would probably just re-add the persisted descriptors without any prior history, which I think it should work fine as a one-time migration step and seems cleaner than having a hidden API call lying around here.
| } | ||
|
|
||
| /// Returns whether the output is spent in the given transaction. | ||
| pub fn is_spent_in(&self, tx: &Transaction) -> bool { |
There was a problem hiding this comment.
What's the reason for this to be pub?
There was a problem hiding this comment.
For one it it may be a useful util to have. For example, I used it in the tests in BP. If you'd rather not have it pub, happy to either copy over the code or do a cfg(test)/cfg(not(test)) dance.
There was a problem hiding this comment.
I guess? Just seems like a kind of random utility to expose.
There was a problem hiding this comment.
I guess? Just seems like a kind of random utility to expose.
True, happy to go either way really. Really not sure what I prefer here (keeping it pub, copying code, adding cfg boilerplate). 🤷♂️
e362dac to
71b3225
Compare
commented
Apr 18, 2024
|
LGTM, feel free to squash. |
.. we move `spend_spendable_outputs` to a new trait which we can easily reuse in `OutputSweeper` later.
.. which users should implement on their on-chain wallet to allow us to retrieve a new change destination script.
71b3225 to
9a24220
Compare
commented
Apr 18, 2024
Squashed without further changes. |
We add a `OutputSweeper` utility that allows to track the state of spendable output descriptors as emitted by `Event::SpendableOutputs`. To this end, the `OutputSweeper` persists the necessary information in our `KVStore` and regularly tries to sweep the spendable outputs, removing them after reaching threshold confirmations, i.e., `ANTI_REORG_DELAY`.
.. which enables users to batch output spends.
.. we simply check that the `OutputSweeper` generates a spending tx and that the `TrackedSpendableOutput` is pruned once it reaches `ANTI_REORG_DELAY`.
9a24220 to
be8c0d0
Compare
commented
Apr 18, 2024
|
Force pushed without any net changes moving the |
5 participants
We add a
OutputSweeperutility that allows to track the state of spendable output descriptors as emitted byEvent::SpendableOutputs.To this end, the
OutputSweeperpersists the necessary information in ourKVStoreand regularly tries to sweep the spendable outputs, removing them after reaching threshold confirmations, i.e.,ANTI_REORG_DELAY.This is a generalized version of the module previously added to LDK Node: lightningdevkit/ldk-node#152.
TODOs: