The construction of the debug object is unwieldy, and error prone
(for example, renamed struct members have to be manually renamed
in the serialization code, see "refs" vs. "refsAt").
Use derived serializers where possible to make this easier to maintain.
Wire up the new `FetcherService`. This reduces the `Service` fetch
methods down to:
- `fetch`
- `fetched`
- `dequeue_fetches`
- `fetch_refs_at`
This simplifies the code by off-loading the logic to the `fetcher`
family of types, rather than handling the intricacies in the `Service`
itself.
The `Service` now just performs the necessary I/O based on the
returned events from the `fetcher`.
This also removes the fetching state from the node sessions. This
follows the single responsibility, and sessions now only care about
their connectivity.
Breaking changes:
- The `Connected` state of a peer no longer contains fetching
information, which was being returned as part of the JSON payload
result.
- The `rad debug` information for ongoing fetches contained the number
listeners awaiting for results, this was removed.
Evaluate the `error!` logging in `radicle-protocol`.
The majority of cases, the `error!` is downgraded to `warn!`. These
cases are generally useful to know if there's an issue, but the
operator cannon necessarily do anything about it.
In a few cases, `debug!` was chosen. These are generally when an error
means that a result is not part of an accumulation, or the system will
eventually correct itself during another event or restart.
If the previous message started with "Error", then the wording is
changed to say "Failed to".
Evaluate the `warn!` logging in `radicle-protocol`.
If the event is a result of something being suspicious in the running
of the protocol, then it remains a `warn!`.
However, if the system ends up ignoring the event and can continue
without issues, the log level is reduced to `debug!`.
Previously, the `RepoId` was backed by the `git2::Oid` type, which has a `Hash`
implementation that uses `std:#️⃣:Hasher::hash`. This uses a length prefixed
form for hashing.
When switching to `radicle_oid::Oid` and implementing `Hash` without the length
prefix, this subtly changed the hashes going forward.
These hashes are used in `BloomFilter`, which is the type underlying `Filter`.
This resulted in cases where a previous `Filter` that certainly contained
`rad:z3gqcJUoA1n9HaHKufZs5FCSGazv5` were suddenly reporting that they did not
contain this repository.
Changing the implementation of `Hash` for `radicle_oid::Oid` fixes this, and is
confirmed by adding the regression test in `filter.rs`.
I was greeted by `rad patch redact` with
called `Result::unwrap()` on an `Err` value:
Error { code: None, message: Some("failed to convert") }
stack backtrace:
[…]
3: core::result::Result<T,E>::unwrap
at …/rust-1.88.0/lib/rustlib/src/rust/library/core/src/result.rs:1137:23
4: sqlite::cursor::Row::read
at …/index.crates.io-1949cf8c6b5b557f/sqlite-0.32.0/src/cursor.rs:136:9
5: radicle::cob::patch::cache::query::find_by_revision
at ./crates/radicle/src/cob/patch/cache.rs:624:65
6: <… as radicle::cob::patch::cache::Patches>::find_by_revision
at ./crates/radicle/src/cob/patch/cache.rs:553:9
7: radicle_cli::commands::rad_patch::redact::run
at ./crates/radicle-cli/src/commands/patch/redact.rs:23:9
8: radicle_cli::commands::rad_patch::run
at ./crates/radicle-cli/src/commands/patch.rs:1026:13
[…]
It turns out that `sqlite::cursor::Row::read` is the panicky version of
`sqlite::cursor::Row::try_read` (which returns a `Result`).
While it is somewhat rare that SQLite reads fail, it is not unheard of.
In `radicle-cli` it might not be critical, but also `radicle-protocol`
and `radicle-fetch` are affected, and they could potentially panic a
`radicle-node` process.
Use `try_read` instead, and propagate down the error handling.
Remove `fn deserialize` and instead provide a trait `fn decode_exact`
for testing.
Refactor errors, so that we can now cleanly distinguish between
`Error::UnexpectedEnd` which means that decoding might work given
more data, and `Error::Invalid`, which means that no amount of
additional data will decode.
While at it, cleanup the variants of `Error::Invalid`, and provide `ControlType`.
What made the issue fixed in the parent commit harder to spot
for me was the multitude of concepts and terms involved in
encoding: What's *serializing* vs. *encoding*? Why is there
this odd `Frame::to_bytes`?
So I decided to clean up a bit. I removed `fn serialize` so that the
logic stays close to the data, and instead of it provide
`fn encode_vec` in `trait Encode` so that we don't add up with
one-offs like `Frame::to_bytes` again.
Also we make use of `encode_vec` in the tests.
Mostly changes of `use`s and moving components to `radicle-protocol`,
with the goal of just getting `radicle-node` to work on top of the new
`radicle-protocol` crate.
Fintan Halpenny
Lorenz Leutgeb
Yaroslav Halchenko