v0.19: ensembles: network #1493

Open
vyzo wants to merge 76 commits from v0.19-std-ensemble into v0.19-staging
Owner

This is the first part of the v0.19 ensembles; complete network implementation.
It is industrial strength with excellent coverage and performance; interaction diagrams for humans included in the documentation.

codesigned and implemented with astra, with oversight from the gerbilosaurus rex 🦖.

This is the first part of the v0.19 ensembles; complete network implementation. It is industrial strength with excellent coverage and performance; interaction diagrams for humans included in the documentation. codesigned and implemented with astra, with oversight from the gerbilosaurus rex 🦖.
vyzo added 14 commits 2026-09-09 23:35:44 +00:00
vyzo added 28 commits 2026-09-11 22:21:55 +00:00
vyzo added 11 commits 2026-09-12 21:14:34 +00:00
vyzo force-pushed v0.19-std-ensemble from 837e16223a to deb9873961 2026-09-14 17:05:42 +00:00 Compare
vyzo changed title from WIP: v0.19: ensembles to v0.19: ensembles: network 2026-09-14 17:06:42 +00:00
vyzo requested reviews from fare, HMarcien, jay 2026-09-14 17:08:06 +00:00
vyzo force-pushed v0.19-std-ensemble from deb9873961 to d6d13b539f 2026-09-17 18:20:37 +00:00 Compare
Author
Owner

rebased on staging.

rebased on staging.
Owner

First comments from Sol:

I’d request changes on the current rebased head. I found two concrete identity/authentication problems and one verification blocker. There are also two lesser API concerns. The PR is huge—192 files, +43,678/−1,437—and the current head was force-pushed/rebased onto staging only minutes before this review. (cons.io git)

  1. P1 — did->public-key deliberately accepts invalid did:key identifiers. The new tests explicitly require acceptance of U, m, and M multibase forms, plus strings containing !, ==ignored, and even a trailing newline. The documentation likewise says this permissiveness is intentional. (cons.io git) But the current did:key method ABNF permits exactly base58btc z... or unpadded base64url u...; characters such as !, =, or newline are outside the grammar. (W3C CCG) RFC 4648 also says that, absent an explicit contrary rule, decoders must reject non-alphabet characters, specifically warning that ignoring them can create covert channels or defeat string equality checks—which is particularly relevant here because capability identities are compared as strings. (RFC Editor)

    I would ask for this before merge: restrict did->public-key to the z and u encodings actually permitted by did:key, validate the complete lexical representation rather than relying on the permissive generic base64 decoder, and reverse those “permissive Base64url decoding” tests into rejection tests. I’d also reject noncanonical base64 pad bits (...LB decoding like ...LA); otherwise there are still multiple u spellings for one byte sequence.

  2. P1 — network DID canonicalization and UCAN root trust have incompatible semantics. The network explicitly canonicalizes API DIDs by decoding and re-encoding them, so e.g. a valid base58btc did:key:z... supplied as a peer identity becomes its did:key:u... form. Signed token fields are intentionally left untouched. (cons.io git) Meanwhile CapabilityContext.add-root! deliberately stores the root string verbatim, and token-rooted-at? tests the issuer with plain equal?. (cons.io git) The DID tests demonstrate that a base58 and canonical-base64url DID can decode to exactly the same public key. (cons.io git)

    So there is a very plausible failure mode:

    add-root!("did:key:z...") → succeeds
    network authenticates that same key as canonical did:key:u... → token signature verifies
    token-rooted-at? → false
    AnchorVerificationError.

    This isn't covered by the root tests: they only add roots using the already-canonical DID spelling. Interestingly, the context tests do already exercise a base58 alias for private-key lookup, so the representation issue is known elsewhere in the API. (cons.io git)

    I’d canonicalize and validate explicit roots on insertion/removal, or otherwise make the network/trust semantics agree, and add exactly the base58-root/canonical-token regression test above. Fixing finding 1 does not eliminate this one because both z and u are valid did:key encodings.

  3. P1 before merge — rerun the broader regression that previously crashed natively. The branch's own acceptance record says that the earlier full regression produced native invalid-return/dispatch crashes whose exact cause remains unknown. After the staging rebase, both ensemble-only runs passed all 30 modules/375 named cases, including the formerly crashing paths—but the broader regression was interrupted and explicitly not rerun. (cons.io git)

    Given that this PR also changes shared code outside Ensemble—BIO writers/cache, SSL cleanup/timeouts, socket semantics, thread joining, make freshness, resource unwinding, etc.—the ensemble-only suite isn't enough evidence that the unexplained crash disappeared rather than merely stopped being exercised. The PR commit list confirms those supporting-library changes are part of the change set. (cons.io git) I would require at least the previously failing broad command from the implementation notes, preferably the full regression, on current d6d13b539f before approving.

  4. P2/design — Network.close has a surprisingly dangerous context restriction. The public interface explicitly warns that calling Network.close from any monitor callback can deadlock because the closer waits for the supervisor, while the supervisor waits for the callback's worker. (cons.io git) The implementation indeed sets closed? and synchronously joins the supervisor unless invoked by the supervisor itself; it doesn't recognize one of its job workers. (cons.io git)

    The documentation makes this not a hidden bug, but I still dislike it as an API contract. A monitor callback is exactly where application policy may naturally decide “fatal condition, shut the network down.” Requiring it to remember to dispatch that operation to some unrelated application worker is a liveness footgun. I’d prefer Network.close to detect an owned worker and request shutdown without self-dependent joining, even if synchronous completion is only guaranteed to outside callers.

  5. Minor/scope — public revocation API is knowingly a trap. REVOKE is a public token type, tokens have revocation-specific args, save-token! is documented as being for future revocation, and revoke! is exported—but revoke! unconditionally raises TODO; the docs explicitly admit this. (cons.io git) Since this PR claims a complete network rather than complete UCAN, I wouldn't necessarily block on implementing revocation. I would consider not exporting revoke! as an apparently callable API until it exists.

I also chased two things that initially looked suspicious and would not comment on them: the conversion of an absolute handshake deadline through IOTimeout is intentional given the timeout representation, and the asymmetric renewal protocol is intentional—connection renewal is mutual while stream renewal is one-way authorization.

Of these, #1 is the clearest code defect, #2 is the most interesting cross-layer bug, and #3 is the strongest reason not to merge the branch yet. #4 is worth discussing with vyzo; #5 is cleanup/scope.

I couldn't independently run this branch: the Forgejo web diff is too large for its normal files view and the local environment couldn't resolve git.cons.io, so this is source-level review plus examination of the branch's own recorded test evidence, not an independent test run.

First comments from Sol: I’d **request changes** on the current rebased head. I found two concrete identity/authentication problems and one verification blocker. There are also two lesser API concerns. The PR is huge—192 files, +43,678/−1,437—and the current head was force-pushed/rebased onto staging only minutes before this review. ([cons.io git][1]) 1. **P1 — `did->public-key` deliberately accepts invalid `did:key` identifiers.** The new tests explicitly require acceptance of `U`, `m`, and `M` multibase forms, plus strings containing `!`, `==ignored`, and even a trailing newline. The documentation likewise says this permissiveness is intentional. ([cons.io git][2]) But the current `did:key` method ABNF permits exactly base58btc `z...` or *unpadded* base64url `u...`; characters such as `!`, `=`, or newline are outside the grammar. ([W3C CCG][3]) RFC 4648 also says that, absent an explicit contrary rule, decoders **must reject non-alphabet characters**, specifically warning that ignoring them can create covert channels or defeat string equality checks—which is particularly relevant here because capability identities are compared as strings. ([RFC Editor][4]) I would ask for this before merge: restrict `did->public-key` to the `z` and `u` encodings actually permitted by `did:key`, validate the complete lexical representation rather than relying on the permissive generic base64 decoder, and reverse those “permissive Base64url decoding” tests into rejection tests. I’d also reject noncanonical base64 pad bits (`...LB` decoding like `...LA`); otherwise there are still multiple `u` spellings for one byte sequence. 2. **P1 — network DID canonicalization and UCAN root trust have incompatible semantics.** The network explicitly canonicalizes API DIDs by decoding and re-encoding them, so e.g. a valid base58btc `did:key:z...` supplied as a peer identity becomes its `did:key:u...` form. Signed token fields are intentionally left untouched. ([cons.io git][5]) Meanwhile `CapabilityContext.add-root!` deliberately stores the root string **verbatim**, and `token-rooted-at?` tests the issuer with plain `equal?`. ([cons.io git][6]) The DID tests demonstrate that a base58 and canonical-base64url DID can decode to exactly the same public key. ([cons.io git][2]) So there is a very plausible failure mode: `add-root!("did:key:z...")` → succeeds network authenticates that same key as canonical `did:key:u...` → token signature verifies `token-rooted-at?` → false → `AnchorVerificationError`. This isn't covered by the root tests: they only add roots using the already-canonical DID spelling. Interestingly, the context tests *do* already exercise a base58 alias for private-key lookup, so the representation issue is known elsewhere in the API. ([cons.io git][7]) I’d canonicalize and validate explicit roots on insertion/removal, or otherwise make the network/trust semantics agree, and add exactly the base58-root/canonical-token regression test above. Fixing finding 1 does **not** eliminate this one because both `z` and `u` are valid `did:key` encodings. 3. **P1 before merge — rerun the broader regression that previously crashed natively.** The branch's own acceptance record says that the earlier full regression produced native invalid-return/dispatch crashes whose exact cause remains unknown. After the staging rebase, both ensemble-only runs passed all 30 modules/375 named cases, including the formerly crashing paths—but the broader regression was interrupted and explicitly **not rerun**. ([cons.io git][8]) Given that this PR also changes shared code outside Ensemble—BIO writers/cache, SSL cleanup/timeouts, socket semantics, thread joining, make freshness, resource unwinding, etc.—the ensemble-only suite isn't enough evidence that the unexplained crash disappeared rather than merely stopped being exercised. The PR commit list confirms those supporting-library changes are part of the change set. ([cons.io git][1]) I would require at least the previously failing broad command from the implementation notes, preferably the full regression, on current `d6d13b539f` before approving. 4. **P2/design — `Network.close` has a surprisingly dangerous context restriction.** The public interface explicitly warns that calling `Network.close` from any monitor callback can deadlock because the closer waits for the supervisor, while the supervisor waits for the callback's worker. ([cons.io git][9]) The implementation indeed sets `closed?` and synchronously joins the supervisor unless invoked by the supervisor itself; it doesn't recognize one of its job workers. ([cons.io git][10]) The documentation makes this not a hidden bug, but I still dislike it as an API contract. A monitor callback is exactly where application policy may naturally decide “fatal condition, shut the network down.” Requiring it to remember to dispatch that operation to some unrelated application worker is a liveness footgun. I’d prefer `Network.close` to detect an owned worker and request shutdown without self-dependent joining, even if synchronous completion is only guaranteed to outside callers. 5. **Minor/scope — public revocation API is knowingly a trap.** `REVOKE` is a public token type, tokens have revocation-specific `args`, `save-token!` is documented as being for future revocation, and `revoke!` is exported—but `revoke!` unconditionally raises `TODO`; the docs explicitly admit this. ([cons.io git][11]) Since this PR claims a complete **network** rather than complete UCAN, I wouldn't necessarily block on implementing revocation. I would consider not exporting `revoke!` as an apparently callable API until it exists. I also chased two things that initially looked suspicious and **would not comment on them**: the conversion of an absolute handshake deadline through `IOTimeout` is intentional given the timeout representation, and the asymmetric renewal protocol is intentional—connection renewal is mutual while stream renewal is one-way authorization. Of these, **#1 is the clearest code defect**, **#2 is the most interesting cross-layer bug**, and **#3 is the strongest reason not to merge the branch yet**. #4 is worth discussing with vyzo; #5 is cleanup/scope. I couldn't independently run this branch: the Forgejo web diff is too large for its normal files view and the local environment couldn't resolve `git.cons.io`, so this is source-level review plus examination of the branch's own recorded test evidence, not an independent test run. [1]: https://git.cons.io/mighty-gerbils/gerbil/pulls/1493 "https://git.cons.io/mighty-gerbils/gerbil/pulls/1493" [2]: https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/ucan/did-test.ss "https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/ucan/did-test.ss" [3]: https://w3c-ccg.github.io/did-key-spec/ "https://w3c-ccg.github.io/did-key-spec/" [4]: https://www.rfc-editor.org/info/rfc4648/ "https://www.rfc-editor.org/info/rfc4648/" [5]: https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/network/auth.ss "https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/network/auth.ss" [6]: https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/ucan/context.ss "https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/ucan/context.ss" [7]: https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/ucan/context-test.ss "https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/ucan/context-test.ss" [8]: https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/network/acceptance.md "https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/network/acceptance.md" [9]: https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/network/interface.ss "https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/network/interface.ss" [10]: https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/network/network.ss "https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/network/network.ss" [11]: https://git.cons.io/mighty-gerbils/gerbil/src/branch/v0.19-std-ensemble/src/std/ensemble/ucan/interface.ss "gerbil/src/std/ensemble/ucan/interface.ss at v0.19-std-ensemble - mighty-gerbils/gerbil - cons.io git"
Author
Owner

some preliminary review comments, before we discuss and address with astra:

  1. this is already mostly addressed in the follow up pr that implements hosts, where we do add did normalization primitives. we can pull the relevant changes down to this pr and ensure the capability context is normalized.
  2. see above, we can address this in this pr.
  3. the crash was almost certainly caused by raising in a thread-interrupt, which is a very dangerous operation that can leave the interpreter in corrupted state. i have very little doubt that this was the cause of the crash, but we can rerun the full test suite including all stdlib tests before merge.
  4. the best we can do here is have a thread local value that marks the network thread and attempting to call close raise an exception. better than a deadlock and not much more we can do without dramatically complicating the implementation.
  5. ok, we can remove it for now.
some preliminary review comments, before we discuss and address with astra: 1. this is already mostly addressed in the follow up pr that implements hosts, where we do add did normalization primitives. we can pull the relevant changes down to this pr and ensure the capability context is normalized. 2. see above, we can address this in this pr. 3. the crash was almost certainly caused by raising in a thread-interrupt, which is a very dangerous operation that can leave the interpreter in corrupted state. i have very little doubt that this was the cause of the crash, but we can rerun the full test suite including all stdlib tests before merge. 4. the best we can do here is have a thread local value that marks the network thread and attempting to call close raise an exception. better than a deadlock and not much more we can do without dramatically complicating the implementation. 5. ok, we can remove it for now.
Author
Owner

Astra says:

Addressed the review findings:

  1. Strict DID validation: only valid Ed25519 did🔑z… and unpadded did🔑u… representations are accepted. Invalid alphabets, padding, trailing junk and nonzero unused bits are rejected. Added allocation-light encoding inspection; canonical DID normalization does not decode into temporary bytevectors or native keys.
  2. Consistent identity semantics: roots, delegation edges, anchors and network credential selection recognize equivalent z/u identities. Normalization uses a bounded capability-context cache for aliases. Signed token fields and serialized bytes remain unchanged. Added mixed-encoding, persistence and cache regressions.
  3. Broader validation: eight-core stdlib/tools builds passed. The previously requested supporting-library regression passed all 43 modules, including all 31 ensemble modules. The full ./build.sh test std/... run completed across 142 modules without a native crash. It reported 14 failures in the recently introduced HTTP compliance tests, explicitly outside this PR’s scope; the other 141 modules had no harness-recorded failures. HTTP source/tests were left unchanged. The historical native-crash cause remains unproved.
  4. Safe close restriction: Network.close now raises ContractViolation from network-owned workers before locking, changing state or joining, rather than risking deadlock. Workers use actor-thread-local markers. Their failures are debug-logged and wrapped internally; joins preserve the original exception without noisy unhandled stack traces.
  5. Revocation API cleanup: removed the unimplemented revoke! export and its placeholder test. Reserved token representation remains, with documentation explicitly stating that revocation is unsupported.
    Commits: 80f1e8f3, a53478b4, 49e7c75d, 76968e6d.
Astra says: Addressed the review findings: 1. Strict DID validation: only valid Ed25519 did:key:z… and unpadded did:key:u… representations are accepted. Invalid alphabets, padding, trailing junk and nonzero unused bits are rejected. Added allocation-light encoding inspection; canonical DID normalization does not decode into temporary bytevectors or native keys. 2. Consistent identity semantics: roots, delegation edges, anchors and network credential selection recognize equivalent z/u identities. Normalization uses a bounded capability-context cache for aliases. Signed token fields and serialized bytes remain unchanged. Added mixed-encoding, persistence and cache regressions. 3. Broader validation: eight-core stdlib/tools builds passed. The previously requested supporting-library regression passed all 43 modules, including all 31 ensemble modules. The full ./build.sh test std/... run completed across 142 modules without a native crash. It reported 14 failures in the recently introduced HTTP compliance tests, explicitly outside this PR’s scope; the other 141 modules had no harness-recorded failures. HTTP source/tests were left unchanged. The historical native-crash cause remains unproved. 4. Safe close restriction: Network.close now raises ContractViolation from network-owned workers before locking, changing state or joining, rather than risking deadlock. Workers use actor-thread-local markers. Their failures are debug-logged and wrapped internally; joins preserve the original exception without noisy unhandled stack traces. 5. Revocation API cleanup: removed the unimplemented revoke! export and its placeholder test. Reserved token representation remains, with documentation explicitly stating that revocation is unsupported. Commits: 80f1e8f3, a53478b4, 49e7c75d, 76968e6d.
Author
Owner

any progress on the review here?

any progress on the review here?
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin v0.19-std-ensemble:v0.19-std-ensemble
git switch v0.19-std-ensemble

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch v0.19-staging
git merge --no-ff v0.19-std-ensemble
git switch v0.19-std-ensemble
git rebase v0.19-staging
git switch v0.19-staging
git merge --ff-only v0.19-std-ensemble
git switch v0.19-std-ensemble
git rebase v0.19-staging
git switch v0.19-staging
git merge --no-ff v0.19-std-ensemble
git switch v0.19-staging
git merge --squash v0.19-std-ensemble
git switch v0.19-staging
git merge --ff-only v0.19-std-ensemble
git switch v0.19-staging
git merge v0.19-std-ensemble
git push origin v0.19-staging
Sign in to join this conversation.
No description provided.