Skip to content

test consensus_encode with &mut#56

Closed
RCasatta wants to merge 2 commits intomasterfrom
testmut
Closed

test consensus_encode with &mut#56
RCasatta wants to merge 2 commits intomasterfrom
testmut

Conversation

@RCasatta
Copy link
Copy Markdown
Owner

@RCasatta RCasatta commented Jun 28, 2022

No description provided.

@RCasatta RCasatta mentioned this pull request Jun 28, 2022
Merged
@dpc
Copy link
Copy Markdown
Contributor

dpc commented Jun 28, 2022

Please check if binaries get any smaller (not by a huge amount, but still) e.g. examples.

@RCasatta
Copy link
Copy Markdown
Owner Author

RCasatta commented Jun 28, 2022 •
edited
Loading

Actually, I checked the main executable and there was no change in size

Fault on my side, sorry. The main executable reduce size of ~100k over ~13M

@dpc
Copy link
Copy Markdown
Contributor

dpc commented Jun 28, 2022

Hmmm... to the byte? We did have a decrease in rust-bitcoin tests, though there is a lot of ser/de code involved.

@dpc
Copy link
Copy Markdown
Contributor

dpc commented Jun 28, 2022 •
edited
Loading

master:

~/l/blocks_iterator (master)> ls -l target/release/blocks_iterator target/release/examples/{heaviest_pipe,less_reward_pipe,most_output_pipe,outputs_versions,verify}
-rwxrwxr-x 2 dpc dpc 13362384 Jun 28 10:30 target/release/blocks_iterator // IGNORE
-rwxrwxr-x 2 dpc dpc  8934680 Jun 28 10:30 target/release/examples/heaviest_pipe
-rwxrwxr-x 2 dpc dpc  8952144 Jun 28 10:30 target/release/examples/less_reward_pipe
-rwxrwxr-x 2 dpc dpc  8929776 Jun 28 10:30 target/release/examples/most_output_pipe
-rwxrwxr-x 2 dpc dpc 13353128 Jun 28 10:30 target/release/examples/outputs_versions
-rwxrwxr-x 2 dpc dpc 13602432 Jun 28 10:30 target/release/examples/verify // IGNORE

with &mut W + one example fix:

~/l/blocks_iterator (pr/56)> ls -l target/release/blocks_iterator target/release/examples/{heaviest_pipe,less_reward_pipe,most_output_pipe,outputs_versions,verify}
-rwxrwxr-x 2 dpc dpc 13362384 Jun 28 10:30 target/release/blocks_iterator // IGNORE
-rwxrwxr-x 2 dpc dpc  4568256 Jun 28 10:32 target/release/examples/heaviest_pipe
-rwxrwxr-x 2 dpc dpc  4581664 Jun 28 10:32 target/release/examples/less_reward_pipe
-rwxrwxr-x 2 dpc dpc  4563336 Jun 28 10:32 target/release/examples/most_output_pipe
-rwxrwxr-x 2 dpc dpc  5745968 Jun 28 10:32 target/release/examples/outputs_versions
-rwxrwxr-x 2 dpc dpc 13602432 Jun 28 10:30 target/release/examples/verify // IGNORE

master + dpc/rust-bitcoin@cf5503a (git version of rust-bitcoin without my &mut W PR):

~/l/blocks_iterator ((27d3cd2d…))> ls -l target/release/blocks_iterator target/release/examples/{heaviest_pipe,less_reward_pipe,most_output_pipe,outputs_versions,verify}
-rwxrwxr-x 2 dpc dpc 13362384 Jun 28 10:30 target/release/blocks_iterator // IGNORE
-rwxrwxr-x 2 dpc dpc  8922328 Jun 28 10:38 target/release/examples/heaviest_pipe
-rwxrwxr-x 2 dpc dpc  8940128 Jun 28 10:38 target/release/examples/less_reward_pipe
-rwxrwxr-x 2 dpc dpc  8921800 Jun 28 10:38 target/release/examples/most_output_pipe
-rwxrwxr-x 2 dpc dpc 13332496 Jun 28 10:38 target/release/examples/outputs_versions
-rwxrwxr-x 2 dpc dpc 13578216 Jun 28 10:38 target/release/examples/verify // IGNORE - I also started messing (investigating why it doesn't get rebuilt) with it here, so somehow it got rebuilt in the meantime

I was confused why blocks_iterator and verify didn't change timestamp, and it turns out in a default build command:

cargo build --release --all --examples

they just don't get built! When I try to compile with:

cargo build --release --all --examples --bin blocks_iterator --features="db,consensus"

I get more compilation errors, so I give up for now (got to go back to work, RN).

Anyway: on the examples I can compile I see almost 50% improvement?! Seems a bit too good to be true so please double check

Fix to this PR I used:

~/l/blocks_iterator (pr/56)> git diff HEAD~1
diff --git a/examples/signatures_in_witness.rs b/examples/signatures_in_witness.rs
index 48b47d1..5d88e86 100644
--- a/examples/signatures_in_witness.rs
+++ b/examples/signatures_in_witness.rs
@@ -61,25 +61,25 @@ struct ParsedSignature {
 }
 
 impl Decodable for ParsedSignature {
-    fn consensus_decode<D: std::io::Read>(mut d: D) -> Result<Self, encode::Error> {
+    fn consensus_decode<D: std::io::Read>(mut d: &mut D) -> Result<Self, encode::Error> {
         //TODO fix for schnorr signatures!
-        let first = u8::consensus_decode(&mut d)?;
+        let first = u8::consensus_decode(d)?;
         if first != 0x30 {
             return Err(encode::Error::ParseFailed("Signature must start with 0x30"));
         }
-        let _ = u8::consensus_decode(&mut d)?;
-        let integer_header = u8::consensus_decode(&mut d)?;
+        let _ = u8::consensus_decode(d)?;
+        let integer_header = u8::consensus_decode(d)?;
         if integer_header != 0x02 {
             return Err(encode::Error::ParseFailed("No integer header"));
         }
 
-        let R = <Vec<u8>>::consensus_decode(&mut d)?;
-        let integer_header = u8::consensus_decode(&mut d)?;
+        let R = <Vec<u8>>::consensus_decode(d)?;
+        let integer_header = u8::consensus_decode(d)?;
         if integer_header != 0x02 {
             return Err(encode::Error::ParseFailed("No integer header"));
         }
-        let s = <Vec<u8>>::consensus_decode(&mut d)?;
-        let sighash_u8 = u8::consensus_decode(&mut d)?;
+        let s = <Vec<u8>>::consensus_decode(d)?;
+        let sighash_u8 = u8::consensus_decode(d)?;
         let sighash = EcdsaSighashType::from_consensus(sighash_u8 as u32);
 
         Ok(ParsedSignature {

How I tested git version of rust-bitcoin without my &mut W PR:

~/l/blocks_iterator ((27d3cd2d…)) [1]> git diff master
diff --git a/Cargo.toml b/Cargo.toml
index 2258d86..16e4b4f 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -11,7 +11,8 @@ readme = "README.md"
 license = "MIT"
 
 [dependencies]
-bitcoin =  { version = "0.28.0", features = ["use-serde"]}
+# bitcoin =  { version = "0.28.0", features = ["use-serde"]}
+bitcoin =  { git="https://github.com/dpc/rust-bitcoin", rev="cf5503af7462bc142e18480f1ee390a336b68311", features = ["serde"]}
 structopt = "0.3.21"
 log = "0.4.11"
 glob = "0.3.0"

@RCasatta
Copy link
Copy Markdown
Owner Author

RCasatta commented Jun 29, 2022

Anyway: on the examples I can compile I see almost 50% improvement?! Seems a bit too good to be true so please double check

Wow I see the 50% improvement too on examples

@RCasatta RCasatta mentioned this pull request Jun 29, 2022
Closed
@RCasatta RCasatta closed this Aug 31, 2022
@RCasatta RCasatta mentioned this pull request Aug 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@RCasatta @dpc