Merge rust-bitcoin#3789: Add hash data to Inventory's Error variant

72e97c6 Add a hash value to Inventory's Error variant (Nick Johnson)

Pull request description:

  I am working on adding BIP324 V2 p2p network message support to the p2p module and ran into a little snag. I can post a draft branch of that work if helpful, but the general strategy is to add a `V2NetworkMessage` which operates in parallel with the existing `RawNetworkMessage` type (which is a bit of misnomer and may be better described in the future as `V1NetworkMessage`, see rust-bitcoin#3157). This allows both p2p message types to hook into the existing `Encodable`/`Decodeable` chain.

  But the `Error` variant of the `Inventory` type message does not have symmetrical encoding and decoding paths. Encoding writes out a zero'd 32 bytes while decoding ignores it completely. And while it is not clear to me if there is [requirement written down](https://en.bitcoin.it/wiki/Protocol_documentation#Inventory_Vectors) anywhere, [bitcoin core does appear to always expect a hash](https://github.com/bitcoin/bitcoin/blob/c1252b14d714448295200a595086dd3e78b46c8f/src/protocol.h#L499) even with the `Error` variant where it is meaningless.

  I believe rust-bitcoin's handling of this was never a problem before because the top level `RawNetworkPackage` pulls all the required bytes off a reader before decoding them. But this is not as easy to do with the v2 p2p network messages since the length is decoded at the transport level, not the message itself. I believe it is preferable for the Decoders to *not* assume that all bytes have been pulled off already given their input stream interface. Maybe somewhat surprisingly, this is the only issue I have run into so far adding the v2 encoding and decoding paths. As it is now, the code panics with an `Unconsumed` because it hasn't pulled the dummy zero bytes off the reader.

  This patch adds the 32 byte array to the Error variant in order to make its Encoding and Decoding paths symmetrical. This also allows a reader to discard the 32 bytes when decoding a message while cleanly keeping things hooked up with the `Decodeable` chain. The hash is still not exposed to the caller.

  This is *a way* to solve my issue, but not sure if it will cause more confusion than its worth. I tried a few other strategies, but preferred this one due to how well it hooks into `Decodeable`.

ACKs for top commit:
  apoelstra:
    ACK 72e97c6; successfully ran local tests
  tcharding:
    ACK 72e97c6

Tree-SHA512: 20cb9fec0768e0fdf7c7f520a00e1a37f5ef0583e2ebc7d47583249c6829c47826d83694a56338efa5844a9fe264a77f6d04c0c46c85f8732c7585515723d7ce

bitcoin/src/p2p/message.rs

@@ -583,7 +583,7 @@ mod test {
             )]),
             NetworkMessage::Inv(vec![Inventory::Block(hash([8u8; 32]).into())]),
             NetworkMessage::GetData(vec![Inventory::Transaction(hash([45u8; 32]).into())]),
-            NetworkMessage::NotFound(vec![Inventory::Error]),
+            NetworkMessage::NotFound(vec![Inventory::Error([0u8; 32])]),
             NetworkMessage::GetBlocks(GetBlocksMessage::new(
                 vec![hash([1u8; 32]).into(), hash([4u8; 32]).into()],
                 hash([5u8; 32]).into(),

bitcoin/src/p2p/message_blockdata.rs

@@ -16,8 +16,9 @@ use crate::transaction::{Txid, Wtxid};
 /// An inventory item.
 #[derive(PartialEq, Eq, Clone, Debug, Copy, Hash, PartialOrd, Ord)]
 pub enum Inventory {
-    /// Error --- these inventories can be ignored
-    Error,
+    /// Error --- these inventories can be ignored.
+    /// While a 32 byte hash is expected over the wire, the value is meaningless.
+    Error([u8; 32]),
     /// Transaction
     Transaction(Txid),
     /// Block
@@ -42,10 +43,10 @@ pub enum Inventory {
 impl Inventory {
     /// Return the item value represented as a SHA256-d hash.
     ///
-    /// Returns [None] only for [Inventory::Error].
+    /// Returns [None] only for [Inventory::Error] who's hash value is meaningless.
     pub fn network_hash(&self) -> Option<[u8; 32]> {
         match self {
-            Inventory::Error => None,
+            Inventory::Error(_) => None,
             Inventory::Transaction(t) => Some(t.to_byte_array()),
             Inventory::Block(b) => Some(b.to_byte_array()),
             Inventory::CompactBlock(b) => Some(b.to_byte_array()),
@@ -66,7 +67,7 @@ impl Encodable for Inventory {
             };
         }
         Ok(match *self {
-            Inventory::Error => encode_inv!(0, [0; 32]),
+            Inventory::Error(_) => encode_inv!(0, [0; 32]),
             Inventory::Transaction(ref t) => encode_inv!(1, t),
             Inventory::Block(ref b) => encode_inv!(2, b),
             Inventory::CompactBlock(ref b) => encode_inv!(4, b),
@@ -83,7 +84,7 @@ impl Decodable for Inventory {
     fn consensus_decode<R: BufRead + ?Sized>(r: &mut R) -> Result<Self, encode::Error> {
         let inv_type: u32 = Decodable::consensus_decode(r)?;
         Ok(match inv_type {
-            0 => Inventory::Error,
+            0 => Inventory::Error(Decodable::consensus_decode(r)?),
             1 => Inventory::Transaction(Decodable::consensus_decode(r)?),
             2 => Inventory::Block(Decodable::consensus_decode(r)?),
             4 => Inventory::CompactBlock(Decodable::consensus_decode(r)?),