Each subsection has to have `serde(default)` to get the behaviour we want
(delete all fields except the ones that have been changed); otherwise, we can
delete only entire sections.
Prior to this change, we required that services that are canceled do not
have a cancel handle in the `cancel_handles` list, based on the
assumption that the handle must have been removed in the process of
canceling this service.
This doesn't holding up though, because it is currently possible for us
to have the same peer connect to us multiple times, the second connect
removes the cancel handle of the original connect and inserts it's own
cancel handle in its place. In this scenario, when the first service is
polled for readiness it will see that it has been canceled and go to
clean itself up, but when it asserts that it doesn't have a cancel
handle it will see the cancel handle of the second connect event, which
uses the same key as the first connect, and fail its debug assertion.
This change removes that debug assert on the assumption that it is okay
for a peer to connect multiple times consecutively, and that the correct
behavior in that case is to just cancel the first connection and
continue as normal.
Prior to this change, the service returned by `zebra_network::init` would spawn background tasks that could silently fail, causing unexpected errors in the zebra_network service.
This change modifies the `PeerSet` that backs `zebra_network::init` to store all of the `JoinHandle`s for each background task it depends on. The `PeerSet` then checks this set of futures to see if any of them have exited with an error or a panic, and if they have it returns the error as part of `poll_ready`.
Co-authored-by: Jane Lusby <jane@zfnd.org>
Prior to this change, the seed subcommand would consistently encounter a panic in one of the background tasks, but would continue running after the panic. This is indicative of two bugs.
First, zebrad was not configured to treat panics as non recoverable and instead defaulted to the tokio defaults, which are to catch panics in tasks and return them via the join handle if available, or to print them if the join handle has been discarded. This is likely a poor fit for zebrad as an application, we do not need to maximize uptime or minimize the extent of an outage should one of our tasks / services start encountering panics. Ignoring a panic increases our risk of observing invalid state, causing all sorts of wild and bad bugs. To deal with this we've switched the default panic behavior from `unwind` to `abort`. This makes panics fail immediately and take down the entire application, regardless of where they occur, which is consistent with our treatment of misbehaving connections.
The second bug is the panic itself. This was triggered by a duplicate entry in the initial_peers set. To fix this we've switched the storage for the peers from a `Vec` to a `HashSet`, which has similar properties but guarantees uniqueness of its keys.
- Add a total peers metric to prevent races between measurements of
ready/unready peers (which can cause the sum to be wrong).
- Add an outbound request counter.
tower-buffer uses tokio's mpsc channels, not the futures-rs mpsc channels.
Unlike futures-rs mpsc channels, which have capacity n+m, where n is the buffer
size and m is the number of senders, tokio channels always have buffer size n.
This means that the buffer size is shared across all peer set handles.
Thanks to @hawkw for sharing details of the Tokio internals!
Previously, we relied on the owner of the handshake future to drive it to
completion. This meant that there were cases where handshakes might never be
completed, just because nothing was actively polling them.
The previous outbound peer connection logic got requests to connect to new
peers and processed them one at a time, making single connection attempts
and retrying if the connection attempt failed. This was quite slow, because
many connections fail, and we have to wait for timeouts. Instead, this logic
connects to new peers concurrently (up to 50 at a time).
Bitcoin does this either with `getblocks` (returns up to 500 following block
hashes) or `getheaders` (returns up to 2000 following block headers, not
just hashes). However, Bitcoin headers are much smaller than Zcash
headers, which contain a giant Equihash solution block, and many Zcash
blocks don't have many transactions in them, so the block header is
often similarly sized to the block itself. Because we're
aiming to have a highly parallel network layer, it seems better to use
`getblocks` to implement `FindBlocks` (which is necessarily sequential)
and parallelize the processing of the block downloads.
This doesn't clean the warnings about unused items in the builder, since
those are unused for a reason (the implementation that should use them
is missing).
PushPeers is more complicated to thread into the rest of our
architecture (we would need to establish a data path connecting our
service handling inbound requests to the network layer's auto-crawler),
and since we crawl the network automatically anyways, we don't actually
need to accept them in order to get updated address information.
The only possible problem with this approach is that zcashd refuses to
answer multiple address requests from the same connection, ostensibly
for fingerprinting prevention (although it's totally happy to give
exactly the same information, as long as you hang up and reconnect
first, lol). It's unclear how this will interact with our design -- on
the one hand, it could mean that we don't get new addr information when
we ask, but on the other hand, we may have enough churn in our
connection pool that this isn't a problem anyways.
Attempting to implement requests for block data revealed a problem with
the previous connection logic. Block data is requested by sending a
`getdata` message with hashes of the requested blocks; the peer responds
with a sequence of `block` messages with the blocks themselves.
However, this wasn't possible to handle with the previous connection
logic, which could only convert a single Bitcoin message into a
Response. Instead, we factor out the message handling logic into a
Handler, which can statefully accumulate arbitrary data into a Response
and signal completion. This is still pretty ugly but it does work.
As a side effect, the HeartbeatNonceMismatch error is removed; because
the Handler now tries to process messages until it comes to a Response,
it just ignores mismatched nonces (and will eventually time out).
The previous Mempool and Transaction requests were removed but could be
re-added in a different form later. Also, the `Get` prefixes are
removed from `Request` to tidy the name.
Closes#158.
As discussed on the issue, this makes it possible to safely serialize
data into hashes, and encourages serializable data to make illegal
states unrepresentable.
These are included in the Block, Transaction objects themselves, so the
previous code ended up trying to deserialize two version fields per
object.
Closes#226.
This replaces the read_list function and makes the code significantly cleaner.
The only downside is that it loses exact preallocation, but this is probably not a big deal.
This means that all sub-modules of `peer` can import everything they need from
the `peer` module itself, without having to be aware of the internal structure
of their sibling modules.
With a 'Transactions' response that gets turned into an 'Inv(Vec<InventoryHash::Tx>)' message.
We don't yet handle a response from our peer for a 'mempool', which will have to be
a more generic 'Inv' type because we might receive transaction hashes we don't know about yet.
Pertains to #26
This does not yet push requests into services that actually respond with transaction
hashes in our node's mempool, which doesn't exist yet.
Pertains to #26
Moved SeedService out of the command closure Command currently spawns
a tokio task to DOS the seed service with `Request::GetPeers` every
second.
Pertains to #54
It's only responsible for doing the handshakes, so it should be named that way,
and then we can have a Connector responsible for actually opening the TCP
connection.
The toml serializer function we are using -- maybe because of to_string_pretty
(?) barfs on structs that mix ordering of simple values and "tables", so just
keep all the Durations to the end.
This splits out the connection handling code into a try_connect closure, which
could be refactored into a Service of its own.
On creation, when we are likely to have very few peers, launch many concurrent
connections to the first few candidates in the initial candidate set, before
continuing to grow the peer set according to demand signals.
The previous implementation failed when timestamps were duplicated between
peers, because there was not a 1-1 relationship between timestamps and peers.
The disconnected_peers() function allows us to prevent duplicate
connections without maintaining shared state between the peerset and the
dial-additional-peers task.
Previously, the TimestampCollector was intended to own the address book
data, so it was intended to be cloneable and hold shared state among all
of its handles. This is now modeled more directly by an
`Arc<Mutex<AddressBook>>`, so the only functionality left in the
`TimestampCollector` is setting up the inital worker, which is better
called `spawn` than `new`.
This also fixes a problem introduced in the previous commit where the
`TimestampCollector` was dropped, causing the worker task to shut down
early.
This allows us to hide the `TimestampCollector` and to expose only the
address book data required by the inbound request service. It also lets
us have a common data structure (the `AddressBook`) for collecting peer
information that can be used to manage information that other peers
report to us.
Doctests can only test public API, so now that the Codec is private, we can't
have a doctest. Since this test was only a code example (no behaviour test),
there's no value in replacing it with a unit test.
This gives API consumers a convenient name, and makes the Rustdoc output
significantly cleaner (because `init` can return a `BoxedZebraService`, not a
`Box<dyn ...ManyTypeConstraints.......>`.
Now that the `PeerConnector` handles both incoming and outgoing
handshakes, determining the next peer address is definitely out of scope
-- it takes a pre-existing tcp connection.
Failure uses a distinct Fail trait rather than the standard library's
Error trait, which causes a lot of interoperability problems with tower
and other Error-using crates. Since failure was created, the standard
library's Error trait was improved, and its conveniences are now
available without the custom Fail trait using `thiserror` (for easy
error derives) and `anyhow` (for a better boxed Error).
* Don't expose submodules of zebra_network::peer.
* PeerSet, PeerDiscover stubs.
Co-authored-by: Deirdre Connolly <deirdre@zfnd.org>
* Initial work on PeerSet.
This is adapted from the MIT-licensed tower-balance implementation.
* Use PeerSet in the connect stub.
This adds a type alias, BoxedStdError, for a boxed std::error::Error
trait object, and uses it in the where bounds for the generic service
code. In the future, we may want to standardize on using
std::error::Error exclusively, but we would then possibly lose out on
backtrace information.
* Fix authorship, license information.
I *thought* I had done a sed pass over the Cargo defaults when doing
repository initialization, but I guess I missed it or something.
Anyways, fixed now.
Add a tower-based peer implementation.
Tower provides middleware for request-response oriented protocols, while Bitcoin/Zcash just send messages which could be interpreted either as requests or responses, depending on context. To bridge this mismatch we define our own internal request/response protocol, and implement a per-peer event loop that scans incoming messages and interprets them either as requests from the remote peer to our node, or as responses to requests we made previously. This is performed by the `PeerService` task, and a corresponding `PeerClient: tower::Service` can send it requests. These tasks are themselves created by a `PeerConnector: tower::Service` which dials a remote peer and performs a handshake.
This field is called `services` in Bitcoin and Zcash, but because we use
that word internally for other purposes, calling it `PeerServices`
disambiguates the meaning to "the services advertised by the peer",
rather than, e.g., a `tower::Service`.
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.
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).
This provides a significantly cleaner API to consumers, because it
allows using adaptors that convert a TCP stream to a stream of messages,
and potentially allows more efficient message handling.
Because we want to be able to read messages from async sources (like a
tcp socket), we need to have at least async header parsing logic, so
that we can correctly determine how many bytes to await to parse each
message, so it makes sense for the entire message parsing functions
to be async.
Because we perform message serialization into async readers and writers
in the context of sending messages over the network, code using these
functions is more clear with these names.
When we perform a handshake with a remote peer, we need to encode the
version messages with a particular network version before we find out
what the remote peer's version preference is. So in addition to having
a CURRENT_VERSION constant (which represents our preference), we need to
have a MIN_VERSION during the handshake (and later to determine whether
we'll talk to the peer at all).
This adds convenience methods to `ReadZcashExt` that read 4 and 12 byte
fixed size arrays from the `Reader`, making the actual parsing code more
legible.
Closes#10.
* Replace Version MetaAddr by (Services, SocketAddr).
The version handshake message doesn't include last-seen timestamps for
the address fields, unlike other messages, so instead of modeling the
message data with a `MetaAddr` (which includes a timestamp), we should
just use a tuple.
* Simplify try_read_version implementation.
Because we no longer need to construct fake timestamps for the
`MetaAddr` fields, we don't need to use any of the parsed fields while
parsing later fields, and we can neatly wrap up the entire parsing logic
into a single expression.
* fmt
I didn't have the toolchain-specified `rustfmt` because I was mostly
offline and couldn't download it.
This has a test that the serialization implementation round trips
correctly, but is very much a work in progress. Issues with this code
include:
The deserialization logic for message headers is somewhat ugly because
of a lack of a convenient idiom for reading a few bytes at a time into a
fixed-size array. Perhaps this could be fixed with an extension trait,
or avoided altogether by using `u32`s instead of `[u8; 4]`, even when
the latter is the "morally correct type".
Deserialization does an extra allocation, copying the body into a
temporary buffer. This avoids two problems: 1) capping the number of
bytes that can be read by the `Read`er passed into the body parser and
2) allows making two passes over the body data, one to parse it and one
to compute the checksum.
We could avoid making two passes over the body by computing the checksum
simultaneously with the parsing. A convenient way to do this would be to
define a
```
struct ChecksumReader<R: Read> {
inner: R,
digest: Sha256,
}
impl<R: Read> Read for ChecksumReader<R> { /* ... */ }
```
and implement `Read` for `ChecksumReader` to forward reads from the
inner reader, copying data into the digest object as it does so. (It
could also have a maximum length to enforce that we do not read past the
nominal end of the body).
A similar `ChecksumWriter` could be used during serialization, although
because the checksum is at the beginning rather than the end of the
message it does not help avoid an allocation there. It could also be
generic over a `Digest` implementation, although because we need a
truncated double-SHA256 we would have to write a custom `Digest`
implementation, so this is probably not worthwhile unless we have other
checksum types.
Finally, this does very minimal testing -- just round-trip serialization
on a single message. It would be good to build in support for
property-based testing, probably using `proptest`; if we could define
generation and shrinking strategies for every component type of every
message, we could do strong randomized testing of the serialization.
The core serialization logic is now in zebra-chain and consists of two
pairs of traits:
These are analogues of the Serde `Serialize` and `Deserialize` traits,
but explicitly intended for consensus-critical serialization formats.
Thus some struct `Foo` may have derived `Serialize` and `Deserialize`
implementations for (internal) use with Serde, and explicitly-written
`ZcashSerialize` and `ZcashDeserialize` implementations for use in
consensus-critical contexts. The consensus-critical implementations
provide `zcash`-prefixed `zcash_serialize` and `zcash_deserialize`
methods to make it clear in client contexts that the serialization is
consensus-critical.
These are utility traits, analogous to the `ReadBytesExt` and
`WriteBytesExt` traits provided by `byteorder`. A generic
implementation is provided for any `io::Read` or `io::Write`, so that
bringing the traits into scope adds additional Zcash-specific traits to
generic readers and writers -- for instance, writing a `u64` in the
Bitcoin "CompactSize" format.
Currently these just have write_compactsize and read_compactsize methods which
allow reading and writing u64s to any `Read` or `Write` implementation using
the Bitcoin "CompactSize" variable integer encoding.
These methods read and write u64s rather than defining a new `CompactSize`
type, because the `CompactSize` is just an encoding detail, not a different
type with any distinct meaning.
The `NetworkAddress` type was a `(Services, SocketAddr)` pair as used in the
`version` handshake message, described as the `net_addr` struct in the Bitcoin
wiki protocol documentation. However, all of the other uses of the `net_addr`
struct are a `(Timestamp, Services, SocketAddr)` pair (where the timestamp is
the last-seen time of the peer), and the timestamp is omitted only during the
`version` messages, which are used only during the handshake, so it seems
better to include the timestamp field and omit it during serialization of
`version` packets.
Deirdre Connolly
Henry de Valence
Jane Lusby
Jane Lusby
George Tankersley
dependabot-preview[bot]
George Tankersley