Skip to content

Channel: Fix channel state max and add regression test #8420

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ddustin
Copy link
Collaborator

@ddustin ddustin commented Jul 20, 2025

Twice now that I've been aware of the channel state max wasn't updated and it slipped through without being noticed.

This PR adds an explicit test so CI will fail if the max value isn't updated.

@ddustin ddustin requested a review from rustyrussell July 20, 2025 18:17
@ddustin ddustin added this to the v25.09 milestone Jul 20, 2025
@ksedgwic
Copy link
Collaborator

ksedgwic commented Jul 21, 2025

We could add a sentinel as the explicit last state so we wouldn't have to update the CHANNEL_STATE_MAX expr every time ...

	CHANNELD_AWAITING_SPLICE,
	DUALOPEND_OPEN_COMMIT_READY,

	/* Always keep this one last */
	CHANNEL_STATE_LIMIT
};
#define CHANNEL_STATE_MAX  (CHANNEL_STATE_LIMIT - 1)

@Lagrang3
Copy link
Collaborator

Nit:

I tested this.
If I add a new channel state type, eg FUTURE_CHANNEL_STATE, I must also update
many switch statements through the code or the compiler complains about a state not being handled, like for example:

./wallet/wallet.h: In function ‘channel_state_in_db’:
./wallet/wallet.h:232:9: error: enumeration value ‘FUTURE_CHANNEL_STATE’ not handled in switch [-Werror=switch]

Once all of those compiler errors are resolved the test ./common/test/run-channel_type doesn't fail.
One would need the compiler to force the user to put the new channel state value in common/test/run-channel_type.c as well
to make this test fail correctly. By removing the default case.

Comment on lines 124 to 145
bool known_state;
switch(state) {
case DUALOPEND_OPEN_INIT:
case CHANNELD_AWAITING_LOCKIN:
case CHANNELD_NORMAL:
case CHANNELD_SHUTTING_DOWN:
case CLOSINGD_SIGEXCHANGE:
case CLOSINGD_COMPLETE:
case AWAITING_UNILATERAL:
case FUNDING_SPEND_SEEN:
case ONCHAIN:
case CLOSED:
case DUALOPEND_OPEN_COMMITTED:
case DUALOPEND_AWAITING_LOCKIN:
case CHANNELD_AWAITING_SPLICE:
case DUALOPEND_OPEN_COMMIT_READY:
largest_state = state;
known_state = true;
break;
default:
known_state = false;
break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
bool known_state;
switch(state) {
case DUALOPEND_OPEN_INIT:
case CHANNELD_AWAITING_LOCKIN:
case CHANNELD_NORMAL:
case CHANNELD_SHUTTING_DOWN:
case CLOSINGD_SIGEXCHANGE:
case CLOSINGD_COMPLETE:
case AWAITING_UNILATERAL:
case FUNDING_SPEND_SEEN:
case ONCHAIN:
case CLOSED:
case DUALOPEND_OPEN_COMMITTED:
case DUALOPEND_AWAITING_LOCKIN:
case CHANNELD_AWAITING_SPLICE:
case DUALOPEND_OPEN_COMMIT_READY:
largest_state = state;
known_state = true;
break;
default:
known_state = false;
break;
bool known_state = false;
switch(state) {
case DUALOPEND_OPEN_INIT:
case CHANNELD_AWAITING_LOCKIN:
case CHANNELD_NORMAL:
case CHANNELD_SHUTTING_DOWN:
case CLOSINGD_SIGEXCHANGE:
case CLOSINGD_COMPLETE:
case AWAITING_UNILATERAL:
case FUNDING_SPEND_SEEN:
case ONCHAIN:
case CLOSED:
case DUALOPEND_OPEN_COMMITTED:
case DUALOPEND_AWAITING_LOCKIN:
case CHANNELD_AWAITING_SPLICE:
case DUALOPEND_OPEN_COMMIT_READY:
largest_state = state;
known_state = true;
break;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes you are absolutely correct! Nice find.

@ddustin ddustin force-pushed the ddustin/channel_state_max branch from 190411f to 9380369 Compare August 1, 2025 16:42
ddustin added 2 commits August 1, 2025 13:33
Adds a test so CI will fail if CHANNEL_STATE_MAX isn’t updated when new channel states are added.

Changelog-None
@ddustin ddustin force-pushed the ddustin/channel_state_max branch from 9380369 to 9fd9371 Compare August 1, 2025 18:14
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.

3 participants