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

Non-plugin version of hsmtool #3186

Merged
merged 6 commits into from Nov 12, 2019

Conversation

darosior
Copy link
Collaborator

@darosior darosior commented Oct 19, 2019

This closes #3177 as it is a concurrent version of the hsmtools functionalities.

Changelog-None

@darosior darosior requested a review from cdecker as a code owner Oct 19, 2019
tests/test_wallet.py Outdated Show resolved Hide resolved
tests/test_wallet.py Show resolved Hide resolved
tests/test_wallet.py Show resolved Hide resolved
tests/test_wallet.py Outdated Show resolved Hide resolved
tests/test_wallet.py Outdated Show resolved Hide resolved
tests/test_wallet.py Show resolved Hide resolved
tests/test_wallet.py Outdated Show resolved Hide resolved
tools/hsmtools.c Outdated
close(fd);

/* Create a backup file, "just in case". */
rename(hsm_secret_path, "hsm_secret.backup");
Copy link
Collaborator

@niftynei niftynei Oct 21, 2019

Choose a reason for hiding this comment

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

does rename update just the file name, or the entire path?

Copy link
Collaborator Author

@darosior darosior Oct 21, 2019

Choose a reason for hiding this comment

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

entire path

Copy link
Collaborator

@niftynei niftynei Oct 21, 2019

Choose a reason for hiding this comment

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

a bit of a stretch goal, but it'd be nice if it's in the same directory as the original hsm file, so that it's easy to find/recover in case of a problem

Copy link
Collaborator Author

@darosior darosior Oct 21, 2019

Choose a reason for hiding this comment

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

Actually it was in plugin version (we chdir in libplugin.c at init) but I kept it although we don"t run it from the same directory.
In case of a not-already handled problem, the backup is still in the running directory.. And it would be quite hacky, just for a tool : recovering the path to the lightning-dir from the hsm_secret path, then prefixing the rename..

tools/hsmtools.c Outdated Show resolved Hide resolved
@darosior
Copy link
Collaborator Author

@darosior darosior commented Oct 21, 2019

Thanks for the review, I am about to push the polish and the dumpcommitments method. I'll integrate your comments.

@Sjors
Copy link
Contributor

@Sjors Sjors commented Oct 21, 2019

Nice! I prefer Co-Authored-By: Sjors Provoost <sjors@sprovoost.nl> over an @ mention in the commit, because the latter can lead to lots of altcoin spam... (when they get around to cloning this)

Can you add a (optional) node public key argument to dumpcommitments? I don't know if this gets purged from the database 30 (?) days after channel closure, but regardless it's nice to be able to dump a commitment without having access to the full database.

@darosior
Copy link
Collaborator Author

@darosior darosior commented Oct 21, 2019

Nice! I prefer Co-Authored-By: Sjors Provoost sjors@sprovoost.nl over an @ mention in the commit, because the latter can lead to lots of altcoin spam... (when they get around to cloning this)

Sure, will do

(when they get around to cloning this)

😂

@darosior
Copy link
Collaborator Author

@darosior darosior commented Oct 21, 2019

Great, Github even takes it into account.

@darosior darosior force-pushed the hsm_tools_rework branch 3 times, most recently from 8744f16 to 04a42b5 Compare Oct 22, 2019
@darosior
Copy link
Collaborator Author

@darosior darosior commented Oct 22, 2019

Can you add a (optional) node public key argument to dumpcommitments? I don't know if this gets purged from the database 30 (?) days after channel closure, but regardless it's nice to be able to dump a commitment without having access to the full database.

Yes, you right this should be the default I think.

@Sjors
Copy link
Contributor

@Sjors Sjors commented Oct 22, 2019

I'm able to produce the same commitment point with this PR as with my original tool. However it's a bit confusing that index 0 is referred to as depth 1.

@Sjors Sjors mentioned this pull request Oct 22, 2019
@darosior
Copy link
Collaborator Author

@darosior darosior commented Oct 22, 2019

Polished, corrected, and rebased on master.

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Looks good, minor updates only...

tools/hsmtools.c Outdated
if (close(fd) != 0)
return false;

fd = open(".", O_RDONLY);
Copy link
Contributor

@rustyrussell rustyrussell Nov 10, 2019

Choose a reason for hiding this comment

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

This is fsyncing the wrong dir? You want dirname(hsm_secret_path) here.

Copy link
Collaborator Author

@darosior darosior Nov 11, 2019

Choose a reason for hiding this comment

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

Bad copy/paste!

tools/hsmtools.c Outdated
@@ -19,6 +19,7 @@ static void show_usage(void)
printf("./hsmtools <method> [arguments]\n");
printf("methods:\n");
printf(" - decrypthsm <path/to/hsm_secret> <password>\n");
printf(" - encrypthsm <path/to/hsm_secret> <password>\n");
Copy link
Contributor

@rustyrussell rustyrussell Nov 10, 2019

Choose a reason for hiding this comment

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

Since the tool is call htmtools, we could just call these commands "decrypt" and "encrypt" perhaps? And maybe "hsmtool" is better than "hsmtools" though that's marginal.

Copy link
Collaborator Author

@darosior darosior Nov 11, 2019

Choose a reason for hiding this comment

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

Yeah I wanted to add more tools (#3217) but I can make it singular ^^

tools/hsmtools.c Outdated

fd = open(hsm_secret_path, O_RDONLY);
if (fd < 0)
errx(ERROR_HSM_FILE, "Could not open hsm_secret");
Copy link
Contributor

@rustyrussell rustyrussell Nov 10, 2019

Choose a reason for hiding this comment

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

Use err() not errx() here, which will tell them what the error was?

Copy link
Collaborator Author

@darosior darosior Nov 11, 2019

Choose a reason for hiding this comment

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

err.. I confused the err functions

tools/hsmtools.c Outdated
if (fd < 0)
errx(ERROR_HSM_FILE, "Could not open hsm_secret");
if (!read_all(fd, hsm_secret, sizeof(*hsm_secret)))
errx(ERROR_HSM_FILE, "Could not read hsm_secret");
Copy link
Contributor

@rustyrussell rustyrussell Nov 10, 2019

Choose a reason for hiding this comment

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

Here too...

darosior and others added 6 commits Nov 11, 2019
A general one, for all things hsm_secret.
And tell about decryption/encryption with hsmtool
This takes a dbid, a "depth" (how many points to dump), the hsm_secret
path, and a potential password to dump informations about all
commitments until the depth.

Co-Authored-By: Sjors Provoost <sjors@sprovoost.nl>
@darosior
Copy link
Collaborator Author

@darosior darosior commented Nov 11, 2019

Corrected the fsync and the confusion between err functions, renamed encryption methods to encrypt and decrypt, and renamed the tool to hsmtool. Thanks for the review!

@darosior darosior changed the title Non-plugin version of hsmtools Non-plugin version of hsmtool Nov 11, 2019
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Ack aacb721

Nice work!

@rustyrussell rustyrussell merged commit de91eda into ElementsProject:master Nov 12, 2019
4 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

4 participants