Simplify the certified-transaction wire format, and test #146 against a real aggregator - #147
Conversation
Bind the leaf reference time to the signed round it was certified in, and drop the copy of it that travelled beside the proof. The service alone picks a leaf's reference time and the SMT path proves only that it committed to whatever it picked, so on its own that lets a service back-date a leaf and admit a request its deadline should have killed. Consensus signs the round's timestamp, which is that round's reference time, and a leaf cannot predate the round certifying it, so InclusionProofVerificationRule now enforces that as an upper bound. The rule reads the reference time off the proof instead of being handed it. The service records the leaf's creation time on the record and serves that same value for every proof of the leaf, so the copy the certified transactions carried could never legitimately differ from it; it cost a wire element and three consistency checks. Certified mint and transfer arrays are back to two elements and Token.VERSION goes to 2, so a token written by an older SDK is rejected by the version check rather than by a CBOR array-length error further down. The worker wire format loses its separate expiresAt element too and recovers the deadline from the transfer bytes the transaction hash commits to. Also: - Report what a partially present proof is missing rather than folding it into "not certified yet", which left the caller polling to its own deadline and blaming the timeout. Binding a transaction to a proof for an uncertified state reports INCLUSION_CERTIFICATE_MISSING again, the status retry paths branch on. - Validate expiresAt where it is accepted. Negative, zero and oversized deadlines were taken and surfaced much later as a bare CborError from inside the transaction hash. - Restore the version getters on MintTransaction, TransferTransaction and CertificationData; nine other wire types still expose theirs. Tests: - An e2e suite against a real aggregator, with the stack to run it: tests/e2e/docker brings up a BFT root node, mongodb, redis and a pinned aggregator build, driven by scripts/e2e-aggregator.sh. It is not wired into CI. Nothing but a real service can tell whether the SDK and the aggregator still agree on these formats: with the leaf derivation reverted to its pre-change form, the fake-aggregator suite passes whole and the e2e suite fails. - The transition flow now runs under both an explicit and a service-assigned deadline, so the explicit path is exercised end to end rather than only constructed. - The fake aggregator models the service-assigned deadline and SERVICE_NOT_READY, the branches every caller in the repo actually uses. - Fixture certificates now certify a round whose clock matches the leaf, instead of pairing a leaf with a round timestamp of zero.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 242d7a1fe4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // this is a signed upper bound available for free. The tree is append-only, | ||
| // so a proof re-fetched later is certified by a later round and the bound | ||
| // only loosens. | ||
| if (referenceTime > inclusionProof.unicityCertificate.inputRecord.timestamp) { |
There was a problem hiding this comment.
When a malicious or misbehaving service receives a request after deadline T, it can commit a new leaf with referenceTime = T - 1; both the expiration check and this comparison pass because the current certifying round's timestamp is later than T - 1. The SMT path authenticates the service-chosen value but not when the leaf was created, so requiring the value to be no later than a current or later proof's round does not prevent the backdating attack described by this change. Verification needs signed evidence of the leaf's creation-round time (or an equivalent admission guarantee), rather than this one-sided upper bound.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and fixed in 3fb2a4b. You're right: the bound is one-sided and the useful direction is the other one.
I verified it concretely rather than reasoning about it — deadline 1000, leaf back-dated to 999, certifying round clock 5000:
deadline=1000 backdated_leaf=999 round_clock=5000 -> OK
The expiry check passes because 999 < 1000, this bound passes because 999 < 5000, and the SMT path authenticates the value the service chose rather than the moment it chose it. The attack survives exactly as you describe.
The origin of the error is worth recording: the review of #146 that prompted this PR proposed referenceTime <= inputRecord.timestamp as the fix for back-dating, and I implemented it while repeating that claim in the code comment, the commit message and the PR description. All three are now corrected.
I kept the check, because a leaf postdating the round that certified it is an impossible pairing worth rejecting and the bound costs nothing — it already caught a test fixture pairing a leaf with a round timestamp of zero. Only what it claims has changed. There is now a test pinning the back-dating gap so it stays visible and fails loudly if the protocol ever closes it, and README.md states that expiresAt is enforced by an honest service at admission and is not something a verifier can prove after the fact.
On your last point — "verification needs signed evidence of the leaf's creation-round time (or an equivalent admission guarantee)" — I agree, and that is an aggregator-side protocol change rather than something this SDK can do on its own: the proof would have to carry the certificate of the round that created the leaf, or bind a round number verifiable against the certificate chain. Worth raising against aggregator-go separately; this PR does not attempt it.
The bound added in the previous commit was described as preventing a service from back-dating a leaf to slip a request past its deadline. It does not, and Codex was right to flag it: it bounds the reference time from above, and the attack needs a bound from below. A service that receives a request after its deadline T can insert the leaf now and write referenceTime = T - 1 into it. The expiry check passes because T - 1 is below the deadline, the new bound passes because the certifying round's timestamp is later still, and the SMT path authenticates the value the service chose rather than the moment it chose it. Verified: with a deadline of 1000, a leaf back-dated to 999 and a round clock of 5000, the rule returns OK. The check stays, because a leaf postdating the round that certified it is an impossible pairing worth rejecting and the bound is free — it already caught a fixture that paired a leaf with a round timestamp of zero. Only the claim about it changes. - Rewrite the rule's comment to say what it does establish, and to state plainly that it does not bound back-dating and why. - Add a test pinning the gap, so it stays visible in the codebase and fails loudly if the protocol ever closes it. - Correct the same claim in the unit and e2e test comments. - Document in README.md that expiresAt is enforced by an honest service at admission and is not something a verifier can prove after the fact. Closing this properly needs signed evidence of the creation round, which an inclusion proof does not carry — an aggregator-side protocol change, not something this SDK can do on its own.
MastaP
changed the title
These tests are not end-to-end. They bring up their own aggregator in docker and talk to nothing outside it, which is what an integration suite is; e2e here means the deployed testnet, and that suite already exists and stays as it is. - tests/e2e/docker -> tests/integration/docker, and the request-deadline suite with it. scripts/e2e-aggregator.sh -> integration-aggregator.sh, and the npm scripts follow: integration:up, integration:down, test:integration. test:ci ignores the new directory. - The integration suite finds its own stack: it defaults to localhost and to the genesis the root node generates on first start, so it needs no environment setup, and it says so rather than failing obscurely when the stack was never started. The e2e suite keeps taking an endpoint and a trust base, since it has no service of its own. - Run the transition flow against the local stack too, under both deadline modes, so the flow is covered without a deployed network. Aggregator image moves to sha-ae08165, which adds request/state-ID, certified-shard and network bindings to its own reference verifier. The CBOR wire format is unchanged, and the SDK already had all three: the network binding at UnicityCertificateVerifier.ts:72, the shard binding in ShardIdMatchesStateIdRule, and the state-ID binding by deriving it from the certification data. Nothing to add; 11/11 pass against it. The image also surfaced a cold-start race: the aggregator gives up creating its indexes while the fresh replica set is still electing, then restarts into a working state, which `compose up --wait` reports as a failure. Its mongo timeouts are raised to ride out the election, and `up` treats certification, not `--wait`, as the gate.
The stack was started by a shell script and stopped by another, which works on a laptop and not much else: the port was fixed, so two runs collided, and a failed run left containers behind. Testcontainers starts it from the same compose file, publishes the aggregator on an ephemeral port, and tears it down when the run ends. npm run test:integration is now the whole thing — no environment, no separate up and down. It costs a cold start of about a minute, so the script stays for the case it was actually good at: start the stack once, point the suite at it with `eval "$(./scripts/integration-aggregator.sh env)"`, and iterate at ~20s a run. A stack the run did not start is also one it does not stop. Waiting for the container to be healthy is not enough to run against: until consensus hands the aggregator a reference time it answers every certification request with SERVICE_NOT_READY, so setup blocks on a certified block rather than on the healthcheck. The support files are .mjs because the babel transform would otherwise rewrite globalSetup to CJS and then fail to require its own ESM imports; that also puts them outside the TS project, so they join tests/utils/*.mjs in the lint ignores. CI gets an integration job. It has no external dependency now — the ephemeral port means concurrent jobs cannot collide, and nothing is left running for the next one to trip over. The e2e suite stays out, since it needs a deployed network to be pointed at.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 683d32c6c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| integration: | ||
| name: Integration against a real aggregator |
There was a problem hiding this comment.
When the real-aggregator integration suite fails but the unit/build job succeeds, this sibling job does not prevent the build job's main-branch or release steps from publishing to npm; GitHub Actions runs jobs without needs independently, so an incompatible package can be published before the workflow ultimately turns red. Make publication depend on successful completion of this integration job, or move the publish steps into a downstream job.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 50e0c16. You're right, and this one is on me for making it worse: before this PR there was no integration job at all, so adding one that looks like a gate without being one is arguably worse than not having it — a reviewer reads the workflow and assumes releases are covered.
Publishing now lives in its own job:
publish:
needs: [build, integration]
if: github.event_name == 'release' || (github.event_name == 'push' && github.ref == 'refs/heads/main')needs is the gate; the event conditions only choose which kind of release it is. I took the second of your two options — a downstream job rather than needs: integration on build — so that PR feedback stays parallel: making build wait on integration would serialise every PR for a constraint that only matters on main and on release.
Two consequences of the split worth noting for review:
id-token: writeis only needed to publish, sobuildandintegrationno longer request it.- The publish job re-runs
npm ciandnpm run build, since jobs don't share a filesystem andfilesin package.json shipslib/. That's ~45s of duplicated work, which seemed the right trade against publishing an unbuilt package.
One thing I checked while making the change: I had initially renamed the build job to "Build and test", which would have changed the reported check name. There is no branch protection on this repo today, so nothing would have broken — but a future required check configured as build would have silently never matched. Reverted; the job keeps its bare name, and only the job body changed.
The publish steps lived in the `build` job, and `integration` was a sibling with no dependency between them. GitHub runs jobs without `needs` independently, so on a push to main `build` published as soon as its own steps went green — whether or not the integration suite had finished, or passed. Adding that job made the workflow look like it gated releases while it did not, and an npm publish cannot be taken back. Publishing moves into its own job with `needs: [build, integration]`. The event conditions stay where they were and now only choose which kind of release this is; `needs` is what gates it. Two things fall out of the split. `id-token: write` is needed only to publish, so the two test jobs no longer request it. And the publish job re-runs `npm ci` and `npm run build`, because jobs do not share a filesystem and `files` in package.json ships `lib/`. The `build` job keeps its bare name, so the check name PRs have always reported does not change under anyone's branch protection. Reported by Codex on #147.
The formats this branch changes are shared with the Unicity Service, and they move in both directions at once: a token minted by 2.x no longer loads, and a 3.0 client and a 2.x-era aggregator cannot verify each other's proofs. Token, MintTransaction, TransferTransaction and CertificationData all go to wire version 2, and the certified leaf value becomes SHA-256(CBOR([transactionHash, referenceTime])) rather than the transaction hash alone. Under semver that is a major, and the package is published with real consumers on 2.1.0, so `^2.1.0` must not resolve to it. README gains an "Upgrading to 3.0" section covering what stops working — existing tokens, aggregator compatibility, and the compile-time breaks for anyone building on the verification internals — and a section on request deadlines, which had no user-facing documentation at all despite being the feature this release exists for. The protocol spec gains a section on deadlines in its own conceptual register: what a round's reference time is, why admission is exclusive, how an explicit deadline differs from a service-assigned one, and what a deadline does not guarantee against a dishonest service. Note the rest of that spec is stale independently of this change — it still describes Request IDs and Authenticators, which the code has not called that since 2.0. Refreshing it is its own piece of work.
The option interfaces a caller writes to said "Unix seconds", but the getters a caller reads did not, and neither did the verification rule, the predicate verifier, the leaf value helper or the protocol spec. Both values are wall-clock instants, and both could plausibly have been round numbers or block heights to someone reading only the types — they are bigints named after time, sitting next to an InputRecord that really does carry a round number. Units added to the expiresAt getters on CertifiedMintTransaction, CertifiedTransferTransaction and TransferTransaction, and to the six places reference time is documented. The comparison itself now records something that was written down nowhere: both sides are consensus time, not the caller's. A round's reference time is the timestamp of the BFT seal, so a deadline derived from a local clock is compared against the root chain's, and the two differ by seconds. README says the same and adds the practical consequence — leave margin for skew and queue time; hour-scale deadlines do not care, second-scale ones do. The spec section says it too, and now states that neither value is a round number or block height.
The three support files are the Testcontainers integration, not leftovers from before it: aggregatorStack.mjs is what constructs the DockerComposeEnvironment, and globalSetup/globalTeardown are the Jest hooks that start and stop it. What was left over was the shell path beside them. scripts/integration-aggregator.sh and the integration:up and integration:down scripts are gone, and with them the branch that reused a stack the run did not start. That branch existed to make the script's workflow fast, and without the script its only remaining effect would be to let a stray AGGREGATOR_URL silently redirect the suite — a green run that proved nothing about the compose file it exists to exercise. IntegrationConfig follows: it now requires the endpoint and trust base the setup hook publishes rather than defaulting to localhost and a guessed path, and says how to run the suite when they are missing. Pointing the SDK at a service someone else runs is what the e2e suite is for, and that distinction is now enforced rather than merely described. Verified from cold with nothing pre-started: 11/11, no containers and no genesis directory left behind.
1 participant
Follow-up to #146, targeting
service-timeso it can be folded in before that PR merges.Two things prompted this: a review of #146 cross-checked against the aggregator source at
0c7f70b0ccbc, and the observation that a change to four wire formats plus a new leaf derivation had no coverage against a real aggregator. The only aggregator #146 exercises is the in-repo fake, which derives leaf values with the very code under test.BREAKING CHANGES — releases as 3.0.0
package.jsonmoves 2.1.0 → 3.0.0. The formats this branch changes are shared with the Unicity Service and move in both directions at once, so^2.1.0must not resolve to it.Existing tokens stop loading
Token.VERSIONis now 2.Token.fromCBORrejects a token minted by 2.x withUnsupported Token version: 1. There is no migration path — affected tokens have to be re-minted. Four of the five wire versions move:TokenMintTransactionTransferTransactionCertificationDataInclusionProofAggregator compatibility
A 3.0 client requires
ghcr.io/unicitynetwork/aggregator-go:sha-ae08165or later. The certified leaf value is nowSHA-256(CBOR([transactionHash, referenceTime]))rather than the transaction hash alone, so a 3.0 client cannot verify proofs from a 2.x-era service, and a 2.x client cannot verify proofs from a current one.Compile-time breaks
InclusionProofVerificationRule.verifydrops itsreferenceTimeparameterInclusionProofVerificationStatus.REFERENCE_TIME_MISMATCHremovedREFERENCE_TIME_AFTER_ROUNDandINCOMPLETE_INCLUSION_PROOF; a downstreamswitchbreaksexpiresAtrecovered from the transfer bytes it is committed toexpiresAtvalidated at the factoriescreateinstead of failing later inside CBOR encodingAdditive
expiresAtonMintTransaction.create,TransferTransaction.createandTokenSplit.split;TransferTransaction.expiresAtFromCBOR;validateExpiresAt.versiongetters restored onMintTransaction,TransferTransactionandCertificationData— #146 had removed them while nine other wire types kept theirs.Documentation
README gains an "Upgrading to 3.0" section and a "Request deadlines" section — the feature this release exists for had no user-facing documentation.
unicity-token-protocol-spec.mdgains §2.7 on deadlines. Note the rest of that spec is stale independently of this branch: it still describes "Request IDs" and "Authenticators", which the code has not called that since 2.0. Refreshing it is its own piece of work and is not attempted here.Request deadlines: what verification can and cannot establish
This section was wrong in the first version of this PR, and is corrected here. The original review of #146 proposed a signed upper bound on the leaf reference time as a fix for back-dating, and I implemented it repeating that claim. Codex flagged it as a P1 and was right.
InclusionProofVerificationRulenow rejects a leaf claiming to postdate the round that certified it (REFERENCE_TIME_AFTER_ROUND). Consensus signs the round timestamp, which is that round's own reference time, so the bound is free and exact:buildCertificationInputRecordin the aggregator setsInputRecord.Timestamp = referenceTime, and against a live service the two are equal on first fetch and diverge only as later rounds certify.It does not stop back-dating, which is the direction that matters. A service that receives a request after its deadline
Tcan insert the leaf now and writereferenceTime = T - 1into it: the expiry check passes becauseT - 1 < T, the new bound passes because the certifying round's timestamp is later still, and the SMT path authenticates the value the service chose rather than the moment it chose it. Verified concretely — deadline1000, leaf back-dated to999, round clock5000→ the rule returnsOK. There is a test pinning exactly this, so the gap stays visible and fails loudly if it is ever closed.The check stays because a leaf postdating its own round is an impossible pairing worth rejecting, and it already caught a fixture that paired a leaf with a round timestamp of zero. Only the claim about it changed.
Closing the real gap needs signed evidence of the creation round, which an inclusion proof does not carry — an aggregator-side protocol change, not something this SDK can do alone. In the meantime
expiresAtis an instruction to an honest service (a late request is dropped rather than executed), not something a verifier can prove after the fact; that is now stated in the README, and it is worth deciding at protocol level whether it should be more.Dropping the duplicated reference time
The rule now reads the reference time off the proof instead of being handed it separately. The aggregator records the leaf's creation time on the record and serves that same value for every proof of that leaf, so the copy the certified transactions carried could never legitimately differ — it cost a wire element and three consistency checks that could only ever agree.
Token.VERSION1 → 2, since the element counts underneath it changed. Without it a token written by an older SDK passes the version check and then dies on a CBOR array-length error that never mentions versioning.expiresAtelement and recovers the deadline from the transfer bytes the transaction hash commits to, via a newTransferTransaction.expiresAtFromCBOR. The old copy travelled outside those bytes and was never cross-checked against them.InclusionProofVerificationStatus.REFERENCE_TIME_MISMATCHis gone — there is no caller-supplied value left to disagree with the proof.Other fixes from the review
INCOMPLETE_INCLUSION_PROOF.INCLUSION_CERTIFICATE_MISSINGagain. A guard in both certified-transaction factories was intercepting it and reporting a missing reference time, which no retry path recognises.expiresAtwas unvalidated at the factory boundary:-1nand2n ** 70nwere accepted and only failed later inside the transaction hash with a bareCborError, and0nwas accepted outright and produced a request expired by construction. A sharedvalidateExpiresAtnow runs inMintTransaction.create,TransferTransaction.createandTokenSplit.split. Its upper bound matches the encoder exactly —2^64-1encodes,2^64throws.MintTransaction,TransferTransactionandCertificationData. Nine other wire types still expose theirs; these three lost theirs in the same change that altered their shape.Integration suite against a real aggregator
Testcontainers starts the stack in
tests/integration/docker— a BFT root node, mongodb, redis and a pinnedaggregator-gobuild (sha-ae08165) — waits for consensus to certify a round, and tears it down when the run ends:No environment, no separate up/down. The aggregator is published on an ephemeral port, so concurrent runs and CI jobs cannot collide, and a failed run does not leave containers behind.
That costs a cold start of about a minute.
scripts/integration-aggregator.shstays for iteration: start the stack once,eval "$(./scripts/integration-aggregator.sh env)", and runs drop to ~20s. A stack the run did not start is one it does not stop.Waiting for the container healthcheck is not sufficient to run against — until consensus hands the aggregator a reference time it answers every certification request with
SERVICE_NOT_READY— so setup blocks on a certified block instead.These are integration tests, not e2e: they own the service they talk to and depend on nothing outside docker.
tests/e2ekeeps its existing meaning, a suite pointed at a deployed network, and is unchanged apart from the transition-flow helper it shares. The integration suite runs in CI (newintegrationjob); the e2e suite does not, since it needs a live network to be pointed at.This is where the wire formats get checked. Certification data, the transaction encodings, the inclusion proof and the reference-time-bound leaf value are all shared with the service, and the fake aggregator in
tests/functionalderives them with the very code under test.Why that matters, concretely: revert
calculateLeafValueto its pre-#146 form — a change that breaks every real submission — and the fake-aggregator suite passes 25/25 while the integration suite fails.tests/integration/RequestDeadlineTest.tscovers the deadline semantics on real data: explicit deadline through mint → transfer → verify,REQUEST_EXPIREDfor a past deadline and for one equal to an observed reference time (exclusivity), the service-assigned branch binding a deadline without recording it, the signed round bound in both directions, and the reference time staying pinned while the certifying round moves on. The transition flow runs against the stack too, under both deadline modes.Three upstream compose bugs had to be worked around: the
x-bftanchor materialises a stray container, the mongo healthcheck reports ready before a primary is elected, and the image's own healthcheck spiders/healthwith HEAD against a GET-only route so it never passes.On the aggregator image
Pinned to
sha-ae08165(2026-08-26), up fromsha-0c7f70b. The one commit between them — unicitynetwork/aggregator-go#182 — adds request/state-ID, certified-shard and network bindings to the Go reference verifier and returnsreferenceTimeon block records. The CBOR wire format is unchanged, and the SDK already had all three bindings: network atUnicityCertificateVerifier.ts:72, shard inShardIdMatchesStateIdRule, and the state-ID binding by deriving it from the certification data rather than trusting the requested key. Nothing to add; 11/11 pass against it.Its
docs/inclusion-proof-wire.mdalso states "Do not recover τ fromUC.IR.t. Use thereferenceTimeelement. They coincide only for the proof issued in the leaf's own round" — which is exactly the relation this PR relies on.Other test changes
expiresAthelper parameter added in Bind service time and enforce request timeouts #146 had no caller, so the explicit path was constructed but never exercised end to end.referenceTime + RequestTTL) andSERVICE_NOT_READY. The null-deadline branch is what every caller in this repo actually uses, and it was tested against a model that could not reject it.UnicityCertificateFixturedefaulted its round timestamp to0n, pairing leaves with rounds that could not have certified them — an impossible combination the rule now rejects. It defaults to the leaf's reference time.CertificationDataTest'sexpect(hex).toContain('f6')passed regardless of how the absent deadline was encoded; it decodes the actual element now.CertifiedTransactionWireTest(element counts, version rejection, pending-status regression),unit/transaction/ExpiresAtTest, partial-proof cases inInclusionProofUtilsTest, worker payload shape, split deadline validation.Verification
build,build:checkandlintclean. 192/192 unit + functional. 11/11 integration onsha-ae08165— verified both ways, Testcontainers starting the stack from cold (~84s, nothing left behind afterwards) and reusing an externally started one (~22s, left running). 7/7 e2e when pointed at that same stack.Negative controls: removing the new round-timestamp bound fails both the unit and the integration test for it; reverting the leaf derivation fails the integration suite while the functional suite passes whole; dropping
expiresAtfrom the worker payload fails exactly the one worker test that covers it.