search

ElementsProject / lightning

Core Lightning — Lightning Network implementation focusing on spec compliance and performance
Other
2.81k stars 889 forks source link

Fix: don't send anchorspends for onchain commitment txs #7593

Open niftynei opened 1 month ago

niftynei commented 1 month ago

Ordering we were using for setting the channel state was causing us to send anchorspend transactions for onchain commitments.

This would occasionally show up in the CI as a flake, due to a race condition with onchaind adding commitment tx utxos that were then picked up in the anchorspend construction, which would fail due to bookkeeper assertions around spent/unspent utxos. (See #7526)

Also includes a fix to not include currently CSV locked outputs in the construction of anchorspends.

Fixes #7526

rustyrussell commented 1 day ago

Changed milestone to next point release, since this fixes a nasty issue.

rustyrussell commented 6 hours ago

This is buggy, hence the CI failures.

"2024-09-20T03:41:23.4480770Z lightningd-2 2024-09-20T03:18:40.405Z UNUSUAL 0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518-chan#1: Not dropping our unilateral close onchain since we already saw theirs confirm."

But l2 is the one which did the unilateral close, so this is wrong!

rustyrussell commented 5 hours ago

OK, I think the premise here is wrong.

We should consider sending anchorspends after broadcast, and we do, if we consider the feerate insufficient. We don't if we've seen the tx mined, though. I don't see the problem?

I'll cherry pick the csv spend fix into a separate PR for this point release.

EDIT: I think I might understand some of this. Christian recently changed this code not to re-xmit commitment transactions once we've seen one onchain, which itself removes many cases where we would spend the anchors.

niftynei commented 1 hour ago

There’s a check that the anchorspend isn’t sent if the channel is onchain but the check happens before the state is updated to onchain, so we’re sending anchorspends out for commitment txs that are already confirmed, which is wrong/wasteful.

This PR changes the state update to before the check but breaks a bunch of tests that are asserting on a logline print which relies on the state being in NORMAL (not ONCHAIN) when logged.

On Sat, Sep 21, 2024 at 06:26 Rusty Russell @.***> wrote:

OK, I think the premise here is wrong.

We should consider sending anchorspends after broadcast, and we do, if we consider the feerate insufficient. We don't if we've seen the tx mined, though. I don't see the problem?

I'll cherry pick the csv spend fix into a separate PR for this point release.

— Reply to this email directly, view it on GitHub https://github.com/ElementsProject/lightning/pull/7593#issuecomment-2364639341, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIMAKJFXIR2WDDVOTY4U6TZXSHIZAVCNFSM6AAAAABM2IBER2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRUGYZTSMZUGE . You are receiving this because you authored the thread.Message ID: @.***>