Skip to content
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

[bug]: SubscribeSingleInvoice and SendPaymentV2 do not wait for the channel balance to be completely updated before returning #9467

Open
ZZiigguurraatt opened this issue Jan 31, 2025 · 4 comments
Labels
architecture Related to system design

Comments

@ZZiigguurraatt
Copy link

Steps to reproduce (simplified by skipping channel reserve, etc.):

  1. Alice has 10 sat in channel
  2. Bob has 0 sat in channel
  3. Bob creates invoice for 1 sat and gives it to Alice
  4. Alice pays the invoice for 1 sat with SendPaymentV2 and waits for a response to include status: SUCCEEDED
  5. Alice uses ListChannels and checks and sees she now has 9 sat in the channel and bob still has 0
  6. Bob runs SubscribeSingleInvoice to wait for the invoice to be in state: SETTLED
  7. Bob uses ListChannels and checks and sees he still has 0 sat and alice now has 9 sat

However, if they both then

  1. Wait a while
  2. Check again with ListChannels, they both later see that bob has 1 sat as he should

If I follow the same process as above, but sending a taproot asset instead of sats, I'm seeing the same behaviour with the balance of taproot assets in the custom_channel_data field.

For some reason both alice and bob are taking longer to update bob's change in channel balance. It's fine if it takes longer to do that, but SubscribeSingleInvoice and SendPaymentV2 should block before returning state: SETTLED and status: SUCCEEDED.

This creates lots of confusion for the node operator and with automated testing.

@ZZiigguurraatt ZZiigguurraatt added bug Unintended code behaviour needs triage labels Jan 31, 2025
@ZZiigguurraatt ZZiigguurraatt changed the title [bug]: SubscribeSingleInvoice and SendPaymentV2 do not wait for the internal balance to be completely updated before returning [bug]: SubscribeSingleInvoice and SendPaymentV2 do not wait for the channel balance to be completely updated before returning Jan 31, 2025
@ziggie1984
Copy link
Collaborator

ziggie1984 commented Feb 1, 2025

Interesting observations would be interesting how much time switch (channel state maschine) lags behind.

In general I am not quite sure if we should regard this as a bug and we might question whether such a testcase is reasonable.

What you are observing is that the Switch (which handles the state transitions) needs a bit of time to exchange another round of commitment signatures to clear the htlc from the commitment state. However we are already in the possession of the preimage (bobs point of view) which means technically the payment happened. So the ListChannels cmd is not really a good representation whether a payment happened. Because imagine a Peer reveils the preimage but than goes offline and does not update the state. We still made the payment (from Alice POV), and Bob can enforce the payment onchain because he has the state of the commitment with the htlc locked in.

Maybe what we could do is, to list the htlcs in the listchannels cmd and attach a state to them, meaning that if you see an outgoing HTLCs which has the preimage attached this means that the State updated.

But I don't think we need to sync the states as you were requiring in your inital statement, because receiving the preimage is the exact point in time when the payment happened.

But maybe some other devs can jump in an share their view on this topic. Going to remove the bug tag for now because it is not a bug.

@ziggie1984 ziggie1984 added architecture Related to system design and removed bug Unintended code behaviour labels Feb 1, 2025
@ZZiigguurraatt
Copy link
Author

I get what you are saying.

I think it could be valuable to adjust ListChannels to provide more clarity.

Also, for SubscribeInvoices, SubscribeSingleInvoice and SendPaymentV2, maybe we could add another status update after returning state: SETTLED and status: SUCCEEDED that is 'OUTOFFLIGHT' or something like that? Also, if we are worried about backward compatibility, this status update would be optional and something that needs to be asked for when making a request to the RPC.

@yyforyongyu
Copy link
Member

Think this is an issue described in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture Related to system design
Projects
None yet
Development

No branches or pull requests

3 participants