Parse inv, addr, getaddr messages. #31

hdevalence · 2019-09-25T21:57:50Z

(Note: this is supposed to apply on top of #20 or later).

Blocked on #21, in the sense that this PR defines some stub types which are actually part of #21 and should be edited once it lands.

This doesn't include tests but I would rather do the testing via #28 or #27; this could either be merged before or after that.

hdevalence · 2019-09-25T21:58:12Z

013268c might be useful for cherry-picking

hdevalence · 2019-09-25T22:02:41Z

(tried changing base to refresh included commits, then realised it required rebasing).

dconnolly · 2019-09-25T22:16:24Z

zebra-network/src/meta_addr.rs


 /// An address with metadata on its advertised services and last-seen time.
 ///
 /// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#Network_address)
+// XXX determine whether we will use this struct in *our* networking handling


My suspicion is that we will want to move it into protocol but I'd rather wait a bit to see how things settle, WDYT?

Sounds good to me, more metadata thingies related to the protocol.

hdevalence · 2019-09-25T23:24:27Z

Did some cleanup and also closes #25.

hdevalence · 2019-09-25T23:27:54Z

zebra-network/src/protocol/codec.rs

@@ -126,7 +126,7 @@ impl Encoder for Codec {
            GetBlocks { .. } => b"getblocks\0\0\0",
            Headers { .. } => b"headers\0\0\0\0\0",
            GetHeaders { .. } => b"getheaders\0\0",
-            Inventory { .. } => b"inv\0\0\0\0\0\0\0\0\0", // XXX Inventory -> Inv ?
+            Inv { .. } => b"inv\0\0\0\0\0\0\0\0\0",


No strong opinions on this -- I'm happy to drop this commit from the PR but aligning names with bitcoin like this makes the table line up nicer.

hdevalence · 2019-09-25T23:29:22Z

zebra-network/src/protocol/codec.rs

+                    addr.zcash_serialize(&mut writer)?;
+                }
+            }
+            Inv(ref hashes) => {


this needs to be ref hashes so that hashes: &Vec<InventoryHash>, not hashes: Vec<InventoryHash>, which would cause the vector to be moved out of the borrowed Message.

(Perhaps the Version fields should also use ref match patterns (I think they're all Copy, but taking borrows might be slightly more efficient).

hdevalence · 2019-09-25T23:30:34Z

zebra-network/src/protocol/codec.rs

@@ -274,7 +287,8 @@ impl Decoder for Codec {
                }

                // Now that we know we have the full body, split off the body,
-                // and reset the decoder state for the next message.
+                // and reset the decoder state for the next message. Otherwise
+                // we will attempt to read the next header as the current body.


Fixup from #22 (comment)

hdevalence · 2019-09-25T23:31:24Z

zebra-network/src/protocol/inv.rs

+pub struct TxHash(pub [u8; 32]);
+/// Stub-- delete later.
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+pub struct BlockHash(pub [u8; 32]);


These need to be replaced with types from #21 before merging; these are just placeholders.

#21 doesn't introduce a transaction hash type (which is part of #35); I would prefer to merge this changeset with placeholders intact until we have all of the types required in place (since it unblocks some other work).

These types are used for protocol messages, so it makes more sense to keep them scoped with the protocol handling, rather than other networking logic.

These are starting to stack up but I think until generic arrays arrive the cure is worse than the disease :S

This removes the inventory vector structs from `zebra-chain` (as they are really part of the network protocol) and refactors them into a single `InventoryHash` type. This corresponds to Bitcoin's "inventory vector" but with a different, better name (it's not a vector, it's just a typed hash of some other item).

I don't feel super strongly about this change, so I'm happy to drop it, but it makes the parsing match statements line up nicely and aligns naming with the naming used in Bitcoin.

dconnolly

LGTM

dconnolly · 2019-09-27T23:23:34Z

zebra-network/src/protocol/codec.rs

+        // case. Although the size of the recieved data buffer is bounded by the
+        // codec's max_len field, it's still possible for someone to send a
+        // short addr message with a large count field, so if we naively trust
+        // the count field we could be tricked into preallocating a large


dconnolly · 2019-09-27T23:25:36Z

zebra-network/src/protocol/codec.rs

+        // addrs are encoded as: timestamp + services + ipv6 + port
+        const ENCODED_ADDR_SIZE: usize = 4 + 8 + 16 + 2;
+        let max_count = self.builder.max_len / ENCODED_ADDR_SIZE;
+        let mut addrs = Vec::with_capacity(std::cmp::min(count, max_count));


dconnolly · 2019-09-27T23:28:31Z

zebra-network/src/protocol/codec.rs

+        // codec's max_len field, it's still possible for someone to send a
+        // short message with a large count field, so if we naively trust
+        // the count field we could be tricked into preallocating a large
+        // buffer. Instead, calculate the maximum count for a valid message from


dconnolly · 2019-09-27T23:37:27Z

zebra-network/src/protocol/inv.rs

+
+/// An inventory hash which refers to some advertised or requested data.
+///
+/// Bitcoin calls this an "inventory vector" but it is just a typed hash, not a


dconnolly · 2019-09-27T23:40:48Z

zebra-network/src/protocol/inv.rs

+    ReadZcashExt, SerializationError, ZcashDeserialize, ZcashSerialize,
+};
+
+/// Stub-- delete later.


dconnolly · 2019-09-27T23:41:20Z

zebra-network/src/protocol/inv.rs

+
+/// Stub-- delete later.
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+pub struct TxHash(pub [u8; 32]);


So the replacement for this would live in zebra-chain, yes?

Yep, and the BlockHash should turn into a BlockHeaderHash from your PR, I just didn't want to change them until both were available.

dconnolly · 2019-09-28T00:39:01Z

zebra-network/src/protocol/message.rs

-        /// Inventory vectors.
-        inventory: Vec<zebra_chain::types::InventoryVector>,
-    },
+    Inv(Vec<InventoryHash>),


dconnolly · 2019-09-28T00:40:41Z

zebra-network/src/protocol/message.rs

-        /// Inventory vectors.
-        inventory: Vec<zebra_chain::types::InventoryVector>,
-    },
+    GetData(Vec<InventoryHash>),


hdevalence changed the base branch from main to block September 25, 2019 22:00

hdevalence changed the base branch from block to main September 25, 2019 22:00

hdevalence force-pushed the inv branch from 801f9d8 to 9bcd81d Compare September 25, 2019 22:02

dconnolly reviewed Sep 25, 2019

View reviewed changes

hdevalence force-pushed the inv branch 2 times, most recently from 1250799 to e42da36 Compare September 25, 2019 23:23

hdevalence commented Sep 25, 2019

View reviewed changes

hdevalence changed the title ~~Parse inv messages.~~ Parse inv, addr, getaddr messages. Sep 25, 2019

hdevalence force-pushed the inv branch from e42da36 to c7c63c7 Compare September 25, 2019 23:34

hdevalence added 8 commits September 27, 2019 09:11

zebra-network: move types -> protocol::types

cee79cb

These types are used for protocol messages, so it makes more sense to keep them scoped with the protocol handling, rather than other networking logic.

Remove unused import in connect stub.

e6fd044

Add a read_32_bytes helper method.

7de0cd8

These are starting to stack up but I think until generic arrays arrive the cure is worse than the disease :S

Rename Message::Inventory -> Message::Inv

f69d57a

I don't feel super strongly about this change, so I'm happy to drop it, but it makes the parsing match statements line up nicely and aligns naming with the naming used in Bitcoin.

Clarify comment in decoder state.

e5cbd0f

nit: suppress async_await nightly warning.

119f881

Add addr, getaddr serialization.

c2154a5

hdevalence force-pushed the inv branch from c7c63c7 to c2154a5 Compare September 27, 2019 16:26

hdevalence marked this pull request as ready for review September 27, 2019 16:40

dconnolly approved these changes Sep 28, 2019

View reviewed changes

dconnolly merged commit 0a85be2 into main Sep 28, 2019

dconnolly deleted the inv branch September 28, 2019 00:41

This was referenced Sep 30, 2019

Parse addr messages. #25

Closed

Parse inv messages. #24

Closed

teor2345 mentioned this pull request May 8, 2023

build(deps): bump serde from 1.0.160 to 1.0.162 #6621

Merged

skyl added a commit to skyl/zebra that referenced this pull request Sep 25, 2024

fix vanity truncation error (ZcashFoundation#31)

460c206

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse inv, addr, getaddr messages. #31

Parse inv, addr, getaddr messages. #31

hdevalence commented Sep 25, 2019

hdevalence commented Sep 25, 2019 •

edited

Loading

hdevalence commented Sep 25, 2019

dconnolly Sep 25, 2019

hdevalence Sep 25, 2019

dconnolly Sep 26, 2019

hdevalence commented Sep 25, 2019

hdevalence Sep 25, 2019

hdevalence Sep 25, 2019

hdevalence Sep 25, 2019

hdevalence Sep 25, 2019

hdevalence Sep 25, 2019 •

edited

Loading

hdevalence Sep 27, 2019

dconnolly left a comment

dconnolly Sep 27, 2019

dconnolly Sep 27, 2019

dconnolly Sep 27, 2019

dconnolly Sep 27, 2019

dconnolly Sep 27, 2019

dconnolly Sep 27, 2019

hdevalence Sep 28, 2019

dconnolly Sep 28, 2019

dconnolly Sep 28, 2019

Parse inv, addr, getaddr messages. #31

Parse inv, addr, getaddr messages. #31

Conversation

hdevalence commented Sep 25, 2019

hdevalence commented Sep 25, 2019 • edited Loading

hdevalence commented Sep 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdevalence commented Sep 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdevalence Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dconnolly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdevalence commented Sep 25, 2019 •

edited

Loading

hdevalence Sep 25, 2019 •

edited

Loading