WIP: HSM_VERSION=6: get_per_commitment_point no longer returns secret #107
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ksedgwic
force-pushed
the
2024-03-get-per-commit-no-old-secret
branch
from
1a39979 to
d6e4832
Compare
Changelog-None: shouldn't affect others HSM_MIN_VERSION is 5 which implies use of WIRE_HSMD_REVOKE_COMMITMENT_TX; prune branches that can't happen.
Changelog-None: internal to channeld Since we don't need a special path for early old_secrets from validate we can factor away duplicate code.
ksedgwic
force-pushed
the
2024-03-get-per-commit-no-old-secret
branch
from
d6e4832 to
31b74c9
Compare
There was a problem hiding this comment.
ACK 31b74c9
Some small nits but are not important to avoid ack this PR
| @@ -436,7 +436,7 @@ static struct io_plan *init_hsm(struct io_conn *conn, | |||
| struct secret *hsm_encryption_key; | |||
| struct bip32_key_version bip32_key_version; | |||
| u32 minversion, maxversion; | |||
| const u32 our_minversion = 4, our_maxversion = 5; | |||
| const u32 our_minversion = 4, our_maxversion = 6; | |||
There was a problem hiding this comment.
thinking, duer that we always upgrade this in sync, our_maxversion can be HSM_MAX_VERSION?
Changelog-None: channeld internal This factoring makes it much clearer which callers need the pubkey and which need the old_secret. It does not alter the unfortunate semantic that fetching future pubkeys beyond the next commit is an error (because the signer will only release secrets for revoked commitments). Using HSM_VERSION 6 will fix this semantic in hsmd.
Changelog-None: hsmd internals
Changelog-Changed: hsmd: HSM_VERSION 6: get_per_commitment_point does not imply index - 2 is revoked, makes it safe to call on any index.
Changelog-Changed: hsmd: the hsmd now supports HSM_VERSION 6 This is actually optional, everything should be ok leaving native hsmd support at HSM_VERSION 5.
ksedgwic
force-pushed
the
2024-03-get-per-commit-no-old-secret
branch
from
31b74c9 to
3005799
Compare
ksedgwic
commented
Mar 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 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.