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

Optimize Witness serialization #942

Closed
Kixunil opened this issue Apr 5, 2022 · 2 comments · Fixed by #1071 or #1068
Closed

Optimize Witness serialization #942

Kixunil opened this issue Apr 5, 2022 · 2 comments · Fixed by #1071 or #1068
Labels
good first issue P-low Low priority perf Performance optimizations/issues

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented Apr 5, 2022

self.to_vec() allocates, it should be possible to avoid it - just feed the items into serializer.

@Kixunil Kixunil added the perf Performance optimizations/issues label Apr 7, 2022
@tcharding
Copy link
Member

Warning re 'good first issue', might not be totally trivial to sort through the thread of discussion on #899 which is related to this.

@Kixunil
Copy link
Collaborator Author

Kixunil commented May 24, 2022

I don't think it's too important, just looking at the code the optimization should be obvious.

DanGould added a commit to DanGould/rust-bitcoin that referenced this issue Jun 28, 2022
DanGould added a commit to DanGould/rust-bitcoin that referenced this issue Jun 28, 2022
DanGould added a commit to DanGould/rust-bitcoin that referenced this issue Jun 28, 2022
apoelstra added a commit that referenced this issue Jul 18, 2022
a1df62a Witness human-readable serde test (Dr Maxim Orlovsky)
68577df Witness human-readable serde (Dr Maxim Orlovsky)
93b66c5 Witness serde: test binary encoding to be backward-compatible (Dr Maxim Orlovsky)
b409ae7 witness: Refactor import statements (Tobin C. Harding)
e23d3a8 Remove unnecessary whitespace (Tobin C. Harding)
ac55b10 Add whitespace between functions (Tobin C. Harding)

Pull request description:

  This is dr-orlovsky's [PR](#899) picked up at his permission in the discussion thread.

  I went through the review comments and implemented everything except the perf optimisations. Also includes a patch at the front of the PR that adds a unit test that can be run to see the "before and after", not sure if we want it in, perhaps it should be removed before merge.

  This PR implicitly fixes 942.

  To test this PR works as advertised run `cargo test display_transaction --features=serde -- --nocapture` after creating a unit test as follows:
  ```rust

      // Used to verify that parts of a transaction pretty print.
      // `cargo test display_transaction --features=serde -- --nocapture`
      #[cfg(feature = "serde")]
      #[test]
      fn serde_display_transaction() {
          let tx_bytes = Vec::from_hex(
              "02000000000101595895ea20179de87052b4046dfe6fd515860505d6511a9004cf12a1f93cac7c01000000\
              00ffffffff01deb807000000000017a9140f3444e271620c736808aa7b33e370bd87cb5a078702483045022\
              100fb60dad8df4af2841adc0346638c16d0b8035f5e3f3753b88db122e70c79f9370220756e6633b17fd271\
              0e626347d28d60b0a2d6cbb41de51740644b9fb3ba7751040121028fa937ca8cba2197a37c007176ed89410\
              55d3bcb8627d085e94553e62f057dcc00000000"
          ).unwrap();
          let tx: Transaction = deserialize(&tx_bytes).unwrap();
          let ser = serde_json::to_string_pretty(&tx).unwrap();
          println!("{}", ser);
      }
  ```

  Fixes: #942

ACKs for top commit:
  apoelstra:
    ACK a1df62a
  Kixunil:
    ACK a1df62a

Tree-SHA512: d0ef5b8cbf1cf8456eaaea490a793f1ac7dfb18067c4019a2c3a1bdd9627a231a4dd0a0151a4df9af2b32b909d4b384a5bec1dd3e38d44dc6a23f9c40aa4f1f9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue P-low Low priority perf Performance optimizations/issues
Projects
None yet
2 participants