fix: re-implement Psbt (de)serialization from/to readers/writers#3255
Conversation
Pull Request Test Coverage Report for Build 10575860768Details
💛 - Coveralls |
|
|
||
| const PSBT_SERPARATOR: u8 = 0xff_u8; | ||
| if bytes.get(MAGIC_BYTES.len()) != Some(&PSBT_SERPARATOR) { | ||
| let separator: u8 = Decodable::consensus_decode(r)?; |
There was a problem hiding this comment.
Might be slightly better to just read 5 bytes at once and compare them in one go.
There was a problem hiding this comment.
Can do that, although I think this version is slightly more readable.
There was a problem hiding this comment.
Given that error[E0277]: the trait bound [u8; 5]: Decodable is not satisfied, I'd have to use read_exact like so:
diff --git a/bitcoin/src/psbt/serialize.rs b/bitcoin/src/psbt/serialize.rs
index 9fa465d1..d201ff45 100644
--- a/bitcoin/src/psbt/serialize.rs
+++ b/bitcoin/src/psbt/serialize.rs
@@ -81,15 +81,16 @@ pub fn deserialize(mut bytes: &[u8]) -> Result<Self, Error> {
/// Deserialize a value from raw binary data read from a `BufRead` object.
pub fn deserialize_from_reader<R: io::BufRead>(r: &mut R) -> Result<Self, Error> {
const MAGIC_BYTES: &[u8] = b"psbt";
+ const PSBT_SERPARATOR: u8 = 0xff_u8;
+
+ let mut magic_and_separator: [u8; 5] = [0; 5];
+ r.read_exact(&mut magic_and_separator)?;
- let magic: [u8; 4] = Decodable::consensus_decode(r)?;
- if magic != MAGIC_BYTES {
+ if &magic_and_separator[..4] != MAGIC_BYTES {
return Err(Error::InvalidMagic);
}
- const PSBT_SERPARATOR: u8 = 0xff_u8;
- let separator: u8 = Decodable::consensus_decode(r)?;
- if separator != PSBT_SERPARATOR {
+ if magic_and_separator[4] != PSBT_SERPARATOR {
return Err(Error::InvalidSeparator);
}
I don't think that's an improvement overall (especially since you already force buffered readers, which should make the benefit of not doing a one-byte read negligible).
There was a problem hiding this comment.
OK, that makes sense. I have a plan to compromise between buffered and unbuffered reading but that's outside of scope of this PR.
|
I think with |
|
Backported in #3266 |
…/writers d5c79fd fix: re-implement (de)serialization from/to readers/writers (elsirion) Pull request description: Backportt #3255 to `bitcoin v0.32`. One change is different from the original patch, import `DisplayHex` and remove imports of `String` and `Vec`. ACKs for top commit: apoelstra: ACK d5c79fd successfully ran local tests Tree-SHA512: 36bb4e8d12bde3f310291b4f066ef0ab17d121fcf1afe4a10ca6f8c29b3bd084d616c212a761e05d6d36255bb84059d4536109a4300208fca66e0752071649a1
Fixes #3250.
The serialization is less than ideal and still allocates a lot. I can understand not wanting to (ab)use the consensus encoding traits, but they have a pretty good interface, copying it and creating some
EncodePsbtandDecodePsbttraits with similar interfaces would have been nice imo.