perf(import): defer the heavy leaves off the eager import graph (#120) - #245
Open
Rinse12 wants to merge 12 commits into
Open
perf(import): defer the heavy leaves off the eager import graph (#120)#245Rinse12 wants to merge 12 commits into
Rinse12 wants to merge 12 commits into
Conversation
A LocalCommunity can now be the minter of a delegated community: it signs with
its own key while its identity is the anchor supplied at creation. A node never
resolves itself, so it cannot derive that identity — the anchor is persisted in
the internal community record and replayed into ipnsHops as [An, Mn] on load,
which makes the inherited identity code report the anchor as publicKey.
createCommunity({ anchor: { publicKey } }) is the new key regime, mutually
exclusive with signer: which key the caller supplies discriminates between the
node holding the identity key and the owner holding it.
Moves to the anchor: publicKey/address, the data directory and MFS namespace,
publication acceptance (a publisher addresses the community by the name it
resolved) and the communityPublicKey stored on content, so stored publications
survive a minter rotation. The domain checks move too: a domain's TXT record
points at the name readers resolve, so comparing it against signer.address would
make a delegated community reject its own correctly configured domain.
Stays with the minter: the record signature, encryption, signer.ipnsKeyName, the
pubsubTopic backfill, and this node's own ipnsName/ipnsPubsubTopic/routing CID.
The inherited _updateIpnsPubsubPropsIfNeeded prefers ipnsHops[0], correct for a
reader and wrong for a publisher, so LocalCommunity overrides it once instead of
repeating the minter-based fixup at each of the three init call sites.
A non-delegated community is the degenerate case: no anchor, no ipnsHops, and
every rule above collapses to signer.address.
An owner can now hand a community to a node, keep the anchor key, and have the node keep the binding alive. The one logical operation is split across the trust boundary: the client signs, the node publishes and re-provides. createAnchorIpnsRecord lives in src/signer/ because a browser holding As is the primary case and anything node-only would break the browser build. It signs An -> Mn with one shared far-future lifetime constant, so an anchor's liveness never depends on which call site signed it. prepareAnchorPublish answers max(highest accepted here, live lookup of /ipns/An) plus a margin. No source can lie upward and skipped sequences are free, so the only real failure is lying downward, which makes the owner sign a record that loses to its own predecessor forever. It therefore errors rather than answering 0 when neither the node nor the network knows a sequence: kubo reports "never existed" and "the routers failed us" identically, and only the client knows whether its anchor is brand new. A first publish signs 0, which is what an absent anchorRecordSequence on the create response tells it. publishAnchorRecord verifies the record is signed by this community's anchor, points at this node's own minter, and carries a strictly greater sequence, then puts it through routing.put. name.publish structurally cannot publish bytes signed by a key the node lacks. Anti-rollback is enforced here and not delegated to kubo, which returns success for a put it silently discards, and the high-water mark lives in its own storage slot rather than sharing LAST_IPNS_RECORD, which holds this node's own minter record. The put also subscribes the node to the anchor's ipns-over-pubsub topic, which is what serves the binding to peers, and that subscription does not survive a kubo restart while the record does. So the re-provide runs at start() as well as on the publish loop, otherwise a restarted node holds the record and quietly stops answering for it. A delegated community refuses to start until its anchor record is published: until then nothing points the anchor at this minter, so starting would only mint records nobody can find. Half-created, not broken. Both methods mirror as RPC methods beside startCommunity/stopCommunity and forward through RpcLocalCommunity, so the in-process self-hosted case is not stranded. Sequences cross the wire as decimal strings and the record as base64, since JSON has neither uint64s nor bytes.
…gated communities (#233, #234) Fills the cases that were left as it.todo because they need the anchor record on the network, which publishAnchorRecord now puts there. Read-back closes the round trip: the chain a reader walks is produced by a pkc-js node rather than by a hand-built fixture, so what we publish is what the resolver expects. A no-dataPath instance resolves the anchor, reports it as the identity, and verifies content signed by the minter. Rotation creates the community again on a second node with its own dataPath, signs An -> Mn' at the sequence that node learns from the network, and publishes. The address does not move, previously stored content still names the anchor with no rewrite, and a reader that re-resolves picks up the new minter and its new pubsub topic. Two things the rotation case surfaced, both pre-existing and neither fixed here: - The started-community registry is process-wide and keyed by publicKey alone, so two instances sharing an address but not a dataPath collide: the second loads the first's state and then fails its own signature validation. The old minter therefore stands down before the new one is created, which is what a real rotation does anyway. - A minter whose name has never resolved stalls its first publish for a minute inside resolveIpnsAndLogIfPotentialProblematicSequence, a diagnostic-only resolve carrying a 120s timeout. The start is kept out of the hook so it is not charged against the hook budget. Export and import cover portability: the export record is keyed by the anchor, so backups are not orphaned by a rotation, and restoring the db into a fresh dataPath brings back the identity with the anchor's private key nowhere in it.
, #234) Anti-rollback TOCTOU: publishAnchorRecord read the high-water mark, then awaited the kubo put and two keyvSets before persisting it, so two concurrent publishes both cleared the check and last-write-wins could persist the lower sequence. A record in between then passed the check, was silently dropped by kubo (which answers 200 to a rollback put) and reported as published. Publishes are now serialized per community address, in-process. anchorRecordSequence was cleared by every internal-record replay, since that record deliberately omits it, leaving start() reporting no sequence for a community whose anchor record is published. Re-derived from the keyv slot that owns it, on the DB path and on the mirror path. Also: - reject anchor passed with address/name/publicKey, which was silently ignored - validate anchor.publicKey as an IPNS name at creation rather than at publish - give the RPC anchor methods their own error code instead of the edit one - accept decimal-string sequences in createAnchorIpnsRecord, matching the rest of the feature's precision convention - split the docs setup example into first-publish and rotation paths; as written it threw ERR_UNABLE_TO_DETERMINE_ANCHOR_SEQUENCE on a first publish - explain why the kubo spike suite cannot run under RPC
…ssertion server-sourced (#234) The new error code had no coverage: RpcLocalCommunity only ever forwards a community the server hosts, but a client can send the params directly, which is what a third-party client would do. Asks the RPC client for a random signer's address and asserts both anchor methods reject with the anchor-method error rather than the edit one. The anchorRecordSequence assertion added alongside it was weaker than it looked. publishAnchorRecord sets the value client-side from its own result, so asserting straight after start() passed on that local copy even against a server that had cleared its own. It now waits for an update from the server first, so the value asserted is the one that came over the wire.
) The anchor record is signed with As, the key that by design never reaches the node, so this is the one step of delegation setup that has to run in the consumer's process. Nothing under src/ calls it and nothing can, and package.json's exports map has no subpath a consumer could deep-import src/signer through, so the primitive was unreachable outside this repo and the setup example in the docs could not be run by anyone following it. Exported the same way as getShortCid: a static on PKC plus a named export, on both the plain entry and the compile-cache entry, since the latter is what the "." -> import condition resolves to for Node ESM consumers. Docs and README carry the import line and now say explicitly that three of the four steps are LocalCommunity methods while the signing step deliberately is not.
) Five cases that had no coverage: - Creating with an anchor where no local community can exist (browser, or node with no dataPath) throws rather than falling through to a RemoteCommunity that reads as success. The signer path shares the guard and was untested too. - The publisher-side domain checks on a delegated community. A domain's TXT record points at the anchor, so comparing it against signer.address would have made a correctly configured delegated community reject its own domain on every start and every edit. Both directions asserted. The rejection surfaces on the error event rather than as a rejected edit(), which is pre-existing fire-and-forget behaviour in validateNewAddressBeforeEditing, so the test asserts what an owner actually observes. - The publish loop's throttled re-provide. Only the start() path was covered, and an inverted interval check would either put every tick or never put again while the node still looks healthy. - A sequence above Number.MAX_SAFE_INTEGER. The decimal-string plumbing exists only for this, and a Number anywhere on the path rounds silently. - createAnchorIpnsRecord reachable from the package entry, which is the only way a consumer can call it: every other test reaches it by deep import, so dropping the re-export would break consumers with the suite still green.
) Adds the cases the delegated suites did not reach: - identity after the FIRST update, on both an RPC client and a db reload. That record carries signature.publicKey (the minter), so the anchor has to be replayed into ipnsHops or the community silently becomes addressed by the key that merely signs for it. The existing reloads all ran before a record existed. - a second instance of a started delegated community, covering both the copy at create and the mirrored update. Every other reload targets a stopped community and takes the db branch instead. - the anchor record's validity horizon, which every other assertion signs and validates inside the same second, plus the negative-sequence and empty-name guards of createAnchorIpnsRecord and the two shared RPC param rejections. - prepareAnchorPublish when the network knows a higher sequence than this node, which is the case its max() over the two sources exists for. - the exported db carrying the anchor record and its high-water mark, without which a restored backup refuses to start. - a delegated community publishing a record that carries the post it accepted, and a reader loading that post with its CommentUpdate through the chain. The last two fail on this commit. They reproduce a real defect, fixed next.
#233, #234) verifyCommunity took a single communityIpnsName and used the record's own signature key as the identity that comments inside its pages must belong to. On a delegated community those are two different keys: content is labelled with the anchor, while the record is signed by the minter. Every comment in the community's own record therefore failed the page check with ERR_COMMENT_IN_PAGE_BELONG_TO_DIFFERENT_COMMUNITY. A publisher runs that same verification on itself before publishing, so a delegated community could publish only its initial empty record and then failed every sync with ERR_LOCAL_COMMUNITY_PRODUCED_INVALID_SIGNATURE. No postUpdates, no pages, and no way for any reader to ever fetch a CommentUpdate. Readers are affected too: the same path runs on them whenever validatePages is on. verifyCommunity now takes communityIpnsHops: string[], the [anchor, ..., terminal] chain RemoteCommunity.ipnsHops already holds, because the two ends verify different things and a record cannot tell them apart on its own. The terminal (last hop) is checked against signature.publicKey exactly as before, and the anchor (first hop) is what page comments are verified against. A non-delegated community passes a single-element chain, where both are the same key, so its behaviour is unchanged. The verification cache key now covers the whole chain rather than one name. _findErrorInCommunityRecord takes the chain as well and derives the anchor and terminal itself, replacing the two separate params it used to take. Both call sites already held the resolved hops. BREAKING: verifyCommunity ships from the package entry, and its communityIpnsName param is replaced by communityIpnsHops.
#234) The export/import block opens a db connection on a community it never starts, so pkc.destroy() - which only tears down started/updating communities - leaves the sqlite file open. Windows then fails the afterAll rmSync with EBUSY, which failed the whole suite on windows-latest while every assertion passed.
A CPU profile of `import("@pkcprotocol/pkc-js")` taken on the production
host showed the eager static closure was still 647 modules, and that most
of the non-Node-internal time was dependency subtrees an RPC-only consumer
can never reach. Each now sits behind a dynamic import at the call site
that needs it:
- the link-preview scraping closure (~350 modules: open-graph-scraper,
probe-image-size, hpagent and their cheerio/parse5/iconv-lite/chardet
transitives), only reachable from getThumbnailPropsOfLink()
- undici (~112 modules), imported at the top of index.ts purely to raise
the global fetch body timeout. It is now applyUndiciPolyfills(), awaited
by createKuboRpcClient() and by the node nativeFunctions.fetch wrapper,
so the dispatcher is still installed before any body-streaming fetch.
An awaited call rather than a floating import(): a floating promise
would race the first fetch, and index.ts cannot use top-level await
because the `require` export condition rejects TLA.
- node-forge (~42 modules), only used when a challenge message is
encrypted or decrypted; every caller was already async
- node:http / node:https, at module scope for one Agent construction
inside createKuboRpcClient(); they also pull _http_agent and Node's
builtin undici
- the kubo address-rewriter setup, statically imported by pkc.ts but
already guarded at its call site, matching the existing helia and
LocalCommunity boundaries
Eager static closure drops 647 -> 225 inputs. Reference host: bundled
index 205ms -> 120ms cold, warm-compile-cache npm entry 155ms -> 100ms.
Production host: package import ~2.0s -> ~1.4s (-30%), and end to end
`bitsocial community list` ~5.0s -> ~4.3s.
verify-bundle gains a gate (check 1a) asserting none of these deps, and
neither node:http nor node:https, re-enter the static closure of
dist/bundled/index.js, since one stray static import silently costs every
consumer process hundreds of milliseconds again. Verified the gate fails
on an injected regression.
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (39)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Continues #120. Profiled
import("@pkcprotocol/pkc-js")on the production host (the slow one that actually hurts), not the fast dev machine, and found the eager static closure was still 647 modules -- mostly dependency subtrees an RPC-only consumer like the bitsocial CLI can never reach.What moved behind a dynamic import
open-graph-scraper,probe-image-size,hpagent(+ cheerio / parse5 / iconv-lite / chardet / needle)getThumbnailPropsOfLink(), i.e. publishing a comment with alinkundicinode-forgenode:http/node:https_http_agent+ Node's builtin undiciAgentconstruction increateKuboRpcClient()The undici one is the only subtle change. It becomes
applyUndiciPolyfills(), awaited bycreateKuboRpcClient()and by the nodenativeFunctions.fetchwrapper (the single choke point for gateway/content fetches), so the dispatcher is still installed before any body-streaming fetch. It has to be an explicit awaited call rather than a floatingimport(): a floating promise would race the first fetch, andindex.tscannot use top-level await because therequireexport condition points at that graph andrequire(esm)rejects TLA.Results
Eager static closure 647 -> 225 inputs.
import("@pkcprotocol/pkc-js")bitsocial community listend to endRegression gate
config/verify-bundle.jsgains check 1a: none of these deps, and neithernode:httpnornode:https, may re-enter the static closure ofdist/bundled/index.js. One stray static import silently costs every consumer process hundreds of milliseconds otherwise. Verified the gate fails on an injected regression (and passes clean).Testing
test/node/pkc-- 161 passed, 5 skippedtest/node/publications+src/rpc/test/node(server, listeners, ws-server) -- 133 passedtest/node/community/start.community,kubo-address-rewriter.unit,address-rewriter-logging.unit,httprouter,test/node/clients-- 41 passedtest/node-and-browser/encryption.signer-- 12 passed (covers the deferred node-forge)describe.skip(they hit live sites), so the deferred scraper path was smoke-tested end to end against a local http server serving an og:image page plus a PNG: both the og-page and direct-image routes returned the right url and 8x8 dimensions.test/node/community/create.community.test.tshas one failure, "Can create a community if it's running in another PKC instance" (community.updatedAtundefined). It reproduces identically on unmodifiedsrc/at the same commit, so it is pre-existing and unrelated.Where the remaining production time goes
Documented in
docs/protocol/import-performance.md. Of the ~4.3s: ~1.4s pkc-js import (largest single slice now our own zod schema construction at ~14%, then@libp2p/peer-id/@libp2p/crypto/keyspulling@noble/curves+js-sha3; peer-id is used inside synchronous zod validators so deferring it is a refactor, not a one-line move), ~0.9s the CLI's own uncached graph + oclif, and ~2s RPC work. That last part is the piece that scales with community count --community listdoes onecreateCommunityround trip per community, and-qskips them and lands at ~2.5s -- so for 20+ communities the next lever is CLI/daemon side, not import side.