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.

Sign up for GitHub

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

Jump to bottom

rpc: checkmessage return an error is the pubkey is not found in the graph #5252

Merged
merged 4 commits into from
Jul 19, 2022
Merged

rpc: checkmessage return an error is the pubkey is not found in the graph #5252

merged 4 commits into from
Jul 19, 2022

Conversation

vincenzopalazzo
Copy link
Collaborator

Returning a warning message when the pub key is not specified and there is no node in the graph.

We try to help people that use core lightning as a signer and nothing else.

Fixes #5247

Changelog-Added: rpc: checkmessage return an warning msg when no node is found in the graph and no pubkey is provided.

Signed-off-by: Vincenzo Palazzo vincenzopalazzodev@gmail.com

@vincenzopalazzo vincenzopalazzo marked this pull request as ready for review May 12, 2022 11:32
@cdecker
Copy link
Member

cdecker commented May 19, 2022

ACK 0c07b7a

@rustyrussell
Copy link
Contributor

Ok, this is obviously confusing for people.

How about we just return an error in this case? Because we've failed to verify, we're probably best off failing: we know nothing useful about the signature on that case.

@vincenzopalazzo
Copy link
Collaborator Author

How about we just return an error in this case? Because we've failed to verify, we're probably best off failing: we know nothing useful about the signature on that case.

Yeah I agree on this, the idea that I have not implemented is to make the public key a requirement for the command, but I was not able to find a historical reason to say that my idea was a good idea.

So I make the question here, what is the reason to have the pub key optional?

@rustyrussell
Copy link
Contributor

Yeah, lnd didn't require the pubkey. I felt it was too easy to misuse, but that was their API. That's kind of OK (it means use a pubkey from known nodes), but it should fail more cleanly.

OK, let's change it (with allow-deprecated-apis=false for now).

So, it will always return valid=true (deprecated), otherwise it will actually error. Make sure the error includes the public key they would have needed to use to make the signature valid though: if someone really wants that they can dig it out.

@vincenzopalazzo
Copy link
Collaborator Author

OK, let's change it (with allow-deprecated-apis=false for now).

I agree, I will try to make this change today!

@vincenzopalazzo vincenzopalazzo force-pushed the macros/checkmsg branch 2 times, most recently from 72ec17a to bde4d04 Compare May 23, 2022 18:50
@vincenzopalazzo
Copy link
Collaborator Author

So, it will always return valid=true (deprecated), otherwise it will actually error. Make sure the error includes the public key they would have needed to use to make the signature valid though: if someone really wants that they can dig it out.

Ok, the suggestion is landed in the PR! let wait the CI to make sure I didn't put any regression!

@vincenzopalazzo vincenzopalazzo force-pushed the macros/checkmsg branch 4 times, most recently from dec3302 to 25a2e82 Compare May 24, 2022 08:17
@vincenzopalazzo vincenzopalazzo changed the title rpc: checkmessage return an warning msg rpc: checkmessage return an error is the pubkey is not found in the graph May 24, 2022
rustyrussell
rustyrussell requested changes Jun 17, 2022
View reviewed changes
tests/test_misc.py Outdated Show resolved Hide resolved
rustyrussell
rustyrussell reviewed Jun 24, 2022
View reviewed changes
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.

Since it needs rebasing anyway, some minor nitpicks :)

lightningd/signmessage.c Outdated Show resolved Hide resolved
doc/lightning-checkmessage.7.md Show resolved Hide resolved
@vincenzopalazzo
Copy link
Collaborator Author

@rustyrussell Apologize for the delay here but I missed your review, just jump here today during a tour of my open PR on cln and found that you were waiting for me!

I rebased and resolved the conflicts

@niftynei niftynei added this to the v0.12 milestone Jul 11, 2022
@vincenzopalazzo vincenzopalazzo force-pushed the macros/checkmsg branch 3 times, most recently from 1618819 to 6b8a44d Compare July 13, 2022 20:30
rustyrussell
rustyrussell approved these changes Jul 17, 2022
View reviewed changes
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.

Ack 6b8a44d

@vincenzopalazzo
rpc: checkmessage return an error if pubkey is not found
666ab51 
Returning an warning message when the pub key is not specified and there is no node in the graph.

We try to help people that use core lightning as a signer and nothings else.

Changelog-Deprecated: rpc: checkmessage return an error when the pubkey is not specified and it is unknown in the network graph.
@vincenzopalazzo
rpc: improve error format
dbfeac0 
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@vincenzopalazzo
test: disable schema check of checkmessage with deprecated API
8d695ab 
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@vincenzopalazzo
Copy link
Collaborator Author

Trivial conflict resolution, and rebase on master

@rustyrussell
Copy link
Contributor

Trivial conflict resolution, and rebase on master

Ack 8d695ab

@vincenzopalazzo
rust: upgrade model with new checkmessage requirements
fe8c3de 
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@vincenzopalazzo
Copy link
Collaborator Author

vincenzopalazzo commented Jul 18, 2022 •
edited
Loading

Fixed the rust autogeneration code :) the diff check works!

@rustyrussell
Copy link
Contributor

Ack fe8c3de

valgrind leak report is fixed in master, we can simply apply this once @cdecker acks!

@cdecker cdecker merged commit e70729b into ElementsProject:master Jul 19, 2022
@vincenzopalazzo vincenzopalazzo deleted the macros/checkmsg branch July 19, 2022 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rustyrussell rustyrussell rustyrussell approved these changes

@cdecker cdecker Awaiting requested review from cdecker cdecker is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.12
Development

Successfully merging this pull request may close these issues.

checkmessage support broken in 0.11 - fails with verify: false when the pubkey is omitted
4 participants
@vincenzopalazzo @cdecker @rustyrussell @niftynei