src: BTC sign message implemented and tested #53

asi345 · 2023-10-25T14:45:08Z

Addresses the request in the issue : #49

The implementation of BTC message signing has been completed and it also supports antiklepto verification. Furthermore, a test is added as an example in 'examples' folder.

benma

gj!!

please setup your editor to run rustfmt to format the code on save, I see the code is unformatted.

benma · 2023-10-25T14:47:14Z

.github/README.md

+For Intel Macs where the homebrew commands are not installed in /opt/homebrew folder:
+
+    AR=/usr/local/opt/llvm/bin/llvm-ar CC=/usr/local/opt/llvm/bin/clang make wasm
+


please make a separate commit for this as it has nothing to do with the sign message functionality.

maybe drop Intel? or is it really about intel macs?

I have experienced the error in an Intel Mac and I am unsure about other ones. That's why I wanted to be specific. But I will add that to another commit.

yeah but maybe for Intel Macs it could also be in the other location, or the same might be necessary for non-Intel Macs :P unless we know for sure it's best to be generic

benma · 2023-10-25T14:48:29Z

examples/btc_sign_msg.rs

+    let _addr = paired_bitbox.btc_address(
+        pb::BtcCoin::Btc,
+        &keypath,
+        &script_config_addr,
+        true,
+    ).await
+    .unwrap();


delete this? it has nothing to do with sign_msg.

It was implemented because a similar call is also made in the python version of the code, but I can delete as it is not that relevant.

Ah now I see, but there it is called with display=False, so the address can be printed to stdout before getting the message from stdin. I'd say all of that is not needed here.

benma · 2023-10-25T14:50:10Z

src/btc.rs

+            }),
+        };
+
+        let response: pb::btc_response::Response = self.query_proto_btc(pb::btc_request::Request::SignMessage(request)).await?;


can drop the : pb::btc_response::Response as it is inferred. if you want to keep it, drop pb:: as you already imported btc_response at the top.

benma · 2023-10-25T14:51:35Z

src/btc.rs

+            _ => return Err(Error::UnexpectedResponse),
+        };
+
+        let request: pb::AntiKleptoSignatureRequest = pb::AntiKleptoSignatureRequest {


same, no need to declare the type : pb::AntiKleptoSignatureRequest here.

i would maybe even inline this struct into the request below.

benma · 2023-10-25T14:51:56Z

src/btc.rs

+            host_nonce: host_nonce.to_vec(),
+        };
+
+        let response: btc_response::Response = self.query_proto_btc(pb::btc_request::Request::AntikleptoSignature(request)).await?;


drop type declaration here too and everywhere below

Okay, removing all type declarations. I was using them for my own comfort.

I see. I'd say it is not idiomatic. Type declarations are usually only put if the compiler can't infer it or if the type is something complicated or involve generics, but in these cases the types are trivial.

benma · 2023-10-25T14:56:29Z

src/btc.rs

+
+        let mut sig: Vec<u8> = signature[..64].to_vec();
+        let recid: u8 = signature[64].clone();
+        let compressed: u8 = 4;


keep the comment from the bitbox02.py impl:

"// See https://github.com/spesmilo/electrum/blob/84dc181b6e7bb20e88ef6b98fb8925c5f645a765/electrum/ecc.py#L521-L523"

Of course, my bad. Adding right away.

benma · 2023-10-25T14:58:34Z

src/btc.rs

+        let compressed: u8 = 4;
+        let electrum_sig65: u8 = 27 + compressed + recid;
+        sig.splice(0..0, electrum_sig65.to_be_bytes());
+        Ok(sig)


in the Go library and in the Python library we return a tuple (signature, recID, electrumSig65).

Please do the same here. Maybe best to add a new struct with these three fields and return that.

Where should I define the struct exactly? At the beginning of the same file I assume.

benma · 2023-10-25T14:59:31Z

src/btc.rs

+        crate::antiklepto::verify_ecdsa(&host_nonce, &signer_commitment, &signature)?;
+
+        let mut sig: Vec<u8> = signature[..64].to_vec();
+        let recid: u8 = signature[64].clone();


no need to call .clone()

btw: you can run clippy, the linter, like cargo clippy --features="usb" and see it complain about this ;) the CI runs clippy as well.

You're right. The command also complained about this :) I think I did instinctly.

benma · 2023-10-25T15:12:57Z

src: BTC sign message implemented and tested

may I propose a better title? src as context does not say much, almost any change will be in src.

btc: add btc_sign_message() and example

it is shorter, and tested does not mean much if there is no unit test. also generally in commit titles it is better to use the imperative form (e.g. add instead of added) for brevity.

asi345 · 2023-10-26T07:43:29Z

I implemented the requested changes and now pushing a new commit according to them

benma

almost there! thanks!

benma · 2023-10-26T07:48:52Z

.github/README.md

@@ -41,7 +41,7 @@ Therefore a new clang compiler and archiver needs to be installed via:
 In order to use that new clang compiler and archiver specify it when runing `make wasm`:

    AR=/opt/homebrew/opt/llvm/bin/llvm-ar CC=/opt/homebrew/opt/llvm/bin/clang make wasm
-
+    


nit: accidental change. pls also set up your editor to delete trailing whitespace :)

You're right, adjusting it

benma · 2023-10-26T07:52:04Z

src/btc.rs

@@ -198,6 +198,13 @@ pub struct Transaction {
    pub locktime: u32,
 }

+#[derive(Debug, PartialEq)]
+pub struct Signature {


please rename - Signature is too generic and it is not used by the other sign functions. SignMessageResult or SignMessageSignature is better.

I also thought of that, but used Signature since it was not used earlier. However, for future implementation, your suggestion is better

benma · 2023-10-26T07:52:31Z

src/btc.rs

+pub struct Signature {
+    pub sig: Vec<u8>,
+    pub recid: u8,
+    pub electrum_sig65: Vec<u8>,


moving the docstring comment here could be helpful to users of this library:

See https://github.com/spesmilo/electrum/blob/84dc181b6e7bb20e88ef6b98fb8925c5f645a765/electrum/ecc.py#L521-L523

benma · 2023-10-26T08:00:53Z

src/btc.rs

+        let mut electrum_sig65 = signature.clone();
+        electrum_sig65.splice(0..0, sig65.to_be_bytes());


it is slightly awkward to serialize a one byte integer as big endian - it is just one byte ;) you can just do [sig65].

Both lines could also be replaced with this, which is probably a bit more readable:

let mut electrum_sig65 = vec![sig65]; electrum_sig65.extend_from_slice(&sig)

Due to my inexperience in Rust, I just did it the first way I discovered :). Updating them according to your suggestion.

benma · 2023-10-26T08:02:27Z

examples/btc_sign_msg.rs

+        .btc_sign_message(pb::BtcCoin::Btc, script_config_sign_msg, b"message")
+        .await
+        .unwrap();
+}


Maybe print the result: println!("Signature: {:?}", signature) where signature is the full result.

Yes, I was planning to also ask this while implementing, but apparently I forgot. Thanks for reminding :)

The implementation of BTC message signing has been completed and it also supports antiklepto verification. Furthermore, an example in 'examples' folder. Signed-off-by: asi345 <[email protected]>

asi345 · 2023-10-26T08:28:03Z

Okay, I updated the code, adjusting according to your review

benma

tACK

benma requested changes Oct 25, 2023

View reviewed changes

asi345 force-pushed the btc_sign_message branch from f52e41e to e99e24e Compare October 25, 2023 15:05

asi345 force-pushed the btc_sign_message branch from e99e24e to be840cd Compare October 26, 2023 07:43

benma requested changes Oct 26, 2023

View reviewed changes

btc: add btc_sign_message() and example

2886a7d

The implementation of BTC message signing has been completed and it also supports antiklepto verification. Furthermore, an example in 'examples' folder. Signed-off-by: asi345 <[email protected]>

asi345 force-pushed the btc_sign_message branch from be840cd to 2886a7d Compare October 26, 2023 08:26

benma approved these changes Oct 26, 2023

View reviewed changes

asi345 merged commit e4ddcbf into BitBoxSwiss:master Oct 26, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: BTC sign message implemented and tested #53

src: BTC sign message implemented and tested #53

asi345 commented Oct 25, 2023

benma left a comment •

edited

Loading

benma Oct 25, 2023

asi345 Oct 25, 2023

benma Oct 25, 2023

benma Oct 25, 2023

asi345 Oct 25, 2023

benma Oct 25, 2023

benma Oct 25, 2023

benma Oct 25, 2023

benma Oct 25, 2023

asi345 Oct 25, 2023

benma Oct 25, 2023

benma Oct 25, 2023

asi345 Oct 25, 2023

benma Oct 25, 2023 •

edited

Loading

asi345 Oct 25, 2023

benma Oct 25, 2023 •

edited

Loading

asi345 Oct 25, 2023

benma commented Oct 25, 2023

asi345 commented Oct 26, 2023

benma left a comment

benma Oct 26, 2023

asi345 Oct 26, 2023

benma Oct 26, 2023

asi345 Oct 26, 2023

benma Oct 26, 2023

benma Oct 26, 2023

asi345 Oct 26, 2023

benma Oct 26, 2023

asi345 Oct 26, 2023

asi345 commented Oct 26, 2023

benma left a comment

		For Intel Macs where the homebrew commands are not installed in /opt/homebrew folder:

		AR=/usr/local/opt/llvm/bin/llvm-ar CC=/usr/local/opt/llvm/bin/clang make wasm

		@@ -41,7 +41,7 @@ Therefore a new clang compiler and archiver needs to be installed via:
		In order to use that new clang compiler and archiver specify it when runing `make wasm`:

		AR=/opt/homebrew/opt/llvm/bin/llvm-ar CC=/opt/homebrew/opt/llvm/bin/clang make wasm

		let mut electrum_sig65 = signature.clone();
		electrum_sig65.splice(0..0, sig65.to_be_bytes());

src: BTC sign message implemented and tested #53

src: BTC sign message implemented and tested #53

Conversation

asi345 commented Oct 25, 2023

benma left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma commented Oct 25, 2023

asi345 commented Oct 26, 2023

benma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asi345 commented Oct 26, 2023

benma left a comment

Choose a reason for hiding this comment

benma left a comment •

edited

Loading

benma Oct 25, 2023 •

edited

Loading

benma Oct 25, 2023 •

edited

Loading