Parse MSG_WTX inventory type (part of ZIP-239)
#2446
Conversation
jvff
force-pushed
the
parse-msg-wtx
branch
2 times, most recently
from
a7194c7 to
8c26e91
Compare
There was a problem hiding this comment.
This looks good and minimal.
I made some comments about test coverage and future bug risks - I'd like to merge after they are resolved.
Can you open a ticket in the mempool epic for the remaining consensus rules?
It was good to leave out codec and network protocol version changes, they're more risky.
Here are the rules that I think aren't covered by this PR, highlighted in bold.
Can you double check against the rules in the ticket and the code in the PR?
A new inv type MSG_WTX ... is added, for use in both inv messages and getdata requests, indicating that the hash being referenced is the wtxid (i.e. the 64-byte value txid || auth_digest). This inv type MUST be used when announcing v5 transactions.
An inv or getdata message MUST NOT use the MSG_WTX inv type for v4 or earlier transactions, or on peer connections that have not negotiated at least the peer protocol version specified in Deployment.
it is possible for a node to receive an inv or getdata message with a MSG_WTX inv type, on a peer connection that has negotiated protocol version 170014 or later, before NU5 has activated. The node MUST handle this case in the same way that it would after NU5 activation when it has no v5 transactions to relay. Receiving a MSG_WTX inv type on a peer connection that has negotiated a protocol version before 170014, on the other hand, SHOULD be treated as a protocol error.
ZIP-239
There was a problem hiding this comment.
Can you open a ticket in the mempool epic for the remaining consensus rules?
Done: #2449. I tried to list all the rules that are missing (or partially missing, as is the case for the first one). I kept them all together, but maybe some of them should be split in separate issues? (E.g., the protocol version handling)
Because the size is not constant anymore, since the `MSG_WTX` inventory type is larger.
A method for a proptest strategy that generates the `InventoryHash` variants that have the smallest serialized size.
In order to properly test the maximum allocation.
Make it easier to navigate from the documentation of the proptest strategies to the variants they generate.
Avoid returning an error if a received `GetData` or `Inv` message contains a `MSG_WTX` inventory type. This prevents Zebra from disconnecting from peers that announce V5 transactions.
The serialized size now depends on what type of `InventoryHash` is being tested.
For now it just copies the stored bytes, in order to allow the tests to run correctly.
Create some mock input bytes representing a serialized `MSG_WTX` inventory item, and check that it can be deserialized successfully.
Create a strategy that only generates `InventoryHash::Wtx` instances, and also update the `Arbitrary` implementation for `InventoryHash` to also generate `Wtx` variants.
Given an arbitrary `InventoryHash`, check that it does not change after being serialized and deserialized. Currently, `InventoryHash::Wtx` can't be serialized, so this test will is expected to panic for now, but it will fail once the serialization code is implemented, and then the `should_panic` should be removed.
Create an random input vector of bytes, and try to deserialize an `InventoryHash` from it. This should either succeed or fail in an expected way.
The attribute is redundant because the `arbitrary` module already has that attribute.
A method to return a proptest strategy that creates `Message::Inv` instances.
A method that returns a proptest strategy that creates `Message::GetData` instances.
Create a `Message` instance, encode it and then decode it using a `Codec` instance and check that the result is the same as the initial `Message`. For now, this only tests `Message::Inv` and `Message::GetData`, because these are the variants that are related to the scope of the current set of changes to support parsing the `MSG_WTX` inventory type. Even so, the test relies on being able to serialize an `InventoryHash::Wtx`, which is currently not implemented. Therefore the test was marked as `should_panic` until the serialization code is implemented.
I split off ticket #2451. The data structures and sending messages are essential. |
Motivation
ZIP-239 describes a new inv type (
MSG_WTX), to be applied from the NU5 network upgrade onward. This message type is to be used for bothinvmessages andgetdatarequests for relaying mempool transactions.Even though a mempool implementation is not the current focus, Zebra could close connections to peers that send these messages. This could lead to Zebra not having enough peers to function properly after the NU5 activation.
Specifications
https://zips.z.cash/protocol/protocol.pdf#transactions
https://zips.z.cash/protocol/protocol.pdf#txnidentifiers
ZIP-239
Bitcoin equivalent
Solution
This PR adds an
InventoryHash::Wtxvariant. This variant is currently assumed to not be used and simply stores the raw bytes of the data it contains. This is currently not an issue because Zebra does not have a mempool implementation.A test vector was added to make sure that the deserialization completes successfully. Some property tests were also added:
InventoryHashvariantsMessage::InvandMessage::GetData(I tried including all variants, but that led to some issues with the strategy used, which required more work, so I decided to limit the scope for now)InventoryHashfrom random bytes (this ended up being less useful than expected, but maybe it can be converted into some sort of fuzz test in the future?)Also included in the PR is an update to two existing preallocation tests, because now the
InventoryHashno longer has a constant size.Review
@teor2345 and @oxarbitrage
Reviewer Checklist
Follow Up Work
Related issues
This PR closes #2233.