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

Add verifymessage feature #343

Closed

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented May 1, 2024

  • Patch 1: Adds new feature "verifymessage" fixing by broken master branch.
  • Patch 2: Fixes pinning allowing this PR to get past CI.

In #326 we changed a function to use bitcoin::sign_message::MessageSignature but doing so requires the "base64" feature to be on for bitcoin. This did not get caught by CI but improvements to CI in #338 will now catch this.

Add a feature to json and client that allows enabling "base64" if the verifymessage RPC call is required.

@tcharding tcharding force-pushed the 05-01-verifymessage-feature branch from 806b654 to af014db Compare May 1, 2024 02:34
@tcharding tcharding closed this May 1, 2024
@tcharding tcharding reopened this May 1, 2024
@tcharding tcharding force-pushed the 05-01-verifymessage-feature branch from af014db to 40d91e1 Compare May 1, 2024 02:57
@tcharding tcharding marked this pull request as draft May 1, 2024 02:57
@tcharding tcharding added this to the v0.19.0 milestone May 1, 2024
@tcharding tcharding marked this pull request as ready for review May 1, 2024 21:11
@apoelstra
Copy link
Member

Can you switch the commits? Currently the first one doesn't compile.

tcharding added 2 commits May 3, 2024 07:43
In rust-bitcoin#326 we changed a function to use
`bitcoin::sign_message::MessageSignature` but doing so requires the
"base64" feature to be on for `bitcoin`. This did not get caught by CI
but improvements to CI in rust-bitcoin#338 will now catch this.

Add a feature to `json` and `client` that allows enabling "base64" if
the `verifymessage` RPC call is required.
MSRV build just broke because of a bunch of dependencies. I did not
investigate why I just found a set of versions that builds.
@tcharding tcharding force-pushed the 05-01-verifymessage-feature branch from 40d91e1 to 56d62dc Compare May 2, 2024 21:43
@tcharding
Copy link
Member Author

Can you switch the commits? Currently the first one doesn't compile.

Oh you build each commit before merging, nice. I have not been doing that when merging things. Lesson learned.

I added feature gating to the import as well which got lost somehow. I thought about it this morning, glad I got to add it.

@apoelstra
Copy link
Member

So, I'm unsure about this verifymessage feature. At least, it shouldn't be called verifymessage.

PSBTs are also base64-encoded and this is handled in the PSBT API by just taking PSBTs as &strs. I think we should either do that here, or change the PSBT API to also be feature-gated on base64, or just change the bitcoin dep to always have the base64 feature on.

@tcharding
Copy link
Member Author

or just change the bitcoin dep to always have the base64 feature on.

I'll do that, thanks.

@tcharding
Copy link
Member Author

Done as a separate PR because it is totally different to this so using this PR would make the discussion confusing. That is a new process idea I just had, please comment if its worse.

Thanks for the review too @apoelstra, I know you are travelling and this is not the most exciting thing to be fixing while doing so. Appreciate the input.

@tcharding tcharding marked this pull request as draft May 4, 2024 21:15
@tcharding
Copy link
Member Author

Close this if/when #349 goes in.

@tcharding tcharding closed this May 5, 2024
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.

2 participants