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.
| /* 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?
When building a tx to spend *now* we shouldn't include CSV locked utxos as we want them to be immediately spendable.
We were seeing occassional spends of commitment tx utxos in anchorspend txs, which was causing occasional test fails. We shouldn't be sending anchorspends at all for onchain funding-spends. The logic is in place for this, but the ordering of the state update to channel was incorrect, which lead to loglines as follows (we'd expect to see failure in ONCHAIN not NORMAL), given that the reason=onchain... "Peer permanent failure in CHANNELD_NORMAL: Funding transaction spent (reason=onchain)" A quick reordering of the state change + firing of `drop_to_chain` fixes the issue of sending the anchorspend tx, which removes the race case of onchaind adding utxos from the commitment tx to the utxoset, where the anchorspend construction was picking them up. Fixes ElementsProject#7526
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:
***@***.***>
|
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