Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recover mode for recovering using codex32 secret string. #6302

Merged
merged 11 commits into from
Jul 31, 2023

Conversation

adi2011
Copy link
Collaborator

@adi2011 adi2011 commented Jun 3, 2023

This PR would enable users to recover their node using codex32 secret (bip-0093) if their HSM secret file doesn't exist.

bip-0093: https://github.com/bitcoin/bips/blob/master/bip-0093.mediawiki

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to see how codex32 works to review this deep :) but i left some comments

common/codex32.c Outdated Show resolved Hide resolved
@adi2011 adi2011 marked this pull request as ready for review June 14, 2023 04:49
common/codex32.c Outdated Show resolved Hide resolved
common/codex32.c Outdated Show resolved Hide resolved
common/codex32.c Outdated Show resolved Hide resolved
common/codex32.c Outdated Show resolved Hide resolved
common/codex32.c Outdated Show resolved Hide resolved
common/codex32.c Outdated Show resolved Hide resolved
common/codex32.c Outdated Show resolved Hide resolved
lightningd/options.c Outdated Show resolved Hide resolved
lightningd/options.c Outdated Show resolved Hide resolved
lightningd/options.c Outdated Show resolved Hide resolved
@adi2011
Copy link
Collaborator Author

adi2011 commented Jun 20, 2023

Thank you for the review @rustyrussell! I've added the suggested changes.

@adi2011 adi2011 force-pushed the RecoverCodex32 branch 2 times, most recently from cb9eeeb to 9ce36d8 Compare June 20, 2023 09:34
@adi2011 adi2011 force-pushed the RecoverCodex32 branch 4 times, most recently from 202760f to 881a440 Compare July 9, 2023 07:48
@rustyrussell
Copy link
Contributor

Try make check-python-flake8:

tests/test_misc.py:1359:64: E231 missing whitespace after ','
tests/test_misc.py:1361:1: W293 blank line contains whitespace
tests/test_misc.py:1376:37: E231 missing whitespace after ':'
make: *** [Makefile:542: check-python-flake8] Error 1

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept is good, but some code can be neatened up for merge!

Also, you should document the hook in doc/guides/Developer-s Guide/plugin-development/hooks.md, and the recover flag in doc/lightningd-config.5.md.

common/codex32.c Outdated Show resolved Hide resolved
common/codex32.c Outdated Show resolved Hide resolved
common/codex32.c Outdated Show resolved Hide resolved
common/codex32.c Outdated Show resolved Hide resolved
common/codex32.c Show resolved Hide resolved
common/codex32.c Outdated Show resolved Hide resolved
lightningd/options.c Outdated Show resolved Hide resolved
lightningd/options.c Outdated Show resolved Hide resolved
lightningd/lightningd.c Outdated Show resolved Hide resolved
lightningd/lightningd.c Outdated Show resolved Hide resolved
@adi2011 adi2011 force-pushed the RecoverCodex32 branch 2 times, most recently from f2a0350 to 0e4d08f Compare July 17, 2023 05:13
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes only...

common/codex32.c Outdated Show resolved Hide resolved
common/codex32.c Show resolved Hide resolved
common/codex32.c Outdated Show resolved Hide resolved
common/codex32.c Outdated Show resolved Hide resolved
common/codex32.c Outdated
Comment on lines 147 to 148
input_hrp((&codex_checksum_engine)->generator, (&codex_checksum_engine)->residue ,hrp, len);
input_data_str((&codex_checksum_engine)->generator, (&codex_checksum_engine)->residue, codex_datastr, len);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for &(...), just &engine?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here engine is not a pointer. So it throws invalid type argument of ‘->’ (have ‘struct checksum_engine’). If I don't use &(...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, engine.generator then!

common/test/run-codex32.c Outdated Show resolved Hide resolved
lightningd/lightningd.c Outdated Show resolved Hide resolved
doc/lightningd-config.5.md Outdated Show resolved Hide resolved
lightningd/options.c Outdated Show resolved Hide resolved
lightningd/options.c Outdated Show resolved Hide resolved
@adi2011 adi2011 force-pushed the RecoverCodex32 branch 11 times, most recently from 36d4c71 to 7607744 Compare July 27, 2023 19:21
@rustyrussell
Copy link
Contributor

Ok, tomorrow I'm going to clean this up so it bisects and then apply.

@adi2011 had promised to implement the hsmtool code to give you the code for your hsm_secret so this is useful!

adi2011 and others added 10 commits July 31, 2023 09:03
…ppropriate secret through a valid codex32 secret.
Nothing major here:
1. size_t for lengths.
2. pass engine to checksum_verify, as caller wants ->len (avoid repeating 13/15 magic numbers).
3. Use x.member instesad of (&x)->member.
4. Return memcmp result directly instead of if.
5. Spacing removal, `;;` removal.
6. codexl is a bool `true`/`false` not 0/1 (it's the same, but clearer)
7. Make sanity_check assign *fail directly.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Firstly, I wanted the results easier to use:
1. Make them always lower case, even if the string was UPPER.
2. Decode the payload for them.
3. Don't give the user any fields they don't need, and make
   the field sizes explicit.

Secondly, I wanted to avoid the pattern of "check in one place, assume
in another", in favour of "check on use".

So, I changed the code to lower the string if it needs to at the start,
and then changed the pull functions so we always use them to get data:
this way we should fail clearly and gracefully if we don't have enough data.

I made all the checks explicit, where we assign the fields.

I also addressed the FIXME: I think the array is *often* one shorter,
but not always, so I trim the last byte at the end if needed.

[ Aditya modified the tests to work ]

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
Copy link
Contributor

One trivial whitespace fix, merged the test fixup with my cleanup, and removed final commit.

Ack 40701a0

@rustyrussell rustyrussell merged commit 58327a5 into ElementsProject:master Jul 31, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants