getblocks, headers, getheaders, tx, getdata, notfound
#44
Conversation
|
|
|
@dconnolly I think that the crate selection issues should be resolved with changes included in #17 |
zebra-network/src/protocol/codec.rs
Outdated
| } | ||
|
|
||
| fn read_inv<R: Read>(&self, reader: R) -> Result<Message, Error> { | ||
| let hashes = self._read_generic_inventory_hash_vector(reader)?; |
There was a problem hiding this comment.
Rather than just having a generic inv vector reader function, since we are duplicating the vector reading code with other messages too, maybe it would be cleaner to add a
fn read_list<T: ZcashDeserialize>(&mut self, max_preallocation: usize) -> Result<Vec<T>, SerializationError>
to the ReadZcashExt trait that takes a bound on the number of items to preallocate, and use it in all of the message deserialization implementations?
We could also add a corresponding writer function but I'm not sure that it's as necessary because the encoding doesn't require validation logic.
There was a problem hiding this comment.
Hmmm ok, I'll pursue this.
There was a problem hiding this comment.
I'll also need the bespoke encoded_<T>_size too.
There was a problem hiding this comment.
Hmm, if max_preallocation is the number of items rather than the size of the allocation in bytes or something, then I think that the ENCODED_<T>_SIZE constants can live at each callsite, and they can compute self.builder.max_message_size / ENCODED_<T>_SIZE or something to get the number of items, right?
There was a problem hiding this comment.
In the future I think it would be good to have size hint methods included in the ZcashSerialize trait but I'm not sure we need them immediately
dconnolly
force-pushed
the
get-blocks
branch
2 times, most recently
from
9e59b04 to
c974e74
Compare
dconnolly
changed the title
getblocks, headers, getheaders, txgetblocks, headers, getheaders, tx, 'getdata', 'notfound'
dconnolly
changed the title
getblocks, headers, getheaders, tx, 'getdata', 'notfound'getblocks, headers, getheaders, tx, getdata, notfound
…st for getheaders/headers
|
Does this add corresponding serialization implementations for these messages? I don't see them in |
Bah you are right, I knew I was missing something last night but couldn't remember. Will add before merge. |
…lative based on Bitcoin docs
|
After this is merged, we have some read/write stuff to do for |
| @@ -138,27 +138,100 @@ pub enum Message { | |||
| /// | |||
| /// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#block) | |||
| Block { | |||
| /// Transaction data format version (note, this is signed). | |||
| version: Version, | |||
There was a problem hiding this comment.
later: we may want to use an internal enum rather than carrying a version field around?
There was a problem hiding this comment.
Yeah this wasn't here for a long time because I think we had discussed handling Versions better.
| // many results. | ||
| GetBlocks { | ||
| /// The protocol version. | ||
| version: Version, |
There was a problem hiding this comment.
later: how does this version field interact with the versions of the blocks themselves?
There was a problem hiding this comment.
also, like, is this something we need to specify in the message itself, or can the codec fill it in? if the codec can't fill it in (<=> the version in the message is independent of the transport version), then how do version mismatches happen?
There was a problem hiding this comment.
(to be clear this could all be a new issue or go somewhere else, it's 'later' because it should be non-blocking for this PR)
No description provided.