lightningdevkit / ldk-sample

Sample node implementation using LDK
Apache License 2.0
166 stars 94 forks source link

Make SpendableOutput claims more robust #103

Closed TheBlueMatt closed 1 year ago

TheBlueMatt commented 1 year ago

Rather than only trying once to claim a SpendableOutputDescriptor, try in a loop forever. This is important as feerates rise as a sweep may fail and fall out of the mempool to be lost forever.

This is currently entirely untested, will need to do that before we land it.

TheBlueMatt commented 1 year ago

I did manage to get one claim through this pipeline on testnet, but not a batch one. Would appreciate some review but will hold it until i get a batch claim through to actually merge.

TheBlueMatt commented 1 year ago

Squashed and pushed with some comment fixes:

$ git diff-tree -U1 49b9e76 353ca38
diff --git a/src/main.rs b/src/main.rs
index 33da6f6..c31f58c 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -805,3 +805,4 @@ async fn start_ldk() {
            // Don't bother trying to announce if we don't have any public channls, though our
-           // peers should drop such an announcement anyway.
+           // peers should drop such an announcement anyway. Note that announcement may not
+           // propagate until we have a channel with 6+ confirmations.
            if chan_man.list_channels().iter().any(|chan| chan.is_public) {
diff --git a/src/sweep.rs b/src/sweep.rs
index f21436d..207a50c 100644
--- a/src/sweep.rs
+++ b/src/sweep.rs
@@ -83,2 +83,7 @@ pub(crate) async fn periodic_sweep(
        // them.
+       // Note that here we try to claim each set of spendable outputs over and over again
+       // forever, even long after its been claimed. While this isn't an issue per se, in practice
+       // you may wish to track when the claiming transaction has confirmed and remove the
+       // spendable outputs set. You may also wish to merge groups of unspent spendable outputs to
+       // combine batches.
        if let Ok(dir_iter) = fs::read_dir(&spendables_dir) {
TheBlueMatt commented 1 year ago

Should I go ahead and squash this?

tnull commented 1 year ago

Should I go ahead and squash this?

Excuse the delay! Yes, please go ahead!