multi: make sure HTLCs are locked in the itest#9602
Conversation
yyforyongyu
added
rpc
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Pull reviewers statsStats of the last 30 days for lnd:
|
guggero
left a comment
guggero
left a comment
There was a problem hiding this comment.
Thanks for squashing more flake bugs!
| } | ||
|
|
||
| flakeSkipPendingSweepsCheckDarwin(ht, bob, numSweeps) | ||
| ht.AssertNumPendingSweeps(bob, numSweeps) |
There was a problem hiding this comment.
Hmm, sorry if this is a misunderstanding on my end. But how does AssertNumPendingSweeps relate to the other changes in AssertNumActiveHtlcs in this commit?
There was a problem hiding this comment.
Good q - it's indeed deeply hidden, and I think I missed adding extra AssertNumActiveHtlcs checks so I added them. Also reorder the last two commits to make it more clear. Long story short, they share a similar cause, tho different in details, I commented on each of the cases independently to make them more clear.
yyforyongyu
force-pushed
the
yy-more-flakes
branch
from
39f57cb to
80ea75e
Compare
yyforyongyu
force-pushed
the
yy-pending-remote-commit
branch
from
1499b3b to
c26e0eb
Compare
| } | ||
|
|
||
| flakeSkipPendingSweepsCheckDarwin(ht, bob, numSweeps) | ||
| ht.AssertNumPendingSweeps(bob, numSweeps) |
There was a problem hiding this comment.
Case one: Prior to the LockIn check, here Bob would have an HTLC on his pending remote commit if the test runs quickly, causing him to create another request that sweeps his anchor output from the pending remote commit, which fails the test.
| } | ||
|
|
||
| flakeSkipPendingSweepsCheckDarwin(ht, carol, numSweeps) | ||
| ht.AssertNumPendingSweeps(carol, numSweeps) |
There was a problem hiding this comment.
Case two: Similar to the above case, Carol would need to make sure the HTLC has locked in at L737 so there's no pending remote commit. In addition, we assert the HTLC is still there when she settles it with the preimage. However, this AssertNumActiveHtlcs only asserts that the HTLC is still there, it doesn't assert the HTLC is now settled with a preimage, which means the new commitment dance may still be in the process while we now check her sweep requests. This happens because, previously we would create another pending remote commit for this settlement, although the link is already broken. This behavior is now fixed in the previous commit - we skip it when the link shuts down.
| } | ||
|
|
||
| flakeSkipPendingSweepsCheckDarwin(ht, carol, numSweeps) | ||
| ht.AssertNumPendingSweeps(carol, numSweeps) |
There was a problem hiding this comment.
This is the same case as case two.
| } | ||
|
|
||
| flakeSkipPendingSweepsCheckDarwin(ht, bob, numSweeps) | ||
| ht.AssertNumPendingSweeps(bob, numSweeps) |
There was a problem hiding this comment.
This is the same as case one.
yyforyongyu
force-pushed
the
yy-pending-remote-commit
branch
from
c26e0eb to
36d0ba6
Compare
yyforyongyu
force-pushed
the
yy-more-flakes
branch
from
80ea75e to
49382a8
Compare
yyforyongyu
force-pushed
the
yy-pending-remote-commit
branch
from
36d0ba6 to
bcc80e7
Compare
In this commit, we add a new field `LockedIn` on HTLCs so it can be used to decide whether an HTLC found on the local commitment has been committed on the remote commitment.
This commit makes sure when processing resolutions, e.g, settling invoices, when the link is already broken, the process would exit with an error. This fixes the issue we found in the itest, where an unexpected empty remote pending commitment was created although the remote peer is already offline.
Now that we have the new RPC to assert the HTLC state, this flake should be fixed.
yyforyongyu
force-pushed
the
yy-pending-remote-commit
branch
from
bcc80e7 to
3b7f9e1
Compare
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
I think we need to enhance the key for the htlc map with its index, otherwise good to go
| uint64 forwarding_htlc_index = 7; | ||
|
|
||
| /* | ||
| Whether the HTLC is locked in. An HTLC is considered locked in when the |
There was a problem hiding this comment.
Q: So are we talking about locked_in for outgoing HTLCs here, and would that mean it is considered as locked_in even if it's only on one commitment transaction e.g. when sending the HTLCs out? My understanding of locked-in was always that the HTLC has to be committed on both sides of the commitment ?
| // used to decide whether the HTLCs from the local commitment has been | ||
| // locked in or not. | ||
| remoteHTLCs := fn.NewSet[[32]byte]() | ||
| for _, htlc := range dbChannel.RemoteCommitment.Htlcs { |
There was a problem hiding this comment.
I think we can we have multiple HTLCs with the same hash (MPP), then I think we cannot rely on this information ? Probably we need to also add the htlcIndex as a key here
There was a problem hiding this comment.
IIUC this is used mainly in the itests right now, and in all cases we end up using unique payment hashes.
There was a problem hiding this comment.
created a short test, it seems like they end up with the same hash:
{
"channels": [
{
"active": true,
"remote_pubkey": "02373b8ebde6e9c5c0398a3bdababc2182530af63679fd166d9adf18fa6184379c",
"channel_point": "ac23a5759face3e0379a1895b8b08ff4338a9830b89d9d4b19f79b6a4fe1ad74:0",
"chan_id": "74ade14f6a9bf7194b9d9db830988a33f48fb0b895189a37e0e3ac9f75a523ac",
"scid": "359540302348288",
"scid_str": "327x1x0",
"capacity": "10000000",
"local_balance": "9495530",
"remote_balance": "500000",
"commit_fee": "3810",
"commit_weight": "1116",
"fee_per_kw": "2500",
"unsettled_balance": "1000",
"total_satoshis_sent": "0",
"total_satoshis_received": "0",
"num_updates": "32",
"pending_htlcs": [
{
"incoming": false,
"amount": "100",
"hash_lock": "4ee4f8273264c66d5065cec2098906e826cb6aa6ff5381c78cf886dea1813e6f",
"expiration_height": 427,
"htlc_index": "100",
"forwarding_channel": "0",
"forwarding_htlc_index": "0"
},
{
"incoming": false,
"amount": "100",
"hash_lock": "4ee4f8273264c66d5065cec2098906e826cb6aa6ff5381c78cf886dea1813e6f",
"expiration_height": 427,
"htlc_index": "101",
"forwarding_channel": "0",
"forwarding_htlc_index": "0"
},
{
"incoming": false,
"amount": "100",
"hash_lock": "4ee4f8273264c66d5065cec2098906e826cb6aa6ff5381c78cf886dea1813e6f",
"expiration_height": 427,
"htlc_index": "102",
"forwarding_channel": "0",
"forwarding_htlc_index": "0"
},
{
"incoming": false,
"amount": "100",
"hash_lock": "4ee4f8273264c66d5065cec2098906e826cb6aa6ff5381c78cf886dea1813e6f",
"expiration_height": 427,
"htlc_index": "103",
"forwarding_channel": "0",
"forwarding_htlc_index": "0"
},
{
"incoming": false,
"amount": "100",
"hash_lock": "4ee4f8273264c66d5065cec2098906e826cb6aa6ff5381c78cf886dea1813e6f",
"expiration_height": 427,
"htlc_index": "104",
"forwarding_channel": "0",
"forwarding_htlc_index": "0"
},
{
"incoming": false,
"amount": "100",
"hash_lock": "4ee4f8273264c66d5065cec2098906e826cb6aa6ff5381c78cf886dea1813e6f",
"expiration_height": 427,
"htlc_index": "105",
"forwarding_channel": "0",
"forwarding_htlc_index": "0"
},
{
"incoming": false,
"amount": "100",
"hash_lock": "4ee4f8273264c66d5065cec2098906e826cb6aa6ff5381c78cf886dea1813e6f",
"expiration_height": 427,
"htlc_index": "106",
"forwarding_channel": "0",
"forwarding_htlc_index": "0"
},
{
"incoming": false,
"amount": "100",
"hash_lock": "4ee4f8273264c66d5065cec2098906e826cb6aa6ff5381c78cf886dea1813e6f",
"expiration_height": 427,
"htlc_index": "107",
"forwarding_channel": "0",
"forwarding_htlc_index": "0"
},
{
"incoming": false,
"amount": "100",
"hash_lock": "4ee4f8273264c66d5065cec2098906e826cb6aa6ff5381c78cf886dea1813e6f",
"expiration_height": 427,
"htlc_index": "108",
"forwarding_channel": "0",
"forwarding_htlc_index": "0"
},
{
"incoming": false,
"amount": "100",
"hash_lock": "4ee4f8273264c66d5065cec2098906e826cb6aa6ff5381c78cf886dea1813e6f",
"expiration_height": 427,
"htlc_index": "109",
"forwarding_channel": "0",
"forwarding_htlc_index": "0"
}
],
"csv_delay": 1201,
"private": false,
"initiator": true,
"chan_status_flags": "ChanStatusDefault",
"local_chan_reserve_sat": "100000",
"remote_chan_reserve_sat": "100000",
"static_remote_key": false,
"commitment_type": "ANCHORS",
"lifetime": "1058",
"uptime": "1058",
"close_address": "",
"push_amount_sat": "500000",
"thaw_height": 0,
"local_constraints": {
"csv_delay": 1201,
"chan_reserve_sat": "100000",
"dust_limit_sat": "354",
"max_pending_amt_msat": "9900000000",
"min_htlc_msat": "1",
"max_accepted_htlcs": 483
},
"remote_constraints": {
"csv_delay": 1201,
"chan_reserve_sat": "100000",
"dust_limit_sat": "354",
"max_pending_amt_msat": "9900000000",
"min_htlc_msat": "1",
"max_accepted_htlcs": 483
},
"alias_scids": [],
"zero_conf": false,
"zero_conf_confirmed_scid": "0",
"peer_alias": "My Lightning ☇",
"peer_scid_alias": "0",
"memo": "",
"custom_channel_data": ""
}
]
}
There was a problem hiding this comment.
just add the htlc_index to the key and we are gtg
| @@ -2155,6 +2155,20 @@ func (c *ChannelArbitrator) checkRemoteDanglingActions( | |||
| continue | |||
| } | |||
There was a problem hiding this comment.
Commit Msg: what do you mean by empty remote pending commitment ? A commitment without HTLCs ? Why was it send in the first place, was it a feeUpdate ?
| // used to decide whether the HTLCs from the local commitment has been | ||
| // locked in or not. | ||
| remoteHTLCs := fn.NewSet[[32]byte]() | ||
| for _, htlc := range dbChannel.RemoteCommitment.Htlcs { |
There was a problem hiding this comment.
IIUC this is used mainly in the itests right now, and in all cases we end up using unique payment hashes.
4 participants
Depends on,
Fix #9486. As seen multiple times in our CI, which is believed to be a flake,
To make sure we can properly assert HTLC states in our test, we now add a new field
LockedIninlnrpc.HTLC. When an HTLC is added to the local commit, we need to wait till the remote has sent us therevoke_and_ackso that we know for sure the HTLC is locked in before proceeding in our tests.In addition, when processing outside resolutions in our link, we now catch the link's quit signal so we don't create unexpected pending commit.