Any connected peer can send a `Subscribe` with `since` after `until`.
The service passes the range to `Store::filtered`, which asserts
`from <= to`. The panic stops the service thread.
Remove the assertion. The query already returns no rows when
`from > to`, so the peer gets no history.
There is no apparent reason why announcements with a timestamp of zero
cannot be stored. Avoid panicking.
Co-authored-by: Lorenz Leutgeb <lorenz.leutgeb@radicle.dev>
To determine whether a patch has been merged to the default branch, we
tally merges based on the default branch quorum: allow set and
threshold. With the introduction of custom branches, the tallying was
not updated to use the quorum rules for the target branch, instead
merging on the identity document threshold and delegate set.
This fix uses the `allow` set and the `threshold` of the most specific
rule that matches the target branch of patch.
Note that a reference that no rule matches never becomes canonical, so a
patch that targets one can never reach a quorum.
The crates `radicle` and `radicle-node` need the default hasher for
`IndexMap<K, V>` and `IndexSet<T>`, which currently gets enabled
indirectly by `serde_json`, which depends on `indexmap` with `std`
enabled by default.
Make the dependency on the `std` feature of `indexmap` explicit.
Co-authored-by: Lorenz Leutgeb <lorenz.leutgeb@radicle.dev>
Serialization and deserialization of symbolic references was implemented
with assumptions on the order of pairs of key and value in JSON objects,
and further, that the enabling the `preserve_order` of `serde_json` would
always guarante that these assumptions hold. See
<https://docs.rs/crate/serde_json/1.0.149/features#preserve_order>.
This is mistaken. The identity document is serialized via `Doc::encode`
which uses `radicle::…::CanonicalFormatter`, which enforces a particular
ordering. From the point of view of the implementation of symbolic
references, this means that the ordering changes, thus violating the
ordering assumption.
The effects are that during deserialization, some symbolic references
might be considered dangling, when they are only missing other symbolic
references that were reordered to occur "later" in the JSON object.
To remedy, introduce `RawSymbolicRefs`, in analogy to `RawRules`. Just
like `RawRules` may be converted to `Rules`, `RawSymbolicRefs` may be
converted to `SymbolicRefs`. This gives the most flexibility for
handling `RawSymbolicRefs`, and one clear point at which validation
happens.
The fact that the implementation cannot depend on ordering anymore means
that all symbolic references have to be validated together, which fits
the pattern of `Raw{,*}` well.
Tests expose the issue.
Co-authored-by: Lorenz Leutgeb <lorenz.leutgeb@radicle.dev>
Testing revealed that the implementation violated RIP-0006, which
requires the most specific rule matching the target of `HEAD`
to use an `allow` of `delegates` and the document threshold.
`validate_head_rule` took the first match from `RawRules`, which orders
patterns lexicographically, so a glob could be validated in place of the
rule that actually governs the branch.
The issue here is that the correct order for matching patterns is only
guaranteed on `Rules`, but not on `RawRules`.
To prevent such errors in the future, remove `RawRules::matches`. This
requires some refactoring, and makes synthesis of the default branch
rule slightly less efficient in the worst case, but we are willing to
take that hit for a cleaner implementation.
Co-authored-by: Lorenz Leutgeb <lorenz.leutgeb@radicle.dev>
When `Service::handle_message` handles `Message::Subscribe`, constraints
regarding visibility of repositories are not considered, this leads to
leaking RIDs of private repositories.
Add a corresponding check.
Add a test to verify that reference announcements relayed via subscribe
correctly apply visibility filtering, there by ensuring references of
private repositories are only propagated to nodes on the allow list.
The test regarding deduplication needs a fix: Ensure that all mock
repositories are public and in storage so that they do not
get filtered out in case of unknown visiblity.
Co-authored-by: Lorenz Leutgeb <lorenz.leutgeb@radicle.dev>
- Assert that with the peer still `Connected`, `worker_result` clears
`active[rid]` as expected
- Confirm the disconnect test is exercising the discard path rather than
passing vacuously
- Drive `worker_result` through the real wire layer with the peer in
`Disconnecting` state, mirroring a fetch that ends mid-disconnect
- Assert `active[rid]` survives, proving the result is discarded without
calling `service.fetched`, which is what orphans the entry
- Add helpers to wrap a service with an active fetch in a `Wire`
- Drive the real cancel-skip path: a connection conflict makes
`disconnected` early-return on a link mismatch, leaving the fetch
uncancelled
- Combined with an undelivered fetch result, the `active[rid]` entry is
orphaned and the repo can no longer be fetched from any node
- Confirm delivering the missing result clears the entry and unblocks
the queued fetch, showing what the fix must guarantee
- Clear active[rid] only when the result's node matches the node that
started the fetch; mismatched results now report NotFound
- Prevent a late completion from a disconnected peer clearing a newer
fetch for the same repo started by a different node
- Add test covering the stale-from mismatch path
A fetch result was discarded whenever the peer was no longer `Connected`
by the time the worker reported, and queued fetches were dropped silently
when the peer disconnected before the `Io::Fetch` was processed. In both
cases the fetcher's `active` entry for the repo was never cleared. Since
that entry is keyed by repo, it blocked the repository from being fetched
from any node until the process restarted.
- Report the result in `worker_result` even when the peer is no longer
connected, instead of returning early
- Report a failed fetch when an `Io::Fetch` is dropped for a disconnected
peer, so the active entry is cleared
- Flip the wire regression test to assert the entry is now cleared on
disconnect
- Fixes a bug where the log level set in the config file was
ignored: `Logger` and `StderrLogger` captured the level in a
`self.level` field at construction time and checked it in
`Log::enabled`. After config was loaded, the global
`log::set_max_level` was updated but `self.level` was not,
so verbose messages were dropped by the per-instance filter
even when the global filter allowed them.
- Make `log::set_max_level` the single source of truth: remove
the `level` field and have `Log::enabled` defer to
`log::max_level()` so the two filters can no longer drift.
- Update call sites to construct loggers without a level.
- Disable the structured logger's internal filter (set to "trace") so
that it also falls back to `log::set_max_level`.
- Drop `type='ipv6'` rows whose bracket-stripped inner part contains
no `:`, since every valid IPv6 textual form has at least one
- Cover the offender (`[]:8776`), bracketed garbage (`[abc]:8776`),
the unspecified (`::`), loopback, and full forms in tests
- Keep `dns` and `ipv4` rows untouched
Migration 8 retyped any `[..]:N` dns row to ipv6 by bracket pattern
alone, without checking the inner part is parseable
- Decode each row independently so one corrupt entry no longer aborts
the whole address-book iterator
- Log a warn with the raw `value` and continue past parse failures
- Add a regression test that mimics the post-migration-8 state where a
legacy `[]:8776` row lands as `type='ipv6'`
A pre-`df8e4e6c` parser admitted `[]:8776` as `HostName::Dns("[]")`.
Migration 8 retyped any `[..]:N` dns row to `ipv6` without validating
the inner part, so on read `Address::from_str` rejects the row with
"invalid IPv6 address syntax", which bubbled up and caused
`available_peers()` to silently return empty, breaking peer discovery
for the affected node.
Daniel Norman