Skip to content

Expose a Balance for inbound HTLCs even without a preimage - #1673

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-may-learn-preimage-balance
Aug 25, 2022
Merged

Expose a Balance for inbound HTLCs even without a preimage#1673
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-may-learn-preimage-balance

Conversation

@TheBlueMatt

@TheBlueMattTheBlueMatt commented Aug 17, 2022

Copy link
Copy Markdown
Collaborator
If we don't currently have the preimage for an inbound HTLC, that
does not guarantee we can never claim it, but instead only that we
cannot claim it unless we receive the preimage from the channel we
forwarded the channel out on.
Thus, we cannot consider a channel to have no claimable balances if
the only remaining output on the commitment ransaction is an
inbound HTLC for which we do not have the preimage, as we may be
able to claim it in the future.
This commit addresses this issue by adding a new `Balance` variant
- `MaybePreimageClaimableHTLCAwaitingTimeout`, which is generated
until the HTLC output is spent.

Fixes#1620, based on #1495.

@TheBlueMattTheBlueMatt added this to the 0.0.111 milestone Aug 17, 2022
wpaulino
wpaulino reviewed Aug 17, 2022
View reviewed changes

@wpaulinowpaulino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, test_claim_value_force_close just needs to be updated to handle the new variant.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-08-may-learn-preimage-balance branch from 57f4228 to 190e6beCompareAugust 17, 2022 22:54
@TheBlueMatt

TheBlueMatt commented Aug 17, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Fixed the failing test and rebased after #1495 landed.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-08-may-learn-preimage-balance branch from 190e6be to c6440b1CompareAugust 17, 2022 23:19
wpaulino
wpaulino previously approved these changes Aug 18, 2022
View reviewed changes
dunxen
dunxen reviewed Aug 18, 2022
View reviewed changes

@dunxendunxen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new variant LGTM, I'm just not following the test 100% right now. Will give it another run-through later.

Just left a few nits.

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/ln/monitor_tests.rs Outdated
Comment threadlightning/src/ln/monitor_tests.rs Outdated
dunxen
dunxen reviewed Aug 18, 2022
View reviewed changes
Comment threadlightning/src/ln/monitor_tests.rs Outdated
dunxen
dunxen previously approved these changes Aug 18, 2022
View reviewed changes

@dunxendunxen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@dunxendunxen removed their assignment Aug 24, 2022
@valentinewallace
valentinewallace self-requested a review August 24, 2022 14:39
valentinewallace
valentinewallace reviewed Aug 24, 2022
View reviewed changes
Comment threadlightning/src/chain/channelmonitor.rs Outdated
valentinewallace
valentinewallace reviewed Aug 24, 2022
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Went through the test, LGTM besides the rename idea and a nit

Comment threadlightning/src/ln/monitor_tests.rs Outdated
@valentinewallacevalentinewallace self-assigned this Aug 24, 2022
@codecov-commenter

codecov-commenter commented Aug 24, 2022•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1673 (39cede6) into main (b84b53a) will increase coverage by 0.13%.
The diff coverage is 93.63%.

@@ Coverage Diff @@## main #1673 +/- ##
==========================================
+ Coverage 90.77% 90.90% +0.13% 
==========================================
Files 80 85 +5 Lines 44763 45888 +1125 Branches 44763 45888 +1125 ==========================================
+ Hits 40632 41715 +1083 - Misses 4131 4173 +42
Impacted FilesCoverage Δ
lightning-block-sync/src/lib.rs93.23% <ø> (ø)
lightning-block-sync/src/test_utils.rs93.46% <ø> (ø)
lightning-invoice/src/lib.rs87.39% <ø> (ø)
lightning-invoice/src/payment.rs89.93% <0.00%> (+2.16%)⬆️
lightning-net-tokio/src/lib.rs77.16% <0.00%> (+0.30%)⬆️
lightning-rapid-gossip-sync/src/lib.rs90.90% <ø> (ø)
lightning/src/chain/mod.rs68.18% <ø> (ø)
lightning/src/lib.rs100.00% <ø> (ø)
lightning/src/ln/mod.rs95.00% <ø> (ø)
lightning/src/util/chacha20poly1305rfc.rs96.25% <ø> (ø)
... and 48 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dunxen
dunxen previously approved these changes Aug 25, 2022
View reviewed changes
valentinewallace
valentinewallace reviewed Aug 25, 2022
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ACK after squash

Comment threadlightning/src/chain/channelmonitor.rs
timeout_height: htlc.cltv_expiry,
});
}
} else if htlc_resolved.is_none() {

@valentinewallacevalentinewallaceAug 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fwiw, tests pass when this is just else

@TheBlueMattTheBlueMattAug 25, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Ah, right, it'd be unreachable otherwise as we have a separate else clause for resolved.is_some && !spend_pending (and spend_pending would require that we have the preimage, which ends up in the previous clause). I'm still gonna leave it as-is if that's okay.

@valentinewallacevalentinewallaceAug 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sg, it reads fine IMO

@TheBlueMatt
Expose a Balance for inbound HTLCs even without a preimage
4c92313
If we don't currently have the preimage for an inbound HTLC, that
does not guarantee we can never claim it, but instead only that we
cannot claim it unless we receive the preimage from the channel we
forwarded the channel out on.
Thus, we cannot consider a channel to have no claimable balances if
the only remaining output on the commitment ransaction is an
inbound HTLC for which we do not have the preimage, as we may be
able to claim it in the future.
This commit addresses this issue by adding a new `Balance` variant
- `MaybePreimageClaimableHTLCAwaitingTimeout`, which is generated
until the HTLC output is spent.
Fixeslightningdevkit#1620
@TheBlueMatt
Rename MaybeClaimableHTLCAwaitingTimeout for consistency
39cede6
As we now have a MaybePreimageClaimableHTLC, its more consistent
to rename `MaybeClaimableHTLCAwaitingTimeout` to
`MaybeTimeoutClaimableHTLC`.
@TheBlueMatt

TheBlueMatt commented Aug 25, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-08-may-learn-preimage-balance branch from c7e98b8 to 39cede6CompareAugust 25, 2022 18:51
@TheBlueMatt
TheBlueMatt merged commit fa62b00 into lightningdevkit:mainAug 25, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dunxendunxendunxen approved these changes
@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@wpaulinowpaulino

Labels

None yet

Projects

None yet

Milestone

0.0.111

Development

Successfully merging this pull request may close these issues.

Expose a claimable Balance for no-preimage-but-may-learn-one

5 participants

@TheBlueMatt@codecov-commenter@dunxen@wpaulino@valentinewallace