lightning icon indicating copy to clipboard operation
lightning copied to clipboard

hsmd: make get_per_commitment_point unconditionally safe by not returning secret

Open ksedgwic opened this issue 3 months ago • 1 comments

Removes returned old_secret from get_per_commitment_point making it safe to call on all commits

Motivation: When get_per_commitment_point returns the old secret it implies a revocation of index - 2. This causes a crash when VLS has validated a commitment but not seen the following revoke of the prior and the channel is asynchronously restarted. See https://gitlab.com/lightning-signer/validating-lightning-signer/-/issues/469

The last two commits in this PR are somewhat optional: by increasing the HSM_VERSION to 6 we can inform the signer that it should never return an old_secret (and therefore can't fail). The native hsmd implementation doesn't have the safety issue and it would be ok to leave it at HSM_VERSION 5. But it's cleaner to have consistent semantics.

All but the last two commits in the PR sequence are cleanup and reduction refactoring and worth considering even if an HSM_VERSION change is not considered.

Cleanup and refactoring Includes:

  • prune unreachable pre HSM_VERSION 5 code
  • factor out unneeded make_revocation_msg_from_secret
  • split get_per_commitment_point uses into separate functions (one for point, one for secret)
  • make the negotiated hsmd version available to libhsmd

ksedgwic avatar Mar 26 '24 21:03 ksedgwic

The short-term fix for the VLS crash is merged on our side as in https://gitlab.com/lightning-signer/validating-lightning-signer/-/merge_requests/643

The matching "clean fix" on the VLS side to be matched w/ the last two commits of this PR is https://gitlab.com/lightning-signer/validating-lightning-signer/-/merge_requests/641

@rustyrussell

ksedgwic avatar Mar 27 '24 01:03 ksedgwic

Ack 1b98ea88caf830743b167e32c7270000fd31ad5b

rustyrussell avatar May 13 '24 20:05 rustyrussell