search

rust-bitcoin / rust-bitcoincore-rpc

Rust RPC client library for the Bitcoin Core JSON-RPC API.
313 stars 231 forks source link

Add verifymessage feature #343

Closed tcharding closed 1 month ago

tcharding commented 2 months ago

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.

apoelstra commented 2 months ago

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

tcharding commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

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

I'll do that, thanks.

tcharding commented 1 month ago

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 commented 1 month ago

Close this if/when #349 goes in.