v0.19: ensembles: network #1493
Labels
No labels
UX
active development
backlog
blocker
bootstrap
bounty
bug
dependencies
discussion
documentation
duplicate
enhancement
flaky test
help wanted
invalid
javascript
question
release
tendentious
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
mighty-gerbils/gerbil!1493
Loading…
Reference in a new issue
No description provided.
Delete branch "v0.19-std-ensemble"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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 🦖.
837e16223atodeb9873961WIP: v0.19: ensemblesto v0.19: ensembles: networkdeb9873961tod6d13b539frebased on staging.
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)
P1 —
did->public-keydeliberately accepts invaliddid:keyidentifiers. The new tests explicitly require acceptance ofU,m, andMmultibase forms, plus strings containing!,==ignored, and even a trailing newline. The documentation likewise says this permissiveness is intentional. (cons.io git) But the currentdid:keymethod ABNF permits exactly base58btcz...or unpadded base64urlu...; 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-keyto thezanduencodings actually permitted bydid: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 (...LBdecoding like...LA); otherwise there are still multipleuspellings for one byte sequence.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 itsdid:key:u...form. Signed token fields are intentionally left untouched. (cons.io git) MeanwhileCapabilityContext.add-root!deliberately stores the root string verbatim, andtoken-rooted-at?tests the issuer with plainequal?. (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...")→ succeedsnetwork authenticates that same key as canonical
did:key:u...→ token signature verifiestoken-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
zanduare validdid:keyencodings.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
d6d13b539fbefore approving.P2/design —
Network.closehas a surprisingly dangerous context restriction. The public interface explicitly warns that callingNetwork.closefrom 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 setsclosed?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.closeto detect an owned worker and request shutdown without self-dependent joining, even if synchronous completion is only guaranteed to outside callers.Minor/scope — public revocation API is knowingly a trap.
REVOKEis a public token type, tokens have revocation-specificargs,save-token!is documented as being for future revocation, andrevoke!is exported—butrevoke!unconditionally raisesTODO; 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 exportingrevoke!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
IOTimeoutis 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.some preliminary review comments, before we discuss and address with astra:
Astra says:
Addressed the review findings:
Commits:
80f1e8f3,a53478b4,49e7c75d,76968e6d.any progress on the review here?
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.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.