PrivateInterest, PrivateAreaContext + relative encodings to Area #26

Merged
gwil merged 8 commits from private-area-context into main 2026-02-16 13:46:00 +01:00
Owner

Implements PrivateInterest and PrivateAreaContext and implements RelativeEncodable<PrivateAreaContext> for Area. Includes fuzz tests, and a great new subspace_includes_subspace function.

Implements [`PrivateInterest`](https://willowprotocol.org/specs/pio/index.html#PrivateInterest) and [`PrivateAreaContext`](https://willowprotocol.org/specs/encodings/index.html#PrivateAreaContext) and implements `RelativeEncodable<PrivateAreaContext>` for `Area`. Includes fuzz tests, and a *great* new `subspace_includes_subspace` function.
AljoschaMeyer left a comment

Wrought.

Wrought.
@ -271,1 +276,3 @@
self.0.can_be_encoded_relative_to(rel.into())
self.0
.can_be_encoded_relative_to(<&willow_data_model::groupings::Area<
4096,

Sorry, missed this one in earlier reviews: please use the proper constants here and elsewhere (just do a search for 4096 in willow25).

Sorry, missed this one in earlier reviews: please use the [proper constants](https://docs.rs/willow25/latest/willow25/constant.MCC.html) here and elsewhere (just do a search for 4096 in `willow25`).
AljoschaMeyer marked this conversation as resolved
@ -122,1 +124,4 @@
}
/// Returns `true` if the outer `Option<S>` includes the inner `Option<S>`, where `None` denotes interest in all subspaces of the namespace.
fn subspace_includes_subspace<S: PartialEq>(outer: Option<&S>, inner: Option<&S>) -> bool {

Huh, doesn't this need to be pub(crate) fn bla?

Huh, doesn't this need to be `pub(crate) fn bla`?
Author
Owner

I remember this function being quite handy outside of the crate, but happy to scope it to the crate if you disagree.

I remember this function being quite handy outside of the crate, but happy to scope it to the crate if you disagree.

Right now it is private, or am I missing something?

Right now it is *private*, or am I missing something?
AljoschaMeyer marked this conversation as resolved
@ -0,0 +86,4 @@
/// Returns `true` is this [`PrivateInterest`] is [comparable](https://willowprotocol.org/specs/pio/index.html#pi_comparable) to `other`.
pub fn is_comparable(&self, other: &Self) -> bool {
self.is_more_specific(other) || other.is_more_specific(other)

Typo, second one should not compare other to other.

The most clearly readable version is self.is_more_specific(other) || self.is_less_specific(other) in my opinion.

Typo, second one should not compare `other` to `other`. The most clearly readable version is `self.is_more_specific(other) || self.is_less_specific(other)` in my opinion.
AljoschaMeyer marked this conversation as resolved
@ -0,0 +90,4 @@
}
/// Returns true if `self` and `other` are [awkward](https://willowprotocol.org/specs/pio/index.html#pi_awkward), meaning they are neither [comparable](https://willowprotocol.org/specs/pio/index.html#pi_comparable) nor [disjoint](https://willowprotocol.org/specs/pio/index.html#pi_disjoint).
pub fn awkward(&self, other: &Self) -> bool {

I prefer are_awkward, to indicate that this is a predicate (compare is_comparable, is_more_specific, etc). Or maybe is_awkward? Or possibly even is_awkward_with? All of these read better than just awkward in actual code that calls this, in any case.

I prefer `are_awkward`, to indicate that this is a predicate (compare `is_comparable`, `is_more_specific`, etc). Or maybe `is_awkward`? Or possibly even `is_awkward_with`? All of these read better than just `awkward` in actual code that calls this, in any case.
AljoschaMeyer marked this conversation as resolved
@ -0,0 +98,4 @@
impl<const MCL: usize, const MCC: usize, const MPL: usize, N: PartialEq, S: PartialEq + Clone>
PrivateInterest<MCL, MCC, MPL, N, S>
{
/// Returns `true` if the given [`Entry`] is included by `self`.

The word "included" should be a link to https://willowprotocol.org/specs/pio/index.html#pi_include_entry

The word "included" should be a link to `https://willowprotocol.org/specs/pio/index.html#pi_include_entry`
AljoschaMeyer marked this conversation as resolved
@ -0,0 +112,4 @@
impl<const MCL: usize, const MCC: usize, const MPL: usize, N, S: PartialEq + Clone>
PrivateInterest<MCL, MCC, MPL, N, S>
{
/// Returns `true` if the given [`Area`] in included by `self`.

The word "included" should be a link to https://willowprotocol.org/specs/pio/index.html#pi_include_area

The word "included" should be a link to `https://willowprotocol.org/specs/pio/index.html#pi_include_area`
AljoschaMeyer marked this conversation as resolved
@ -0,0 +114,4 @@
{
/// Returns `true` if the given [`Area`] in included by `self`.
pub fn includes_area(&self, area: &Area<MCL, MCC, MPL, S>) -> bool {
//self.subspace_id.includes_area_subspace(area.subspace())

delete the comment

delete the comment
AljoschaMeyer marked this conversation as resolved
@ -0,0 +119,4 @@
&& self.path.is_prefix_of(area.path())
}
/// Returns `true` if the given [`Area`] in related to `self`.

Comment should define "related" (this isn't in the spec, I think).

/// Returns `true` if the given [`Area`] in related to `self`, that is:
/// 
/// - the path of `self` and the path of the `area` are [related](Path::is_related_to), and
/// - either `self.subspace()` is `None` or `self.subspace() == area.subspace()`.
Comment should define "related" (this isn't in the spec, I think). ```rs /// Returns `true` if the given [`Area`] in related to `self`, that is: /// /// - the path of `self` and the path of the `area` are [related](Path::is_related_to), and /// - either `self.subspace()` is `None` or `self.subspace() == area.subspace()`. ```
AljoschaMeyer marked this conversation as resolved
@ -0,0 +143,4 @@
impl<const MCL: usize, const MCC: usize, const MPL: usize, N: Clone, S: Clone>
PrivateInterest<MCL, MCC, MPL, N, S>
{
/// Clones self, but replaces the subspace id with `any`.

None instead of any.

`None` instead of `any`.

(or make any link to the spec, but I prefer the self-contained rust-only version of the doc comment)

(or make `any` link to the spec, but I prefer the self-contained rust-only version of the doc comment)
AljoschaMeyer marked this conversation as resolved
@ -0,0 +162,4 @@
N: Arbitrary<'a>,
S: Arbitrary<'a>,
{
fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result<Self> {

Am I missing a reason why this cannot simply be derived?

Am I missing a reason why this cannot simply be derived?
AljoschaMeyer marked this conversation as resolved
@ -0,0 +188,4 @@
pub struct PrivateAreaContext<const MCL: usize, const MCC: usize, const MPL: usize, N, S> {
/// The [`PrivateInterest`] to be kept private.
private: PrivateInterest<MCL, MCC, MPL, N, S>,

spurious newline

spurious newline
AljoschaMeyer marked this conversation as resolved
@ -0,0 +189,4 @@
/// The [`PrivateInterest`] to be kept private.
private: PrivateInterest<MCL, MCC, MPL, N, S>,
/// The almost containing Area relative to which we encode.

"almost containing" should be a link to the spec, "Area" be a link to the struct.

Also "almost-containing"?

"almost containing" should be a link to the spec, "Area" be a link to the struct. Also "almost-containing"?
AljoschaMeyer marked this conversation as resolved
@ -0,0 +197,4 @@
#[derive(Debug, Clone, Copy)]
pub struct AreaNotAlmostIncludedError;
impl core::fmt::Display for AreaNotAlmostIncludedError {

Import the trait instead of using fully qualified names here.

Import the trait instead of using fully qualified names here.

🐭 Okay, sorry, I was imprecise here. Import everything and do not use fully qualified names anywhere =S

🐭 Okay, sorry, I was imprecise here. Import *everything* and do not use fully qualified names anywhere =S
AljoschaMeyer marked this conversation as resolved
@ -0,0 +199,4 @@
impl core::fmt::Display for AreaNotAlmostIncludedError {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
core::fmt::Display::fmt("ComponentsNotRelatedError", f)

Slightly misleading fmt implementation =P

Slightly misleading fmt implementation =P
AljoschaMeyer marked this conversation as resolved
@ -0,0 +208,4 @@
impl<const MCL: usize, const MCC: usize, const MPL: usize, N, S>
PrivateAreaContext<MCL, MCC, MPL, N, S>
{
/// Returns a new [`PrivateInterest`] with the given `private` and `rel` attributes. Will fail if the given [`PrivateInterest`]'s path is not a prefix of `rel`'s path.

Copy-pasted doc comment =(

Copy-pasted doc comment =(
AljoschaMeyer marked this conversation as resolved
@ -0,0 +253,4 @@
N: Arbitrary<'a>,
S: Arbitrary<'a>,
{
fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result<Self> {

Can this be derived?

Can this be derived?
AljoschaMeyer marked this conversation as resolved
@ -0,0 +234,4 @@
impl<const MCL: usize, const MCC: usize, const MPL: usize, N, S: PartialEq>
PrivateAreaContext<MCL, MCC, MPL, N, S>
{
/// Returns whether an [`Area`] _almost includes_ another area, that is, if the other [`Area`] would be included by this if [`Area] if it had the same [`SubspaceId`].

This sentence no correct grammar.

This sentence no correct grammar.

🐭

Returns whether an [Area] almost includes another area, that is if the other [Area] would be included by this **if [Area] if it had** the same [SubspaceId`].

:mouse: Returns whether an [`Area`] _almost includes_ another area, that is if the other [`Area`] would be included by this **if [`Area] if it had** the same [`SubspaceId`].
AljoschaMeyer marked this conversation as resolved
@ -0,0 +348,4 @@
}
(Some(rel_end), Some(val_end)) => {
let end_from_start = *val_end - *rel.rel().times().start() <= *rel_end - *val_end;
// println!("end_from_start: {:?}", end_from_start);

remove comment

remove comment
AljoschaMeyer marked this conversation as resolved
@ -0,0 +362,4 @@
}
};
consumer.consume(either::Either::Left(header)).await?;

Import Left instead of using fully qualified paths.

Import `Left` instead of using fully qualified paths.
AljoschaMeyer marked this conversation as resolved
@ -0,0 +376,4 @@
_ => {}
}
/*

Remove comment.

Remove comment.
AljoschaMeyer marked this conversation as resolved
@ -0,0 +390,4 @@
cu64_encode(end_diff.into(), 2, consumer).await?;
}
// We can use new_unchecked because we know the paths from the context prefix the area's path.

Add a debug_assert! for that, please. The unsafe feels a lot less scary then, given our thorough fuzzing.

Add a `debug_assert!` for that, please. The `unsafe` feels a lot less scary then, given our thorough fuzzing.
AljoschaMeyer marked this conversation as resolved
@ -0,0 +397,4 @@
rel.rel().path().clone(),
);
self.path()

Unsafe blocks should be of minimal size; move the relative encoding out of the unsafe block.

Unsafe blocks should be of minimal size; move the relative encoding out of the unsafe block.
Author
Owner

But then private_path_context is out of scope.

But then `private_path_context` is out of scope.
AljoschaMeyer marked this conversation as resolved
@ -0,0 +407,4 @@
/// Returns `false` when `self` is not [almost included by](https://willowprotocol.org/specs/encodings/index.html#almost_include) the relative [`PrivateAreaContext`].
fn can_be_encoded_relative_to(&self, rel: &PrivateAreaContext<MCL, MCC, MPL, N, S>) -> bool {
!(!rel.almost_includes_area(self) || !rel.private.almost_includes_area(self))

!(!a || !b) is the same as a && b

`!(!a || !b)` is the same as `a && b`
AljoschaMeyer marked this conversation as resolved
@ -0,0 +494,4 @@
Some(end)
};
// We can use new_unchecked because we know the paths from the context prefix the area's path.

debug assertion here again

debug assertion here again
AljoschaMeyer marked this conversation as resolved
@ -0,0 +500,4 @@
rel.private().path().clone(),
rel.rel().path().clone(),
);

Close the unsafe block here (and open a new, small unsafe block later).

Close the `unsafe` block here (and open a new, small unsafe block later).
Author
Owner

But then private_path_context is no longer in scope when it is needed later.

But then `private_path_context` is no longer in scope when it is needed later.
AljoschaMeyer marked this conversation as resolved
@ -0,0 +509,4 @@
return Err(DecodeError::Other(Blame::TheirFault));
}
// We can do this we just rejected any case where time_start is strictly greater than end.
let times = TimeRange::new_closed_unchecked(time_start.into(), end.into());

debug assertion =)
(yes, this is overkill, but unsafe is really scary okay?)

debug assertion =) (yes, this is overkill, but `unsafe` is really scary okay?)
Author
Owner

Is it so scary that it's going to somehow make it past the same case returning two lines above?

Is it so scary that it's going to somehow make it past the same case returning two lines above?

Yeah, fair.

Yeah, fair.
AljoschaMeyer marked this conversation as resolved
@ -46,2 +46,4 @@
pub use super::test_parameters::*;
}
/// Checks if a bit of a given byte is flagged at a particular index (with 0 being the most significant bit)

Needs a dot at the end =S

Needs a dot at the end =S
AljoschaMeyer marked this conversation as resolved
AljoschaMeyer left a comment

🐭

🐭
@ -0,0 +122,4 @@
&& self.path.is_prefix_of(area.path())
}
/// Returns `true` if the given [`Area`] in related to `self`, that is:

🐭 Sorry, had a typo there. in [included] -> is [included].

🐭 Sorry, had a typo there. `in [included]` -> `is [included]`.
AljoschaMeyer marked this conversation as resolved
gwil merged commit 1e3d29ee81 into main 2026-02-16 13:46:00 +01:00
gwil deleted branch private-area-context 2026-02-16 13:46:00 +01:00
Sign in to join this conversation.
No description provided.