Skip to content

Update intro and add new reestablish tests to splicing-test.md#5

Merged
t-bast merged 6 commits intot-bast:splicingfrom
remyers:splicing-add-tests
Jun 2, 2025
Merged

Update intro and add new reestablish tests to splicing-test.md#5
t-bast merged 6 commits intot-bast:splicingfrom
remyers:splicing-add-tests

Conversation

@remyers
Copy link

@remyers remyers commented May 23, 2025

These changes are the result of running interop tests with clightning. This PR suggests running variants of the happy path for splices and adds three new tests that rift on the reestablish tests.

remyers added 2 commits May 23, 2025 14:34
Addition to intro to make sure splice variations are also tested.
Three new tests that rift on the reconnection after sending tx_signatures and update_add_htlc .
@remyers remyers marked this pull request as ready for review May 23, 2025 12:53
@remyers
Copy link
Author

remyers commented May 23, 2025

The new tests may redundantly test existing non-splice reestablish behavior, but it's hard to know what side effects splice implementations may have on existing functionality.

@t-bast t-bast self-assigned this May 26, 2025
Copy link
Owner

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those tests look useful, but can you add a matching PR on eclair that verifies the values used in channel_reestablish in unit tests?

@remyers
Copy link
Author

remyers commented May 27, 2025

Those tests look useful, but can you add a matching PR on eclair that verifies the values used in channel_reestablish in unit tests?

I created Eclair PR #3094 with new/modified tests used in the interop testing scripts.

@remyers remyers force-pushed the splicing-add-tests branch from 1319d59 to 0114973 Compare May 28, 2025 16:28
| | +------------+ +------------+
```

### Disconnection after exchanging `tx_signatures` and both sides send `commit_sig` for channel update; `revoke_and_ack` not received
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this test case should appear before the previous one ("Disconnection after exchanging tx_signatures and both sides send commit_sig for channel update"), since it stops one step earlier in the protocol than the previous one?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 9dc1c78. The original order matched how I implemented them in the interop tests, but you're right swapping the order of he last two would be more logical.

I could also change the names so that the last test is "revoke_and_ack is received" and remove "revoke_and_ack is not received", but it would be kind of a hassle if I have to also update the interop tests to match the new names.

* [Disconnection with concurrent `splice_locked`](#disconnection-with-concurrent-splice_locked)
* [Disconnection after exchanging `tx_signatures` and one side sends `commit_sig` for channel update](#disconnection-after-exchanging-tx_signatures-and-one-side-sends-commit_sig-for-channel-update)
* [Disconnection after exchanging `tx_signatures` and both sides send `commit_sig` for channel update](#disconnection-after-exchanging-tx_signatures-and-both-sides-send-commit_sig-for-channel-update)
* [Disconnection after exchanging `tx_signatures` and both sides send `commit_sig` for channel update; revoke_and_ack not received](#disconnection-after-exchanging-tx_signatures-and-both-sides-send-commit_sig-for-channel-update-revoke_and_ack-not-received)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
* [Disconnection after exchanging `tx_signatures` and both sides send `commit_sig` for channel update; revoke_and_ack not received](#disconnection-after-exchanging-tx_signatures-and-both-sides-send-commit_sig-for-channel-update-revoke_and_ack-not-received)
* [Disconnection after exchanging `tx_signatures` and both sides send `commit_sig` for channel update; `revoke_and_ack` not received](#disconnection-after-exchanging-tx_signatures-and-both-sides-send-commit_sig-for-channel-update-revoke_and_ack-not-received)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 8f7a2d3.

Copy link
Owner

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for adding those!

@t-bast t-bast merged commit 2822a52 into t-bast:splicing Jun 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants