Skip to content

Conversation

@guggero
Copy link
Contributor

@guggero guggero commented Jun 16, 2025

Fixes root cause of #1595.
Also fixes #1598.

Corresponding integration tests can be found here: lightninglabs/lightning-terminal#1089

guggero added 3 commits June 16, 2025 15:12
This commit addresses an edge case where one party only has a BTC
balance (but no asset balance). In that case the balance is allowed to
be below the dust limit. But because those sats will be added to the fee
and can't be materialized on-chain, we also can't create an allocation
for the balance. Otherwise we'll have a mismatch between the number of
allocations and the number of expected on-chain outputs of the
transaction.

This isn't an issue when there is an asset balance, as in that case we
enforce that the BTC balance is above the dust limit.
Due to (probably) a copy/paste error, we returned the wrong policy for
an invoice. We didn't notice this because in our integration tests we
never set non-default routing policies.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 15682067785

Details

  • 0 of 3 (0.0%) changed or added relevant lines in 2 files are covered.
  • 24 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.003%) to 37.292%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rpcserver.go 0 1 0.0%
tapchannel/commitment.go 0 2 0.0%
Files with Coverage Reduction New Missed Lines %
address/address.go 2 67.47%
commitment/tap.go 2 72.05%
tapfreighter/interface.go 4 48.0%
tapdb/multiverse.go 6 53.03%
tapgarden/caretaker.go 10 68.31%
Totals Coverage Status
Change from base Build 15682002381: 0.003%
Covered Lines: 27258
Relevant Lines: 73094

💛 - Coveralls

Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

Nice LGTM!

We could consider introducing distinct types for inbound and outbound policies to prevent misuse—e.g., accidentally passing an outbound policy where an inbound one is expected. That said, probably not something for this PR.

Also, do we need any regression tests (changes to tests) to cover this fix?

@guggero
Copy link
Contributor Author

guggero commented Jun 16, 2025

We could consider introducing distinct types for inbound and outbound policies to prevent misuse—e.g., accidentally passing an outbound policy where an inbound one is expected. That said, probably not something for this PR.

Yeah, that would be nice. But that would require changes in lnd and its RPC.

Also, do we need any regression tests (changes to tests) to cover this fix?

Yes, I've added them in the linked litd PR: lightninglabs/lightning-terminal#1089
Gonna check why they're currently failing.

@ffranr
Copy link
Contributor

ffranr commented Jun 16, 2025

Yeah, that would be nice. But that would require changes in lnd and its RPC.

@guggero I was thinking of just wrapper types in tap.

@guggero
Copy link
Contributor Author

guggero commented Jun 16, 2025

@guggero I was thinking of just wrapper types in tap.

Ah, okay. But we currently only use the policies in a single place, so not sure if that's a bit overkill for now.

@levmi levmi moved this from 🆕 New to 👀 In review in Taproot-Assets Project Board Jun 16, 2025
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

LGTM 🎯

// peer in the channel.
policy := edge.Node2Policy
if edge.Node2Pub == remotePubStr {
if edge.Node1Pub == remotePubStr {
Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, the direction of the policy here is relative

@Roasbeef Roasbeef merged commit c8b3b64 into main Jun 16, 2025
17 of 18 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Taproot-Assets Project Board Jun 16, 2025
@guggero guggero deleted the fix-dust-balance branch June 16, 2025 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

[bug]: The fee in the asset channel was not successfully collected.

6 participants