Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hsmd: name the hsmd_ready_channel to hsmd_setup_channel #6724

Conversation

vincenzopalazzo
Copy link
Collaborator

Originally VLS used hsmd_ready_channel as an early call during channel setup, but later the BOLT-2 spec changed the name of funding_locked to channel_ready.

This is very confusing because the hsmd_ready_channel is not directly related to the new channel_ready.

This commit is renaming the hsmd_ready_channel to hsmd_setup_channel.

Fixes: #6717
Suggested-by: @ksedgwic

@vincenzopalazzo
Copy link
Collaborator Author

Mh this should also notify the vls team alone, but looks like it is not working, I will check it later when the PR is happy. For now adding @ksedgwic manually

hsmd/libhsmd.c Outdated Show resolved Hide resolved
hsmd/hsmd.c Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

A self review before going to bed

hsmd/libhsmd.c Show resolved Hide resolved
hsmd/libhsmd.c Outdated Show resolved Hide resolved
openingd/dualopend.c Outdated Show resolved Hide resolved
openingd/dualopend.c Show resolved Hide resolved
openingd/openingd.c Outdated Show resolved Hide resolved
openingd/openingd.c Outdated Show resolved Hide resolved
hsmd/hsmd.c Outdated Show resolved Hide resolved
@vincenzopalazzo
Copy link
Collaborator Author

vincenzopalazzo commented Sep 27, 2023

Pushed the Ken review, but now there is the CI that it is catch the hsmd change
and tell us to bump a version.

MISSING-HEADERS common
*** hsmd_wire.csv changed to 60b92a0930b631cc77df564cb9235e6cb220f4337a2bb00e5153145e0bf8c80e without update to common/hsm_version.h
make: *** [hsmd/Makefile:63: check-hsm-versions] Error 1
make: *** Waiting for unfinished jobs....

but as @ksedgwic mentioned this is not strictly required. Moving this conversation on Discord
for the people that are interested


Streaming here the conversation with @cdecker

The key question here is "does the change break a signer that was working before?" and in this case it seems no. The message format did not change, and we don't really care about the signer using another name for the same concept. There is no semantic change, so a new version should not be needed.

In fact, the hsmd has a versioning check in common/hsm_version.h and this contains multiple changes on the version v4. So I just add a new item with the v4 version

@vincenzopalazzo vincenzopalazzo force-pushed the macros/renaming-hsmd-event branch 2 times, most recently from 58bbff2 to 34bdb8f Compare September 29, 2023 08:40
@vincenzopalazzo
Copy link
Collaborator Author

Trivial rebase to fix lnprototest

@vincenzopalazzo vincenzopalazzo force-pushed the macros/renaming-hsmd-event branch 2 times, most recently from 4313961 to 412512f Compare October 2, 2023 20:52
Originally VLS used hsmd_ready_channel as an early call during channel
setup, but later the BOLT-2 spec changed the name of funding_locked to channel_ready.

This is very confusing because the hsmd_ready_channel is not directly
related to the new channel_ready.

This commit is renaming the hsmd_ready_channel to hsmd_setup_channel.

Link: ElementsProject#6717
Suggested-by: Ken Sedgwick
Signed-off-by: Vincenzo Palazzo <[email protected]>
@vincenzopalazzo vincenzopalazzo force-pushed the macros/renaming-hsmd-event branch from 412512f to 7bc7d04 Compare October 3, 2023 15:18
@rustyrussell rustyrussell merged commit f4bf89b into ElementsProject:master Oct 23, 2023
33 of 38 checks passed
@vincenzopalazzo vincenzopalazzo deleted the macros/renaming-hsmd-event branch October 23, 2023 17:08
ksedgwic added a commit to lightning-signer/c-lightning that referenced this pull request Oct 31, 2023
This change was meant to be made in ([ElementsProject#6724]), maybe lost in a rebase.

ChangeLog-None
nepet pushed a commit that referenced this pull request Nov 1, 2023
This change was meant to be made in ([#6724]), maybe lost in a rebase.

ChangeLog-None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hsmd: hsmd_ready_channel is confusing name
3 participants