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

feeadjuster: assert can trigger #199

Closed
gallizoltan opened this issue Jan 15, 2021 · 4 comments
Closed

feeadjuster: assert can trigger #199

gallizoltan opened this issue Jan 15, 2021 · 4 comments

Comments

@gallizoltan
Copy link
Collaborator

Description

I've seen a curious line in the lightningd log:

2020-12-06T20:32:06.150Z **BROKEN** plugin-feeadjuster.py: Adjusting fees:

I've investigated the situation and figured out what happened. It caused by the assert at line 122 in feeadjuster/feeadjuster.py:

assert 0 <= percentage and percentage <= 1

Steps to reproduce

  1. Let's assume I have a channel with a total size of 100,000 Sats. 20,000 Sats are mine in that channel.
  2. I call a feeadjust. It saves my balance in plugin.adj_balances[scid] at line 198.
  3. Then an incoming payment happens: 30,000 Sats arrive into the channel. (In my case it was actually a circular payment, rebalance, but it does not matter.)
    At this point I have 50,000 Sats in the channel, but plugin.adj_balances[scid] still stores the 20,000 Sats value. The feeadjuster is not aware of the incoming payments.
  4. After these a forward event occurs: 40,000 Sats leave the channel.
    The feeadjuster is subscribed to the forward event, so it reduces my stored balance by 40,000 Sats at line 173. The result will be negative: -20,000 Sats. This will trigger the assert.

A possible solution

The easy way: it will not happen again if I turn off the automatic fee updates in feeadjuster.

@m-schmoock
Copy link
Member

Good that we put that assert in ;)

@m-schmoock
Copy link
Member

@gallizoltan Do we need to fix this by i.e. by replacing the forward_event_subscription early return in forward_event to check for this in line 178 where maybe_adjust_fees is called? Can you make a test for this?

Partly addressed by #198 (only rebalance)

@gallizoltan
Copy link
Collaborator Author

I'm not sure what would be the best solution. Maybe we should write a @plugin.subscribe("invoice_payment") and a @plugin.subscribe("sendpay_success").

@chrisguida
Copy link
Collaborator

Fixed by #525

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

No branches or pull requests

3 participants