fix(api): bind inclusion proofs to the certified shard and network; return referenceTime on block records - #182
Conversation
…k records get_block_records dropped three fields the README documented. referenceTime is the consequential one: it is what a consumer needs to rebuild the certified leaf value, LeafValue(transactionHash, referenceTime), and api.AggregatorRecord had no field for it at all. expiresAt was dropped from the certification data, leaving no way to check the request deadline, and finalizedAt was declared on the API type but never populated by anything. - adds ReferenceTime to api.AggregatorRecord - copies Version and ExpiresAt through the conversion - populates FinalizedAt from the block, one read per page rather than per record - corrects the README example, which also showed publicKey/signature -- fields that no longer exist on CertificationData -- and hex-encoded predicate bytes that actually serialize as base64 Pins the emitted key set in a test so the README cannot drift again.
Mutating the comparison at pkg/api/types.go from >= to > left the whole pkg/api suite green: the only fully-built proof fixture leaves ExpiresAt nil, so the branch was never entered. The server-side boundary is pinned at both ends but the verifier clients actually run was not. Covers reference time below, equal to, and above the deadline, plus the presence-mismatch cases. Note buildSignedSingleLeafProof aliases the proof's certification data to the request's, so the presence tests detach the copy first -- without that the two can never disagree and the test passes vacuously. Also corrects a comment in inclusion_cert.go still describing the leaf value as the transaction hash, which the reference-time change invalidated.
pkg/api/types.go and pkg/api/inclusion_cert.go cite docs/inclusion-proof-wire.md as the frozen specification in three places, but the file did not exist -- the Go comments were the only statement of the wire shape. Documents the tagged 5-element InclusionProofV2, the fixed 6-element CertificationData with expiresAt holding its slot as CBOR null, the leaf value preimage (verified byte-for-byte: 825820 || txhash || CBOR uint), the InclusionCert and ExclusionCert binary layouts, the hash rules, and the verification order including the exclusive deadline comparison. Records two things a client integrator would otherwise get wrong: reference time must be read from the proof's referenceTime element and not recovered from UC.IR.t, which coincides only for the proof issued in the leaf's own round; and Verify does not check that sid routes to the expected shard.
The yellowpaper defines the certification request with a mandatory exclusive
timeout tau_Q, and makes 'tau < tau_Q' a step of verifying a certified
transaction. This implementation accepts CBOR null in that slot and assigns a
deadline from DEFAULT_REQUEST_TTL instead -- but that substitute is service-local:
not in the leaf, not signed, not served in the proof. A transaction certified
that way leaves a later verifier unable to perform the spec's check at all.
Rejecting the absent form outright would break existing clients, so this only
adds the visibility needed to retire it: the service_assigned series is the
migration backlog, and it reaching zero is the precondition for rejecting
requests that omit the deadline.
aggregator_certification_requests_by_deadline_total{origin}
![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: f25a0095b0
ℹ️ 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".
| ## Hash rules | ||
|
|
||
| - Leaf: `H(0x00 || key || value)` | ||
| - Inner node, two children: `H(0x01 || depth_byte || left || right)` |
There was a problem hiding this comment.
Correct the frozen wire specification to include the 32-byte key-prefix region in every binary inner-node preimage. The verifier actually computes H(0x01 || depth_byte || RegionFromKeyBytes(key, depth) || left || right) in pkg/api/inclusion_cert.go, so an independent client implementing the documented formula will derive a different root for any proof containing a sibling and reject otherwise valid proofs.
Useful? React with 👍 / 👎.
The inner-node preimage in docs/inclusion-proof-wire.md omitted the key-prefix
region. The verifier computes
H(0x01 || depth_byte || region(key, depth) || left || right)
(pkg/api/inclusion_cert.go), so the documented formula reproduces the correct
root only for a proof with zero siblings. Any proof carrying a sibling would
verify against a different root in an independent client implementing the doc --
and that document is cited in three places as the frozen specification.
The same formula was already wrong in README.md, which is where it was copied
from; both are corrected. README also described key bit addressing as LSB-first,
which the switch to big-endian ordering invalidated: bit(key,d) is
(key[d/8] >> (7 - d%8)) & 1, verified against KeyBitBE.
Pins both rules in tests that build a root from the documented formula and
require Verify to accept it, and that assert a root built without the region is
rejected -- so this drifts in CI rather than in someone else's client.
|
Good catch, fixed in 6f4bd56 — and it was worse than the comment says. The verifier computes `H(0x01 || depth_byte || region(key, depth) || left || right)` ( Two things beyond the flagged line:
Rather than just editing prose, both rules are now pinned by tests that build a root from the documented formula and require |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f4bd5652e
ℹ️ 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".
| - Value: `SHA-256(CBOR([transactionHash, referenceTime]))` for every inclusion proof | ||
| - Leaf: `H(0x00 || key || value)` | ||
| - Inner node (two children): `H(0x01 || depth_byte || left || right)` | ||
| - Inner node (two children): `H(0x01 || depth_byte || region(key, depth) || left || right)` |
There was a problem hiding this comment.
The fresh evidence is that this newly updated hash rule includes region(key, depth), while the immediately following pseudocode still computes each inner hash without it. An independent client copying that pseudocode will derive the wrong root for every proof containing a sibling, despite the surrounding text now claiming the algorithm is wire-compatible; add the region to both pseudocode branches.
Useful? React with 👍 / 👎.
| return nil, fmt.Errorf("failed to get block %s: %w", req.BlockNumber.String(), err) | ||
| } | ||
| if block != nil { | ||
| finalizedAt = block.CreatedAt |
There was a problem hiding this comment.
In standalone and BFT-shard mode, models.NewBlock sets CreatedAt before the certification request is sent to BFT (internal/round/batch_processor.go), so certification and finalization occur later. Exposing that proposal-creation timestamp as finalizedAt makes every block record underreport its finalization time by the BFT wait and persistence duration; persist and return the actual finalization/proof-ready timestamp instead.
Useful? React with 👍 / 👎.
…izedAt Two follow-ups from review of the previous commits. The README hash-rule bullet was corrected to include region(key, depth), but the verification pseudocode immediately below it still computed each inner hash without the region -- the same defect, in the same section, one code block down. An independent client copying the pseudocode would still derive a wrong root for every proof containing a sibling. finalizedAt was populated from the block's CreatedAt, which models.NewBlock stamps when the block is constructed at proposal time, before the certification request is sent to BFT. Every record would therefore have underreported its finalization time by the BFT wait plus persistence. Nothing currently persists a real finalization timestamp, so the field is removed from api.AggregatorRecord and the documented example rather than returned wrong; adding one means writing it on the block at finalization, which belongs in its own change.
|
Both fixed in 6c46303. Pseudocode (P1). Right — I corrected the hash-rule bullet and missed the code block four lines below it, which is the part an integrator would actually copy. Same defect, same section. Now finalizedAt (P2). Also right, and it was my change that introduced it. I have not persisted a real finalization timestamp here, and want to be explicit about why rather than quietly doing the smaller thing: the finalization path branches across disk/non-disk, already-finalized-duplicate, and recovery, and I have not traced all of them. Adding a timestamp that is right on one branch and missing on another is a worse outcome than not having one. So The original audit finding stands: the field was documented but never populated. This resolves that by removing the claim rather than by inventing a value. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c46303db7
ℹ️ 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".
| ) | ||
|
|
||
| func main() { | ||
| os.Setenv("BFT_ENABLED", "false") |
There was a problem hiding this comment.
When cfgdump is run with a valid SHARDING_MODE=bft-shard deployment configuration, this assignment overwrites the required BFT_ENABLED=true value before loading it. Config.Validate then rejects the configuration (internal/config/config.go:625-627), so the new command exits without dumping anything for BFT-shard deployments; avoid overriding the caller's environment.
Useful? React with 👍 / 👎.
|
Had uncommitted YP changes. Wanted to rewrite by hand. Committed as ba5d71680f47e9d978f24bff819aeb6b6f394ce2 , now should be in sync with the implementation |
A section-by-section audit against the implementation, with each claim checked by executing something -- loading the config, marshalling the real response struct, running the routing predicate -- rather than reading struct tags. Factually wrong: - CHAIN_FORK_ID default is testnet, not mainnet - BFT_KEY_CONF_FILE is read by no code; the real variable is SIGNING_KEY_FILE - BATCH_LIMIT caps nothing, it is only logged at startup - shard routing reads the LEADING bits of the state ID, not the trailing ones; the prose, the worked example and both ASCII diagrams said the opposite - the two child shard IDs were swapped relative to sharding-compose.yml - STATE_ID_MISMATCH is SHA256(CBOR[ownerPredicate, sourceStateHash]), not publicKey; SIGNATURE_VERIFICATION_FAILED covers the source state hash too - get_block omitted totalCommitments; several example payloads had fields that no longer exist or values the code never produces - bft-shard and parent/child use the SAME routing key, not different ones Undocumented: 22 environment variables the code reads had no README entry at all -- HTTP/2, log rotation, Redis tuning, SMT proof precomputation and the child poll timings. Every variable read by config.go is now documented, and every variable documented is now read by config.go; both directions checked mechanically.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f031c4a4f1
ℹ️ 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".
| | Index | Field | Type | | ||
| |-------|-------|------| | ||
| | 0 | `version` | uint, `2` | | ||
| | 1 | `ownerPredicate` | array | |
There was a problem hiding this comment.
Document ownerPredicate as #39032([engine, code, params]), not a plain array, and add tag 39032 to the registry above. Every emitted predicate is wrapped by Predicate.MarshalCBOR, while Predicate.UnmarshalCBOR requires that tag, so an independent client implementing this newly frozen byte specification will reject or re-encode every inclusion proof's certification data incorrectly.
Useful? React with 👍 / 👎.
…as design An audit against the yellowpaper found that this document had turned two implementation gaps into apparent specification. The shard binding. platform.tex VerifyInclusionProof takes the partition description as an input and requires f_SH(sid) = sigma -- the expected shard is derived from the KEY. It also states that VerifyUnicityCert does not by itself prove a state identifier belongs to the shard named in the certificate, and that the binding is the proof verifier'\''s job. InclusionProofV2.Verify instead compares the UC'\''s shard against a caller-supplied ExpectedShardID. A leaf routing to shard A, committed in shard B'\''s SMT under shard B'\''s validly signed UC, verifies -- reproduced in testing. The previous text told integrators to derive ExpectedShardID from configuration, which is not the specified check and does not close the gap. Now recorded as the soundness divergence it is. The network id check UC.C^r.alpha = T.alpha is likewise absent. ExclusionCert. appendix-hashtrees.tex orders the certificate bitmap || siblings || k'\'' || v'\''; the Go type puts the fixed 32-byte fields first, which also makes a variable-length v'\'' unencodable and leaves the spec'\''s empty-tree certificate undecodable. Nothing generates or verifies these, and neither security-critical check the spec names exists. Documented as a divergence rather than as a frozen format. The preamble now says plainly that the yellowpaper is authoritative and that this file must not present an implementation gap as a specification.
|
Section 4 should be resolved, YP was out of sync. Sec. 1, 2 are not related. Also, it should be stated that time unit is in seconds. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17e2ec0ae1
ℹ️ 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".
| // The yellowpaper makes the deadline a mandatory element of the request; the | ||
| // service_assigned series is the migration backlog, and reaching zero is the | ||
| // precondition for rejecting requests that omit it. | ||
| CertificationRequestsByDeadline = promauto.NewCounterVec( |
There was a problem hiding this comment.
When any accepted request omits its deadline, this Prometheus counter becomes nonzero and can never “reach zero” as the migration condition above requires; conversely, a process restart resets it to zero even if unmigrated clients are still active. This makes the advertised signal unsafe for deciding when to reject missing deadlines. Track the absence rate over an explicit observation window (or represent an actual outstanding population) rather than treating the raw cumulative counter as a backlog.
Useful? React with 👍 / 👎.
…ation case
Yellowpaper ba5d716 ("us request expiry as absolute time") specifies the request
timeout as optional: Q carries tau_Q_bar in T union {bottom}, bottom is written
as CBOR null at a fixed position, and the effective timeout is tau_a + Delta when
absent and tau_Q_bar otherwise, where tau_a is the latest consensus-derived
reference time at admission and Delta the service's default request lifetime.
The assigned value is service metadata and does not alter txhash.
That is what this service already does, so the comments claiming the paper makes
the deadline mandatory were wrong, as was framing the deadline-origin counter as
a migration backlog with retirement of the absent form as its goal. The counter
stays -- what share of traffic depends on DEFAULT_REQUEST_TTL is worth seeing --
but it is operational visibility, not a deprecation clock.
Appendix ba5d716 also confirms the fixed-shape encoding this wire already uses:
"Each version of a structure therefore has exactly one tuple shape and one
element count, and an optional element occupies its position whether or not it
carries a value."
Also conformant, and now stated as such: the admission check is permitted but not
sufficient, with the authoritative check at leaf materialisation against that
round's pinned reference time -- which is where it already runs.
Eleven throwaway probe tests written by the spec-audit subagents in this worktree were swept into f031c4a and 17e2ec0 by a 'git add -A'. They are scratch reproductions, not tests meant to ship -- roughly 1,700 lines of zz_-prefixed files under internal/smt and pkg/api. Net diff against main is unaffected; they were added and are now removed. The most useful of them, the reproduction of the missing f_SH(sid) shard binding, is preserved in issue #183 rather than here, where it would have shipped as a permanently failing test.
Three more subagent scratch files reached the branch via 'git add -A': cmd/cfgdump/main.go (a config dumper), internal/signing/zzaudit_test.go and internal/signing/zzlens_predicate_test.go. The earlier cleanup missed them because it globbed zz_* and these are cfgdump and zz-without-underscore. Also fixes a real defect in the wire spec found in review: ownerPredicate was documented as a bare array, but Predicate.MarshalCBOR emits tag 39032 and Predicate.UnmarshalCBOR requires it, so a client following the document would reject or re-encode every proof's certification data. Verified on the wire: engine 1 / code 0x01 / params 0x02 encodes as d99878 83 01 4101 4102. Tag 39032 added to the registry.
MastaP
changed the title
|
@ristik rechecked with YP. the only business-logic fix is |
MastaP
changed the title
2 participants
Security fix: the verifier accepted leaves from the wrong shard (c767749, @ristik)
InclusionProofV2.Verifychecked the UC against a caller-suppliedExpectedShardIDand never checked that the state ID actually belongs to the certified shard. A leaf routing to shard A, committed in shard B's SMT under B's validly signed UC, verified — so in a multi-shard deployment a faulty or Byzantine quorum could certify a state ID owned by another shard, and the same state ID could hold accepted proofs in two shards. The seal's network ID was unchecked too, a domain-separation failure exploitable when validator keys are reused across trust bases.The fix takes the shard from the certificate rather than the caller —
uc.ShardTreeCertificate.Shard.Comparator()(key)— which is the same binding the JS, Java and Rust SDKs enforce, with noVerifierContextbreak. It also closes a more general gap:stateIdis now derived from the certification data viaCreateStateID(), and owner predicate, source state hash and witness must match between proof and request. Previously onlytransactionHashandexpiresAtwere compared, so a proof could carry different certification data than the request it was verified against.Four regression tests come with it. I mutation-checked the shard binding locally: deleting the block makes
TestInclusionProofV2Verify_StateIDMustBelongToCertifiedShardfail, so it guards the hole rather than passing vacuously.Supersedes #183 (my proposal there wanted the full partition descriptor and a public API break — unnecessary). Tracked in #185.
The other functional fix:
get_block_recordsreturned unverifiable recordsmodelToAPIAggregatorRecordcopied four fields and dropped the rest.referenceTimewas not even a field onapi.AggregatorRecord— and since the leaf value isLeafValue(transactionHash, referenceTime), a consumer had no way to rebuild it. The endpoint's whole purpose is defeated for anyone verifying proofs.expiresAtwas dropped too, so the request deadline couldn't be checked either.Both are now returned, along with
version. The README documented all of this pluspublicKey/signature— fields that no longer exist onCertificationData— and hex-encoded predicate bytes that actually serialize as base64. The example is now generated from real output, and a test pins the emitted key set so it can't drift again.finalizedAtis removed rather than returned:models.NewBlockstampsCreatedAtat construction, i.e. proposal time, before the certification request goes to BFT, so exposing it would underreport finalization by the whole BFT round trip. Nothing persists a real finalization timestamp. It was alwaysnull, so nothing regresses.Everything below is correction, not capability.
Documentation (402 lines)
The README was wrong in ways that would mislead an integrator:
KeyBitBE. The prose, the worked example, and both ASCII diagrams all said the opposite. Confirmed against the spec too: "keys are read as big-endian bit strings".BFT_KEY_CONF_FILEis read by no code — the real variable isSIGNING_KEY_FILE. Proven by execution: setting the documented one loads cleanly with an empty key path.CHAIN_FORK_IDdefaults totestnet, notmainnet;BATCH_LIMITcaps nothing (one consumer, a startup log field); the two child shard IDs were swapped relative tosharding-compose.yml;STATE_ID_MISMATCHisSHA256(CBOR[ownerPredicate, sourceStateHash]), notpublicKey.config.goreads is documented (104/104), and every documented variable is actually read (0 phantom).docs/inclusion-proof-wire.mdis new — three source comments cited it as the frozen specification, but the file did not exist. Two defects were caught in it during review, both of which would have broken an independent client:ownerPredicateis tagged, not a bare array —Predicate.MarshalCBORemits tag 39032 and unmarshal requires it, so a client following the document would reject every proof's certification data.The doc also records the remaining
ExclusionCertdivergence (#184). The shard-binding section it carried is gone — c767749 fixed the underlying gap, so the document no longer needs to warn about it.Tests (316 lines)
>=to>inpkg/api/types.goleft the wholepkg/apisuite green, because the only fully-built proof fixture leavesExpiresAtnil. Now covered at both ends, plus presence-mismatch. (Note for future tests here:buildSignedSingleLeafProofaliases the proof's certification data to the request's, so the two can never disagree unless the copy is detached — my first attempt passed vacuously because of it.)Verifymust accept it, and a root built without the region must be rejected. Documentation of a wire format that nothing executes is exactly how this drifted twice.get_block_recordskey set is pinned so the README can't silently diverge again.Metric (21 lines)
aggregator_certification_requests_by_deadline_total{origin}splits accepted requests by whether the requester supplied a deadline or the service assigned one fromDEFAULT_REQUEST_TTL.Its original justification was wrong and has been retracted in-branch. I had read the yellowpaper as making the request timeout mandatory and framed this as a migration backlog. Yellowpaper
ba5d716specifies it as optional —τ̄_Q ∈ 𝕋 ∪ {⊥},⊥written as CBOR null at a fixed position, effective timeoutτ̂_Q = τ_a + Δwhen absent — which is exactly what this service does. It also confirms the fixed-shape encoding used here: "each version of a structure has exactly one tuple shape and one element count, and an optional element occupies its position whether or not it carries a value." The counter stays as operational visibility into how much traffic depends on the default lifetime; it is not a deprecation clock. Drop this commit if you'd rather not carry it.A review round also caught it being incremented at deadline assignment rather than after acceptance, so expired and duplicate requests inflated it. Fixed, and the regression test was mutation-checked — reintroducing the bug makes it fail.
Housekeeping
Several throwaway probe files written by audit subagents in the worktree were swept in by
git add -A(cmd/cfgdump/main.go,zz*-prefixed tests underinternal/smt,pkg/api,internal/signing). All removed; the net diff is clean. The reproduction of the shard-binding bug is preserved in #183 rather than shipped as a permanently failing test.Verification
go buildandgo vetclean on Go 1.26. Passing:pkg/...,internal/config,internal/models,internal/gateway,internal/signing, and the targetedinternal/servicetests.internal/serviceandinternal/roundare flaky against MongoDB testcontainers on loaded hardware — every failure is ani/o timeoutduring index creation, never an assertion, and unmodifiedmainreproduces the same failing set under identical conditions. Usego test -p 1locally; the default per-package parallelism starts enough Mongo containers to starve them.Related
Found by the same spec audit: #185 (shard/network binding — fixed here by c767749; supersedes #183) and #184 (four non-critical divergences, recorded, not being pursued).