Implement binary encoding for raw::Key using bitcoin-consensus-encoding crate#57
Implement binary encoding for raw::Key using bitcoin-consensus-encoding crate#57nymius wants to merge 11 commits intorust-bitcoin:masterfrom
raw::Key using bitcoin-consensus-encoding crate#57Conversation
nymius
force-pushed
the
feat/impl-binary-encoding-for-psbt
branch
from
2435cbd to
2f65ba1
Compare
nymius
force-pushed
the
feat/impl-binary-encoding-for-psbt
branch
from
2f65ba1 to
78c5aa2
Compare
We should be able to use |
src/binary_encoding.rs
Outdated
| #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ||
| pub struct Key { | ||
| /// The `keytype` of this PSBT map key (`keytype`). | ||
| pub type_value: u8, |
There was a problem hiding this comment.
On master branch of bitcoin this is a u64 now.
There was a problem hiding this comment.
|
Good work man, the logic looks correct to me. I am a bit of a brain dead reviewer; even knowing the high level design of this code it took me a while to work out what was going on. We can probably iterate on a bunch of the identifiers and also we could help readers with some code comments about how the decoder works in two passes because that is the surprising bit. Props for the idea @mpbagot, its was a decent creative leap. |
I dropped the internals dep in #47, and I think my reasoning still applies. Unless you think this is just a temp dep until we release consensus_encoding? |
|
I don't mind having a dep on |
|
The method I'm using here is already merged, so I can hold this until we have the method in the release, or next release candidate. |
src/binary_encoding.rs
Outdated
| State::KeyType(type_and_data, decoder) => { | ||
| let key_type = decoder.end().map_err(|e| E(Inner::Type(e)))?; | ||
|
|
||
| let is_valid = matches!(key_type, 0x00..=0x18 | 0xFB | 0xFC); |
There was a problem hiding this comment.
BIP 174 states that all unknown keytypes should be ignored. Previously the code threw an error when the keytype wasn't defined in BIP 174 table. To avoid going against the specification this commit removes that check.
3 participants
Description
This is a proof of concept of how to implement
raw::Keybinary encoding usingbitcoin-consensus-encoding.This is what I understood was the idea behind #48, and this is the reason why I opened rust-bitcoin/rust-bitcoin#5611.
I applied @mpbagot suggestion (thanks!). I couldn't figure out another way to do it without the extra allocation.
I think that something like
BytesVecDecoder::without_length_prefix(length: usize)would avoid the allocation and consume directly from the same byte slice. But I don't know if this is undesired from an API perspective.Notes to the reviewers
I've imported
bitcoin-encoding-consensusfrom the git path becauseCompactSizeEncoder::encoded_sizeis not available in the published1.0.0-rc.3release fromcrates.io.