📅 Original date posted:2023-04-06
🗒️ Summary of this message: The issue with `announcement_signatures` is similar to `commit_sig`. Storing the last_short_channel_id is crucial to ignore invalid messages until `revoke_and_ack`. Implementations need to handle this edge case correctly to avoid breaking during the splice lock process. The message flow needs to be detailed to explain protocol issues accurately. The proposed solution allows any invalid messages to come through and be ignored without warning or error.
📝 Original message:
>
> I haven't looked at the `announcement_signatures` case yet, but at least
> for the `commit_sig` case this should never be an issue.
>
Yup the issue is the same for `announcement_signatures`. Storing the
last_short_channel_id is key here so you know which announcement
message you can safely ignore and which are considered errors. This
behavior should be enacted until sending OR receiving `revoke_and_ack`.
> It only means
> that sometimes, after sending `splice_locked`, you will receive some
> `commit_sig` messages that are for commitments that you don't care about
> anymore. You should be able to safely ignore those `commit_sig`. I have
> provided more details in the gist linked.
>
Yup you should ignore invalid commit_sigs for this brief period of time but
then
go back to rejecting invalid commit_sigs after sending OR receiving
`revoke_and_ack`.
> Let me know if I'm missing something, but I believe this is simply an
> edge case that implementations need to correctly handle, not a protocol
> issue? Or maybe I'm not understanding the scenario correctly?
>
It is a critical detail to implement correctly. Implementations without
good test coverage can easily ship releases that will break / force close
randomly during the splice lock process. What's worse is this effects
the larger more valuable/busy routing nodes more often and is easy to
miss on a simple regtest node 😳.
By the way, I find your notation a bit hard to follow...I think that we
> really need to detail the exact message flow (like I did in the linked
> gist) to be able to explain protocol issues, otherwise there's always
> a risk that people think about a slightly different message flow, which
> means we'll just talk past each other...
>
The original gist from back in January is here:
https://gist.github.com/ddustin/7ee222eb31c3eac5b141c991c0937fae
Happy to add more details but the example is accurate to exactly the
messages sent during this time. Confirmed it with actual tests as well.
Is there some message that it looks like I missed here? If it helps I can
give you an actual log message flow from the real tests, but honestly,
they just make it more confusing to understand 🤷.
It might be simpler to follow by working backwards from this solution:
https://gist.github.com/ddustin/017aeadfbf34d2fcd950f1238614afe2
My original proposal from late January ^ 100% solves the issue but
adds a lock on the connection via an extra STFU step.
My current proposal is essentially "let's allow the race condition and
observe which things become invalid during it." I settled on two things
invalid events that occur:
* A stale `commitments_signed` message (bundle) with extra signatures
* A stale `announcement_sigs` message for the pre-splice channel
An "easy" solution an implementation could do is allow any invalid
`commitments_signed` or `announcement_sigs` to come through
and ignore them without warning or error. This "works" but leaves the
node in a messy state where actual errors are being squelched
and opens up a potential DoS issue.
So the ideal solution is to allow a certain class of 'stale' messages
for a period of time, that time being from:
A) mutual_splice_locked (the moment we have both sent & received
`splice_locked`)
B) A successful `commitment_signed` <-> `revoke_and_ack` round trip (in
either direction)
The definition of A is pretty straightforward and we need to detect
mutual_splice_lock anyway so that's easy.
The definition of B can be a couple of things but the simplest is I believe
the sending OR receiving of `revoke_and_ack`.
Is it a little more work to implement it this way? Yeah. But I think the
effort
is worth it to make the protocol & our nodes more robust.
Cheers,
Dusty
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linuxfoundation.org/pipermail/lightning-dev/attachments/20230406/cee4770b/attachment-0001.html>;
Dusty Daemon [ARCHIVE] /
npub1ka…lmafd
2023-06-09 13:13:03
in reply to nevent1q…xacu
Author Public Key
npub1kahkfhc5z355rglknr9mgqdymjesyf5lnszdxc75z2g7u0tz2cyqslmafdPublished at
2023-06-09 13:13:03Event JSON
{
"id": "059ab2275aa3bfc6ea5ca5a6b502b1dad97df9fce9e00bdc08d5519dca39a161",
"pubkey": "b76f64df14146941a3f698cbb401a4dcb302269f9c04d363d41291ee3d625608",
"created_at": 1686316383,
"kind": 1,
"tags": [
[
"e",
"b4b2711c48c5804f9601e328d48e4c1f3a15cdcff6a2d883ff2a4976e0400e9d",
"",
"root"
],
[
"e",
"71b58727cf136eaa27d73e38771f941f2982461c514172ff6f8e0f175dc123cf",
"",
"reply"
],
[
"p",
"f26569a10f83f6935797b8b53a87974fdcc1de6abd31e3b1a3a19bdaed8031cb"
]
],
"content": "📅 Original date posted:2023-04-06\n🗒️ Summary of this message: The issue with `announcement_signatures` is similar to `commit_sig`. Storing the last_short_channel_id is crucial to ignore invalid messages until `revoke_and_ack`. Implementations need to handle this edge case correctly to avoid breaking during the splice lock process. The message flow needs to be detailed to explain protocol issues accurately. The proposed solution allows any invalid messages to come through and be ignored without warning or error.\n📝 Original message:\n\u003e\n\u003e I haven't looked at the `announcement_signatures` case yet, but at least\n\u003e for the `commit_sig` case this should never be an issue.\n\u003e\nYup the issue is the same for `announcement_signatures`. Storing the\nlast_short_channel_id is key here so you know which announcement\nmessage you can safely ignore and which are considered errors. This\nbehavior should be enacted until sending OR receiving `revoke_and_ack`.\n\n\n\u003e It only means\n\u003e that sometimes, after sending `splice_locked`, you will receive some\n\u003e `commit_sig` messages that are for commitments that you don't care about\n\u003e anymore. You should be able to safely ignore those `commit_sig`. I have\n\u003e provided more details in the gist linked.\n\u003e\nYup you should ignore invalid commit_sigs for this brief period of time but\nthen\ngo back to rejecting invalid commit_sigs after sending OR receiving\n`revoke_and_ack`.\n\n\n\u003e Let me know if I'm missing something, but I believe this is simply an\n\u003e edge case that implementations need to correctly handle, not a protocol\n\u003e issue? Or maybe I'm not understanding the scenario correctly?\n\u003e\nIt is a critical detail to implement correctly. Implementations without\ngood test coverage can easily ship releases that will break / force close\nrandomly during the splice lock process. What's worse is this effects\nthe larger more valuable/busy routing nodes more often and is easy to\nmiss on a simple regtest node 😳.\n\nBy the way, I find your notation a bit hard to follow...I think that we\n\u003e really need to detail the exact message flow (like I did in the linked\n\u003e gist) to be able to explain protocol issues, otherwise there's always\n\u003e a risk that people think about a slightly different message flow, which\n\u003e means we'll just talk past each other...\n\u003e\nThe original gist from back in January is here:\nhttps://gist.github.com/ddustin/7ee222eb31c3eac5b141c991c0937fae\n\nHappy to add more details but the example is accurate to exactly the\nmessages sent during this time. Confirmed it with actual tests as well.\n\nIs there some message that it looks like I missed here? If it helps I can\ngive you an actual log message flow from the real tests, but honestly,\nthey just make it more confusing to understand 🤷.\n\nIt might be simpler to follow by working backwards from this solution:\nhttps://gist.github.com/ddustin/017aeadfbf34d2fcd950f1238614afe2\n\nMy original proposal from late January ^ 100% solves the issue but\nadds a lock on the connection via an extra STFU step.\n\nMy current proposal is essentially \"let's allow the race condition and\nobserve which things become invalid during it.\" I settled on two things\ninvalid events that occur:\n* A stale `commitments_signed` message (bundle) with extra signatures\n* A stale `announcement_sigs` message for the pre-splice channel\n\nAn \"easy\" solution an implementation could do is allow any invalid\n`commitments_signed` or `announcement_sigs` to come through\nand ignore them without warning or error. This \"works\" but leaves the\nnode in a messy state where actual errors are being squelched\nand opens up a potential DoS issue.\n\nSo the ideal solution is to allow a certain class of 'stale' messages\nfor a period of time, that time being from:\nA) mutual_splice_locked (the moment we have both sent \u0026 received\n`splice_locked`)\nB) A successful `commitment_signed` \u003c-\u003e `revoke_and_ack` round trip (in\neither direction)\n\nThe definition of A is pretty straightforward and we need to detect\nmutual_splice_lock anyway so that's easy.\n\nThe definition of B can be a couple of things but the simplest is I believe\nthe sending OR receiving of `revoke_and_ack`.\n\nIs it a little more work to implement it this way? Yeah. But I think the\neffort\nis worth it to make the protocol \u0026 our nodes more robust.\n\nCheers,\nDusty\n-------------- next part --------------\nAn HTML attachment was scrubbed...\nURL: \u003chttp://lists.linuxfoundation.org/pipermail/lightning-dev/attachments/20230406/cee4770b/attachment-0001.html\u003e",
"sig": "4e3e526eaba74889023d488a3e94424681c053b7e9230404497f65b75a439cf4b28abdbbcae778db6adbe2d0446346e32d7d45205a1a5e696fe37b11ee10b32e"
}