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

signmessage: improve the UX of the rpc command when zbase is not a valid one #5297

Merged
merged 1 commit into from
Jun 24, 2022
Merged

signmessage: improve the UX of the rpc command when zbase is not a valid one #5297

merged 1 commit into from
Jun 24, 2022

Conversation

vincenzopalazzo
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo commented May 31, 2022 •
edited
Loading

This PR improves the UX of the RPC command when zbase is not a valid one because we have a crash on bad zbase!

Fixes #5293

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

@vincenzopalazzo vincenzopalazzo marked this pull request as ready for review May 31, 2022 21:01
@cdecker
Copy link
Member

cdecker commented Jun 5, 2022

ACK 3fbf38a

Just needs a changelog entry in the commit 👍

@vincenzopalazzo
Copy link
Contributor Author

Just needs a changelog entry in the commit +1

I will never learn this :) added now!

Was thinking that maybe having a unit test can be a good idea to avoid regression with future refactoring.

What do you think? or it is meaningless?

@cdecker
Copy link
Member

cdecker commented Jun 6, 2022

Maybe not a full unit test (startup time vs runtime tradeoff) but if there's a sign message test already that would be a good addition 👍

@vincenzopalazzo
Copy link
Contributor Author

vincenzopalazzo commented Jun 6, 2022 •
edited
Loading

Maybe not a full unit test (startup time vs runtime tradeoff)

Good point! I think next time I will judge also with this in mind. I expanded a signmessage test that was already present

@vincenzopalazzo vincenzopalazzo force-pushed the macros/signmsg_ux branch 2 times, most recently from db36d2c to a84ec74 Compare June 6, 2022 23:54
cdecker
cdecker previously requested changes Jun 8, 2022
View reviewed changes
lightningd/signmessage.c Outdated Show resolved Hide resolved
rustyrussell
rustyrussell requested changes Jun 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.

While it's fine to refer the github bugs like you did, you should also explain the problem, and not assume that GH is accessible in future.

i.e. mention that we crash on bad zbase arg :)

tests/test_misc.py Outdated Show resolved Hide resolved
@vincenzopalazzo
signmessage: improve the UX of the rpc command when zbase is not a va…
fd82346 
…lid one

Changelog-Fixed: signmessage: improve the UX of the rpc command when zbase is not a valid one

Stacktrace generated with a bad `zbase`

```
lightningd: lightningd/signmessage.c:59: from_zbase32: Assertion `len == tal_bytelen(u8arr)' failed
lightningd: FATAL SIGNAL 6 (version v0.11.1)
0x55b9b1b4e617 send_backtrace
[...]
```

Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@vincenzopalazzo
Copy link
Contributor Author

While it's fine to refer the github bugs like you did, you should also explain the problem, and not assume that GH is accessible in future.

Yeah! this was a terrible PR description, take me also some time to remember what was the problem!

Sorry about that!

@rustyrussell
Copy link
Contributor

Ack fd82346

@rustyrussell rustyrussell merged commit d4bc4f6 into ElementsProject:master Jun 24, 2022
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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FATAL on malformed zbase on checkmessage RPC
3 participants
@vincenzopalazzo @cdecker @rustyrussell