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]: lease_fee double counted, causes bkpr-listbalances to crash lightningd #5557

Closed
chrisguida opened this issue Aug 30, 2022 · 4 comments
Assignees
Milestone

Comments

@chrisguida
Copy link
Contributor

chrisguida commented Aug 30, 2022

Issue and Steps to Reproduce

  1. Open private dual-funded channel using liquidity ads page on RTL 0.13.0 (this bug was fixed, in 0.13.1 RTL opens public channels, not sure if relevant)
  2. Close channel
  3. run bkpr-listbalances
  4. lightningd crashes with this message:
2022-08-30T00:06:43.169777Z 2022-08-30T00:06:43.162Z **BROKEN** plugin-bookkeeper: Get account balance returned err for account 58c817f67224a80373ad7f1eafc4c069b6ea41986a2c9404bd531a47f1997e1c: bc channel balance is negative? 23268000msat - 26536000msat
2022-08-30T00:06:43.343931Z 2022-08-30T00:06:43.339Z INFO    plugin-bookkeeper: Killing plugin: exited during normal operation
2022-08-30T00:06:43.350405Z 2022-08-30T00:06:43.339Z **BROKEN** plugin-bookkeeper: Plugin marked as important, shutting down lightningd!

getinfo output

# lightning-cli getinfo
{
   "num_peers": 26,
   "num_pending_channels": 0,
   "num_active_channels": 21,
   "num_inactive_channels": 0,
   "address": [
      {
         "type": "torv3",
         "port": 9735
      },
      {
         "type": "ipv4",
      }
   ],
   "binding": [
      {
         "type": "ipv4",
         "address": "0.0.0.0",
         "port": 9735
      }
   ],
   "version": "v0.12.0",
   "blockheight": 751861,
   "network": "bitcoin",
   "lightning-dir": "/root/.lightning/bitcoin",
   "our_features": {
      "init": "0200000000000008a080282269a2",
      "node": "0200000000000088a080282269a2",
      "channel": "",
      "invoice": "02000020024100"
   }
}

bkpr-listaccountevents 58c817f67224a80373ad7f1eafc4c069b6ea41986a2c9404bd531a47f1997e1c shows that the least fee was counted twice.

deleting the duplicate lease_fee entry from the db fixes the issue.

cc @niftynei

@niftynei niftynei self-assigned this Sep 12, 2022
@niftynei niftynei added this to the v0.12.1 milestone Sep 12, 2022
@niftynei
Copy link
Contributor

niftynei commented Sep 13, 2022

Ok got a test case that reproduces this, thinking about a fix. Suspect that you had initiated the channel open and then upgraded to v12 very soon after, but before the channel had opened.

We wrote down the channel info once you restarted the node, even tho it wasn't open yet. When the channel actually confirmed onchain, we re-issued duplicate events. We ignore duplicate onchain events (so the channel_open event itself doesn't get written twice) but dont do any de-duping for channel events, which the lease_fee is, so we ended up with a duplicate just for this event.

Current thinking is to not record the channel until it's definitely open, which would avoid this entirely.

niftynei added a commit to niftynei/lightning that referenced this issue Sep 13, 2022
Reproduce crash for ElementsProject#5557!

If we record the channel open because bookkeeper was added after the
channel open request started but the channel confirms later, we end up
with re-recording any associated push or leased fees (paid or rcvd).

In the case where you've paid for these fees, your channel balance goes
negative and the node crashes the next time you call `listbalances`.

Reported-by: @chrisguida
niftynei added a commit to niftynei/lightning that referenced this issue Sep 13, 2022
Clean up for ElementsProject#5557.

If you've got duplicate 'lease_fee' entries, we delete them!
niftynei added a commit to niftynei/lightning that referenced this issue Sep 13, 2022
Reproduce crash for ElementsProject#5557!

If we record the channel open because bookkeeper was added after the
channel open request started but the channel confirms later, we end up
with re-recording any associated push or leased fees (paid or rcvd).

In the case where you've paid for these fees, your channel balance goes
negative and the node crashes the next time you call `listbalances`.

Reported-by: @chrisguida
niftynei added a commit to niftynei/lightning that referenced this issue Sep 13, 2022
Clean up for ElementsProject#5557.

If you've got duplicate 'lease_fee' entries, we delete them!
@niftynei
Copy link
Contributor

See #5587

@chrisguida
Copy link
Contributor Author

Possible, I'd installed all of the RCs leading up to this. So, possible I opened the channel then updated from the last RC to v0.12.0 while the channel was still opening. Don't remember though.

rustyrussell pushed a commit that referenced this issue Sep 14, 2022
Reproduce crash for #5557!

If we record the channel open because bookkeeper was added after the
channel open request started but the channel confirms later, we end up
with re-recording any associated push or leased fees (paid or rcvd).

In the case where you've paid for these fees, your channel balance goes
negative and the node crashes the next time you call `listbalances`.

Reported-by: @chrisguida
rustyrussell pushed a commit that referenced this issue Sep 14, 2022
Clean up for #5557.

If you've got duplicate 'lease_fee' entries, we delete them!
@niftynei
Copy link
Contributor

niftynei commented Sep 14, 2022

marking as fixed by #5587

niftynei added a commit that referenced this issue Sep 14, 2022
Reproduce crash for #5557!

If we record the channel open because bookkeeper was added after the
channel open request started but the channel confirms later, we end up
with re-recording any associated push or leased fees (paid or rcvd).

In the case where you've paid for these fees, your channel balance goes
negative and the node crashes the next time you call `listbalances`.

Reported-by: @chrisguida
niftynei added a commit that referenced this issue Sep 14, 2022
Clean up for #5557.

If you've got duplicate 'lease_fee' entries, we delete them!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants