Conversation
This allows VecDecoder<Vec<u8>> to work, enabling natural composition of nested vector types like Vec<Vec<u8>> (witness data). The implementation delegates to ByteVecDecoder which provides optimized bulk copying rather than decoding bytes one at a time.
6d5f782 to
4b3e8e5
Compare
Delegate to the consensus_encoding byte vector decoder instead of duplicating the logic over in this crate. Also allows DoS protections to live entirely in consensus_encoding.
DoS protection can now be an implementation detail of the consensus_encoding crate.
4b3e8e5 to
9c0a138
Compare
| /// This delegates to [`ByteVecDecoder`] which provides an optimized implementation | ||
| /// that uses bulk copying rather than decoding bytes one at a time. | ||
| #[cfg(feature = "alloc")] | ||
| impl Decodable for Vec<u8> { |
There was a problem hiding this comment.
This makes the types checkout nicely, but assumes any vec of bytes will have a compact size at the front. I believe this is an ok assumption in the bitcoin domain?
|
This is a really nice PR. Hurts a bit that I did not think of it during development of the witness decoder. Thanks for the work man, big improvement. |
Heh, I felt that with the error composites stuff. |
|
This is a nice refactor |
|
In 86cd93d: Whoah, this doubles the number of allocations required by Concept NACK. |
Dang too good to be true? I was focused on the types, totally overlooked allocations. I think I need some pointers on where the new allocations are though. Does "immediately need to construct a new vector to hold the inner of Witness" refer to the |
Yes. This is the whole reason we had |
Got it, this is making sense as I sink into the |
Oof. This is what I get for merging #4998 with multiple big issues. Apparently I missed a big issue. I guess we could merge this as-is since it's a strict improvement, but we're gonna have to rewrite the witness decoder at some point. I'm a little concerned by this PR tightening the API though by changing the error types. It makes me worried that we're tying our hands in a way that will prevent us from writing a proper decoder. |
I am taking a trip down memory lane here and learning a bunch on how the old decoder was written. It is convincing me to just close this, I don't think it will help in the long run like you are saying. |
|
Yeah, I think you might be right. Sorry for the wasted effort! |
|
I'll attempt to re-write the |
|
\me crawls slowly off into the corner ... |
|
I'm kidding, we got this. |
df64705 primitives: single allocation witness decoder (Nick Johnson) Pull request description: When attempting to simplify the `WitnessDecoder` in #5224, we noticed that we were double allocating which erased the performance gains of the old decoder from #672. This updates `WitnessDecoder` to use a single allocation like the old decoder. While the intention of this patch was purely for performance, three of the new tests fail against the current version of the `WitnessDecoder`: `decode_empty_element`, `decode_multiple_empty_elements`, and `decode_incomplete_witness_count`. For the first two tests, there is a functional change in how empty witness elements are handled. This patch introduces a short-circuit where the old version might get stuck in a loop forever requesting zero bytes. The third test, `decode_incomplete_witness_count`, shows that the impl has a more explicit failure now if `end` is called before the witness element count is complete, instead of just returning and empty witness. ACKs for top commit: tcharding: ACK df64705 jrakibi: ACK df64705 apoelstra: ACK df64705; successfully ran local tests Tree-SHA512: 5177410da2d3ceb65a7bfc3e8444a913d23cad1c8e83f045501882b45bac2a90f403d0f7e72fe6b88c25774bbf636e007726d0dc93aeb77bed4ec94d67efa2bb
Delegate the witness decoder to the consensus_decoding
VecDecoderandByteVecDecoderdrops a lot of duplicated logic. It also allows the DoS protection ofconsensus_encodingto be an implementation detail instead of part of the crate's exposed API.