hsmd: split revocation from validation #7010

devrandom · 2024-01-22T10:26:28Z

In https://gitlab.com/lightning-signer/validating-lightning-signer/-/issues/207 we discovered that it's possible for CLN to validate a holder commitment N, but then forget that it did so and try to use revoked commitment N-1. A validating signer cannot allow that, since it can result in funds loss in general.

This splits the two calls so that the action can be persisted after the validation and before the revocation.

channeld/channeld.c

vincenzopalazzo · 2024-01-22T16:49:08Z

channeld/channeld.c

+	/* Now that the master has persisted the new commitment advance the HSMD
+	 * and fetch the revocation secret for the old one. */
+	struct secret old_secret2;
+	struct pubkey next_point2;


this is not important for functionality, but this would be good to declare all of the top functions with all the parameters declaration

vincenzopalazzo

ACK 846c3e0

Waiting just for the CI but LGMT

ksedgwic · 2024-01-22T20:01:36Z

I think the comparisons should stay

They are important for testing that version 5 signers work with version 4 CLN.

ksedgwic · 2024-01-22T20:07:11Z

This is needed if you want to test compatibility. You can set HSM_MAX_VERSION to 4 and the code will behave the old way even if a signer is capable.

What would be really good is if we could set it differently w/o rebuilding for regression test passes ...

devrandom · 2024-01-22T20:55:05Z

if we want to test version 4 CLN, we should test with that version of the code, because we might have changed something that is not completely restored to the previous behavior via the #ifdef flag.

ksedgwic · 2024-01-22T21:46:30Z

The (redundant normally) comparisons allow a single line mod to force CLN to v4 despite any capabilities flag. This is useful tool for debugging in the future.

I do "side-by-side" comparisons (before the feature, after the feature) a lot when debugging. I think it is really nice to be able to force downgrade the protocol simply, w/o changing everything else in the experiment (by using an old version of something)

cdecker · 2024-01-23T10:17:55Z

Added to milestone, if this is ready I can take over and shepherd it through CI :-)

rustyrussell

Nice API improvement! Just minor cleanup advice, no significant flaws. Thanks!

rustyrussell · 2024-01-23T10:23:04Z

common/hsm_capable.c

+
+/* Is this capability supported by the HSM? (So far, always a message
+ * number) */
+bool hsm_is_capable(u32 *capabilities, u32 msgtype)


Nitpick: const u32 *capabilities.

rustyrussell · 2024-01-23T10:24:27Z

lightningd/channel_control.c

@@ -1631,6 +1631,9 @@ bool peer_start_channeld(struct channel *channel,
 	initmsg = towire_channeld_init(tmpctx,
 				       chainparams,
 				       ld->our_features,
+				       /* Capabilities arg needs to be a tal array */
+				       tal_dup_arr(tmpctx, u32, ld->hsm_capabilities,


Weird... since you use tal_count then ld->hsm_capabilities must be a tal array, so you shouldn't need to copy it?

rustyrussell · 2024-01-23T10:25:32Z

channeld/channeld.c

@@ -1923,6 +1925,7 @@ static void send_revocation(struct peer *peer,
 	const struct failed_htlc **failed;
 	struct added_htlc *added;
 	const u8 *msg;
+	const u8 *msg2;


We can just reuse msg here, and don't need a new var I think.

rustyrussell · 2024-01-23T10:26:43Z

channeld/channeld.c

+		memcpy(&old_secret2, old_secret, sizeof(old_secret2));
+		memcpy(&next_point2, next_point, sizeof(next_point2));


I always prefer assignment to memcpy, since it's typesafe (and compiler will do the same thing). old_secret2 = *old_secret; and next_point2 = *next_point.

cdecker · 2024-01-26T16:50:16Z

Thanks for the feedback Rusty, I think as soon as it's been amended we can merge this PR, and close this racy behavior :-)

Thanks @ksedgwic and @devrandom for finding and fixing it 👍

Changelog-Added: Added hsm_capabilities and hsm_is_capable to channeld.

ChangeLog-Added: Added hsmd_revoke_commitment_tx to ensure synchronization of local state with remote signers.

devrandom · 2024-01-27T10:23:42Z

ready

Changelog-None: fixes bug in ([ElementsProject#7010]) After HSM_VERSION 5 we explicitly revoke the commitment in case the original revoke didn't complete. The hsmd_revoke_commitment_tx call is idempotent.

Changelog-None: fixes hole in ([ElementsProject#7010]), see ([ElementsProject#7026]) Explicit hsmd_revoke_commitment_tx instead of get_per_commitment_point needed when revokes are retried. The hsmd_revoke_commitment_tx message is idempotent.

Changelog-None: fixes hole in ([#7010]), see ([#7026]) Explicit hsmd_revoke_commitment_tx instead of get_per_commitment_point needed when revokes are retried. The hsmd_revoke_commitment_tx message is idempotent.

Changelog-None: fixes hole in ([ElementsProject#7010]), see ([ElementsProject#7026]) Explicit hsmd_revoke_commitment_tx instead of get_per_commitment_point needed when revokes are retried. The hsmd_revoke_commitment_tx message is idempotent.

devrandom requested a review from cdecker as a code owner January 22, 2024 10:26

devrandom mentioned this pull request Jan 22, 2024

signer: next_commitment_number mismatch causing the signer to exit Blockstream/greenlight#349

Closed

devrandom force-pushed the 202401-split-revoke branch 2 times, most recently from 980ad99 to 0ba9192 Compare January 22, 2024 14:44

devrandom commented Jan 22, 2024

View reviewed changes

channeld/channeld.c Outdated Show resolved Hide resolved

devrandom force-pushed the 202401-split-revoke branch from 0ba9192 to 846c3e0 Compare January 22, 2024 16:39

vincenzopalazzo reviewed Jan 22, 2024

View reviewed changes

vincenzopalazzo approved these changes Jan 22, 2024

View reviewed changes

cdecker added this to the v24.02 milestone Jan 23, 2024

rustyrussell approved these changes Jan 23, 2024

View reviewed changes

cdecker self-assigned this Jan 23, 2024

ksedgwic and others added 4 commits January 27, 2024 11:22

channeld: add hsm_capabilities and add hsm_is_capable to common

86a326b

Changelog-Added: Added hsm_capabilities and hsm_is_capable to channeld.

channeld: defer construction of revocation message until master sync

08770c3

hsmd: separate revoke_commitment_tx

7ea5649

ChangeLog-Added: Added hsmd_revoke_commitment_tx to ensure synchronization of local state with remote signers.

channeld: use revoke_commitment_tx if hsmd supports

d09b3a6

devrandom force-pushed the 202401-split-revoke branch from 14a6870 to d09b3a6 Compare January 27, 2024 10:23

rustyrussell merged commit c5c2e9f into ElementsProject:master Jan 29, 2024
38 checks passed

This was referenced Jan 31, 2024

channeld: use explicit revoke on retry when HSM_VERSION 5 lightning-signer/c-lightning#105

Open

Retries of revoke_and_ack need to use explicit hsmd_revoke_commitment_tx #7026

Closed

ksedgwic mentioned this pull request Jan 31, 2024

channeld: use explicit revoke on retry when HSM_VERSION >= 5 #7027

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hsmd: split revocation from validation #7010

hsmd: split revocation from validation #7010

devrandom commented Jan 22, 2024

vincenzopalazzo Jan 22, 2024

vincenzopalazzo left a comment

ksedgwic commented Jan 22, 2024

ksedgwic commented Jan 22, 2024

devrandom commented Jan 22, 2024

ksedgwic commented Jan 22, 2024

cdecker commented Jan 23, 2024

rustyrussell left a comment

rustyrussell Jan 23, 2024

rustyrussell Jan 23, 2024

rustyrussell Jan 23, 2024

rustyrussell Jan 23, 2024 •

edited

cdecker commented Jan 26, 2024

devrandom commented Jan 27, 2024

		memcpy(&old_secret2, old_secret, sizeof(old_secret2));
		memcpy(&next_point2, next_point, sizeof(next_point2));

hsmd: split revocation from validation #7010

hsmd: split revocation from validation #7010

Conversation

devrandom commented Jan 22, 2024

vincenzopalazzo Jan 22, 2024

Choose a reason for hiding this comment

vincenzopalazzo left a comment

Choose a reason for hiding this comment

ksedgwic commented Jan 22, 2024

ksedgwic commented Jan 22, 2024

devrandom commented Jan 22, 2024

ksedgwic commented Jan 22, 2024

cdecker commented Jan 23, 2024

rustyrussell left a comment

Choose a reason for hiding this comment

rustyrussell Jan 23, 2024

Choose a reason for hiding this comment

rustyrussell Jan 23, 2024

Choose a reason for hiding this comment

rustyrussell Jan 23, 2024

Choose a reason for hiding this comment

rustyrussell Jan 23, 2024 • edited

Choose a reason for hiding this comment

cdecker commented Jan 26, 2024

devrandom commented Jan 27, 2024

rustyrussell Jan 23, 2024 •

edited