Improvements to Witness type#4
Closed
dr-orlovsky wants to merge 8 commits intoRCasatta:witness_with_blockfrom
BP-WG:scripts/witness
Closed
Improvements to Witness type#4dr-orlovsky wants to merge 8 commits intoRCasatta:witness_with_blockfrom BP-WG:scripts/witness
dr-orlovsky wants to merge 8 commits intoRCasatta:witness_with_blockfrom
BP-WG:scripts/witness
Conversation
Witness struct is in place of the Vec<Vec<u8>> we have before this commit. Implementation of Default, Iterator and others allows to have similar behaviour but using a single Vec prevent many allocations during deserialization which in turns results in better performance, even 20% better perfomance on recent block.
RCasatta
reviewed
Nov 25, 2021
| } | ||
|
|
||
| /// Gets nth element from the witness stack. Does not allocate | ||
| pub fn get(&self, index: usize) -> Option<&[u8]> { |
Owner
There was a problem hiding this comment.
If we really need Index (I want to see what you and other think of the comment rust-bitcoin#672 (comment)) I think we can just use something like self.iter().nth(index) (also the iterator is not allocating)
Author
There was a problem hiding this comment.
Why not to keep the whole map of indexes as a part of the struct itself? This will make indexing performance much better.
Indexes are needed for circumstances like "get the previous to last item in witness stack" useful in Taproot witness parsing (for instance "if there is more than two stack elems after annex removal, get the second to the last as a script")
|
|
||
| impl ToHex for Witness { | ||
| fn to_hex(&self) -> String { | ||
| let mut vec = Vec::with_capacity(self.content.len()); |
Owner
There was a problem hiding this comment.
self.content.len() should be self.serialized_len()
be017cf to
5df226f
Compare
5df226f to
4454699
Compare
ba224cf to
106acdc
Compare
RCasatta
pushed a commit
that referenced
this pull request
Feb 21, 2025
f3c80ea Use concrete type for all_zeros call (Tobin C. Harding) Pull request description: Currently we use the `Hash` trait in a bunch of places to call `all_zeros`. We are attempting to improve the `hashes` API and this usage is both unnecessary and also hindering that effort. Use the concrete type (e.g. `BlockHash`) instead of calling through the trait method. Refactor only, no logic changes. ACKs for top commit: apoelstra: ACK f3c80ea I contend that this meets one-ACK carve-out #4 "code moves that do not change the API" Tree-SHA512: a8d7ba48cf6816b722d626ed0a9a7ccfeee2dff19ef689c78661e9afff1f9053a53752562c70c201e33f8e979a2ea9d14660b36d3f732c0f37c327a062514919
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.