Fix: don't send anchorspends for onchain commitment txs #7593
Conversation
| if not anchors: | ||
| tags = check_utxos_channel(l1, [channel_id], expected_1) | ||
| check_utxos_channel(l2, [channel_id], expected_2, tags) | ||
| tags = check_utxos_channel(l1, [channel_id], expected_1) |
There was a problem hiding this comment.
we can now re-activate this part of the test, since we're no longer sending out anchorspend txs, which were causing this to fail.
wallet/wallet.c
Outdated
| /* Don't add csv-locked ones */ | ||
| if (utxo_is_csv_locked(utxo, blockheight)) | ||
| continue; |
There was a problem hiding this comment.
Could this be a fix for: #6973?
rustyrussell
force-pushed
the
nifty/anchor-acct
branch
from
84f55b1 to
a7c25bf
Compare
|
Changed milestone to next point release, since this fixes a nasty issue. |
|
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! |
|
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. |
|
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
<#7593 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIMAKJFXIR2WDDVOTY4U6TZXSHIZAVCNFSM6AAAAABM2IBER2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRUGYZTSMZUGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
OK, I've solved this a different (more intrusive way). Christian already changed the code so we don't send our own unilateral close tx when we have already seen a spend onchain, which basically resolves this (since we don't create anchors in that case either). Changing the order made some messages make less sense, e.g. channel_fail_permanent prints the current state of the channel, and this doesn't work any more. |
rustyrussell
force-pushed
the
nifty/anchor-acct
branch
2 times, most recently
from
42f3339 to
17d89a1
Compare
We don't need to change these txs. Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
Cleans up the API: we have two functions now, one which is explicitly for "I'm failing this because I saw this tx onchain". Now we can correctly report the tx which closed the channel (previously we would always report our own tx(s)!). Signed-off-by: Rusty Russell <[email protected]> Changelog-Fixed: JSON-RPC: `close` now correctly reports the txid of the remote onchain unilateral tx if it races with a peer close. Changelog-Fixed: Protocol: we no longer try to spend anchors if a commitment tx is already mined (reported by @niftynei). Fixes: ElementsProject#7526
rustyrussell
force-pushed
the
nifty/anchor-acct
branch
from
17d89a1 to
c7b112d
Compare
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