Cleanups from reviewing #178: single-pass CBOR decode, TTL default, drop metric - #180
Merged
Conversation
UnmarshalCBOR ran UnmarshalTagged to check the tag and element count, then UnmarshalTaggedValue to decode the same bytes again. The probe pass was redundant: the toarray decode rejects a wrong tag and a wrong element count on its own, so only the version check needs the decoded value. This is the per-request path (internal/gateway/handlers.go decodes every certification_request), and the probe was roughly half of the decode cost: before 5701-5781 ns/op 1288 B/op 32 allocs/op after 3010-3098 ns/op 576 B/op 13 allocs/op
The 1h default was written twice: once as the DEFAULT_REQUEST_TTL environment default and once as the zero-value fallback in RequestTTL(). The two could drift, and which one applied depended on whether the config came from the environment or was built in code. Both now derive from DefaultRequestTTLFallback. Validate still accepts 0, which remains the "unset, use the default" signal for programmatic configs.
commitmentLeafInput reads as a pure builder but assigns commitment.ReferenceTime. The adjacent models.CertificationRequest.LeafValue is genuinely pure, and the PR's own test asserts that -- so the two sat side by side with opposite contracts and near-identical names. Renamed to materializeCommitmentLeaf and made the write explicit in the doc comment. No behaviour change.
An expired request is acked out of the queue after the service already
answered SUCCESS, leaving no aggregator record and no durable trace. It was
logged at Debug with no counter, so a node could discard an arbitrary volume
of acknowledged work with nothing visible on a dashboard -- and a backlog
exceeding DEFAULT_REQUEST_TTL drops requests in bulk.
Adds aggregator_commitments_dropped_total{reason}, covering the pre-existing
duplicate and rejected drop paths as well, neither of which was instrumented
either. Raises the expiry log from Debug to Warn to match the neighbouring
rejected-leaf path, and includes effectiveTimeout so the service-assigned
deadline is visible alongside the requester's own.
This was referenced Aug 31, 2026
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
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.
Stacked on #178 — base is
service-time, notmain. Merge #178 first, or merge this into it.Four independent cleanups from reviewing #178. Each commit stands alone; take, drop, or squash whichever you like. No behaviour change except the drop-log level.
1.
CertificationData.UnmarshalCBORdecoded twice (c819645)UnmarshalTaggedprobed the tag, element count and version, thenUnmarshalTaggedValuedecoded the same bytes again. The probe is redundant: the toarray decode rejects a wrong tag and a wrong element count on its own, so only the version check needs the decoded value.This is the per-request path (
internal/gateway/handlers.go:45), and the probe was roughly half the decode cost:Verified by differential test against the old implementation across 14 inputs — wrong tag, 5 and 7 fields, versions 0/1/3, untagged array, tagged non-array, truncated, garbage, empty, nil — asserting identical accept/reject decisions and identical decoded values. Error text differs; nothing matches on it. The interesting cases are kept as
pkg/api/certification_data_decode_test.goso the validation surface cannot be weakened silently later.ValidateCoreDeterministicis unaffected: it runs on the whole payload inUnmarshalCertificationRequestCBORbefore the nested decode.2. One source for the default request TTL (
5665511)The 1h default was written twice — as the
DEFAULT_REQUEST_TTLenv default (config.go:393) and as the zero-value fallback inRequestTTL()(config.go:126-131). They could drift, and which one applied depended on whether the config came from the environment or was constructed in code. Both now derive fromDefaultRequestTTLFallback.Validateis unchanged and still accepts0as the "unset, use the default" signal for programmatic configs.3. Name the leaf builder for the write it performs (
ffb3e31)commitmentLeafInputreads as a pure builder but assignscommitment.ReferenceTime(leaf_add.go:50). The adjacentmodels.CertificationRequest.LeafValueis genuinely pure —TestCertificationRequestLeafValue_DoesNotMutateReferenceTimeasserts exactly that — so two near-identically-named functions sat side by side with opposite contracts.Renamed to
materializeCommitmentLeaf, with the write called out in the doc comment. Test names updated to match.4. Count commitments dropped before reaching a block (
93dab79)An expired request is acked out of the queue after the service already answered
SUCCESS, leaving no aggregator record and no durable trace. It was logged atDebugwith no counter, so a node could discard an arbitrary volume of acknowledged work with nothing visible on a dashboard — and a backlog exceedingDEFAULT_REQUEST_TTLdrops requests in bulk.aggregator_commitments_dropped_total{reason}, covering the pre-existingduplicateandrejecteddrop paths as well, neither of which was instrumented either.DebugtoWarn, matching the neighbouring rejected-leaf path, and logseffectiveTimeoutalongside the requester's ownexpiresAt.deploy/grafana/dashboards/aggregator.json.Label values are resolved once at init rather than per call: these increments happen while
roundMutexis held (round_manager.go:896-922), so they should not pay for aCounterVeclabel lookup per dropped commitment.The counter counts drop events, not distinct commitments — if the queue ack fails the commitment is retried and counted again. The
Helptext says so.Verification
go buildandgo vetclean on Go 1.26. Passing:pkg/...,internal/config,internal/models,internal/gateway,internal/service,internal/ha.internal/roundshares the base branch's pre-existing container flakiness on the machine I ran this on — every failure on both sides is a MongoDB testcontaineri/o timeoutduring index creation, not an assertion. Compared against unmodifiedservice-time:--- FAILcountservice-time(base)The failing set on this branch is a strict subset of the base's, so nothing here introduces a regression. CI on less contended hardware is the real check.