lightningd: test that hsm_secret is as expected, at startup. #5425
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
cdecker
approved these changes
Jul 14, 2022
lightningd/lightningd.c
Outdated
| * We also check that our node_id is what we expect: otherwise a change | ||
| * in hsm_secret will have strange consequences! */ | ||
| if (!wallet_sanity_check(ld->wallet)) | ||
| errx(1, "Wallet sanity check failed."); |
There was a problem hiding this comment.
Suggested change
| errx(1, "Wallet sanity check failed."); | |
| errx(WALLET_MISMATCH, "Wallet sanity check failed."); |
What do you think to add a more accurate error here? this use case looks like a use case that can happens during some scripting stuff (raspiblitz stuff are usually happy with specific error on the cmd)
But maybe there are other reason to return a generic one?
There was a problem hiding this comment.
Good improvement! I'll add another commit...
There was a problem hiding this comment.
Actually, this is a cleanup which deserves its own (not 0.12) branch....
|
*and Suggested-By: @fiatjaf! |
rustyrussell
force-pushed
the
startup-id-check
branch
from
26542c1
to
9ffe68f
Compare
|
Trivial rebase on master, added Changelog-Changed line. |
rustyrussell
force-pushed
the
startup-id-check
branch
from
9ffe68f
to
b6544a5
Compare
Didn't know/remember that, but I've credited him too! |
If you get the wrong hsm_secret, your node_id will change, and peers won't know who you are, bitcoind will reject your transaction signatures, and other madness. Catch this as soon as it happens, by storing our node_id in the db. Suggested-by: @cdecker, @fiatjaf Signed-off-by: Rusty Russell <[email protected]> Changelog-Changed: Config: `lightningd` will refuse to start with the wrong node_id (i.e. hsm_secret changes).
Most unexpected ones are still 1, but there are a few recognizable error codes worth documenting. Rename the HSM ones to put ERRCODE_ at the front, since we have non-HSM ones too now. Signed-off-by: Rusty Russell <[email protected]>
rustyrussell
force-pushed
the
startup-id-check
branch
from
b6544a5
to
57da410
Compare
|
Fixed hsm generation test, which barfed because we change secret... Ack 57da410 |
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.
If you get the wrong hsm_secret, your node_id will change, and
peers won't know who you are, bitcoind will reject your transaction
signatures, and other madness.
Catch this as soon as it happens, by storing our node_id in the db.
Suggested-by: @cdecker