CHANNEL_STATE_MAX has wrong value

[ksedgwic]

enum channel_state {
    DUALOPEND_OPEN_INIT = 1,
    CHANNELD_AWAITING_LOCKIN,
    CHANNELD_NORMAL,
        ...
    DUALOPEND_OPEN_COMMITTED,
    DUALOPEND_AWAITING_LOCKIN,
    CHANNELD_AWAITING_SPLICE,
    DUALOPEND_OPEN_COMMIT_READY,
};
#define CHANNEL_STATE_MAX CHANNELD_AWAITING_SPLICE

I think it should be set to DUALOPEND_OPEN_COMMIT_READY instead. Looks like a likely merge conflict.

All uses of CHANNEL_STATE_MAX should be carefully considered as well ...

thumbs up from @niftynei in #core-lightning-dev-chat:
https://discord.com/channels/899980449231814676/899989708065632276/1392940900493295746

[ksedgwic]

Give the memory layout of the billboard I think writing past the permanent array would inadvertently assign to transient instead?:

struct billboard {
	/* Status information to display on listpeers */
	const char *permanent[CHANNEL_STATE_MAX+1];
	const char *transient;
};

[ddustin]

This same issue happened when I added CHANNELD_AWAITING_SPLICE and it was very difficult to notice from the resulting behavior.

It would be nice to get this into a compile or assert or something that would directly fail CI 👀.

[ddustin]

Here is a PR that updates the max state and adds a regression test: #8420

[ksedgwic]

Does ARRAY_SIZE() work? I think it is a compile-time constant ...

bah, nm, it is an enum

[ksedgwic]

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)

[ddustin]

That is similar to my proposal when the problem originally happened (I caused it heh).

The coding style doesn't allow using the last element in an enum being used for the max value. I don't remember exactly why, @rustyrussell would know.

This PR #8420 accomplishes the same goal without breaking the coding conventions of the code base.