feat: validator node version attestation with observer demotion - #86
feat: validator node version attestation with observer demotion#86Test0rMaik wants to merge 17 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds fork-gated validator node-version attestation, persists attested versions and epochs, enforces configured versions during end-of-epoch processing, and demotes or restores validator peer-list status using supermajority and minimum-electable-node guards. ChangesValidator version attestation
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Sequence Diagram(s)sequenceDiagram
participant ValidatorConfig
participant UpdateValidator
participant ProcessEconomicsEndOfEpoch
participant updatePeerListStatus
ValidatorConfig->>UpdateValidator: provide NodeVersion
UpdateValidator->>UpdateValidator: store AttestedVersion and AttestedEpoch
ProcessEconomicsEndOfEpoch->>ProcessEconomicsEndOfEpoch: compute version enforcement
ProcessEconomicsEndOfEpoch->>updatePeerListStatus: pass enforcement state
updatePeerListStatus->>updatePeerListStatus: demote or restore validator
🚥 Pre-merge checks | ✅ 6 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
core/kapp/validators/peersUpdate.go (3)
315-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNested enforcement branch adds noticeable complexity to an already-dense function.
Logic is correct (verified against the demote/no-demote/restore semantics documented above it), but the two-level nested conditional inside the
enforcement.activebranch pushes this function's cyclomatic complexity notably higher. Consider extracting the enforcement decision into a small helper, e.g.resolveVersionEnforcedList(current state.List, enforcement versionEnforcement, val *ValidatorData) state.List, and haveupdatePeerListStatuscall it andSetListonce.♻️ Sketch of the extraction
+func resolveVersionEnforcedList(current state.List, enforcement versionEnforcement, val *ValidatorData) state.List { + if !enforcement.isSatisfiedBy(val) { + if enforcement.demote { + return state.List_observer + } + if current != state.List_observer && current != state.List_elected { + return state.List_eligible + } + return current + } + if current != state.List_elected { + return state.List_eligible + } + return current +} + } else if enforcement.active && !enforcement.isSatisfiedBy(val) { - if enforcement.demote { - peerAcc.SetList(state.List_observer) - } else if peerAcc.GetList() != state.List_observer && peerAcc.GetList() != state.List_elected { - peerAcc.SetList(state.List_eligible) - } + peerAcc.SetList(resolveVersionEnforcedList(peerAcc.GetList(), enforcement, val))Based on the line-range change details flagging
[code_block_complexity_high]for this block, extracting the enforcement decision improves testability and readability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/kapp/validators/peersUpdate.go` around lines 315 - 339, Extract the version-enforcement decision from updatePeerListStatus into a helper such as resolveVersionEnforcedList, accepting the current state.List, enforcement versionEnforcement, and *ValidatorData. Preserve the existing demotion, observer/elected guard, and eligible restoration semantics, then have updatePeerListStatus apply the helper’s result with a single SetList call for the enforcement path.
466-506: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCorrect enforcement wiring; note the
getValidatorre-fetch at Lines 483-486.
enforcementis threaded through correctly and passed toupdatePeerListStatusin the right position. ThegetValidatorcall at Lines 483-486 re-fetches the same*ValidatorDatathatcomputeVersionEnforcementalready fetched for electable validators (seeversionAttestation.go) — see consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/kapp/validators/peersUpdate.go` around lines 466 - 506, Preserve the existing enforcement wiring in processValidatorEpochV1: keep enforcement passed to updatePeerListStatus in its current argument position. Retain the getValidator call that reloads the validator data before reward and status updates, matching the established flow used by computeVersionEnforcement for electable validators.
620-664: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCorrect enforcement wiring; note the
getValidatorre-fetch at Lines 637-640.Same pattern as
processValidatorEpochV1:enforcementis threaded through correctly, but Lines 637-640 re-fetch*ValidatorDataalready fetched bycomputeVersionEnforcement— see consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/kapp/validators/peersUpdate.go` around lines 620 - 664, Remove the redundant validator fetch in processValidatorEpochV2 by reusing the *ValidatorData already loaded during computeVersionEnforcement, while preserving the existing error and downstream processing behavior. Update the function’s inputs or caller as needed so the existing val-dependent logic uses that fetched instance instead of calling getValidator again.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/kapp/validators/versionAttestation_test.go`:
- Around line 655-689: Replace the positional infos[2] update in the “outdated
validator is demoted and restored after attesting” test with a lookup that finds
the entry whose OwnerAddress matches owner3, then set that entry’s List to
observer before the next ProcessEconomicsEndOfEpoch call. Preserve the existing
restore assertions and avoid relying on map iteration order.
In `@core/kapp/validators/versionAttestation.go`:
- Line 1: Update computeVersionEnforcement to retain each successfully fetched
*ValidatorData by owner address in a cache and log getValidator errors before
continuing. Return or otherwise expose this cache, then have
processValidatorEpochV1 and processValidatorEpochV2 consult it before calling
v.getValidator, falling back only when the address is absent.
- Around line 191-198: Update the validator evaluation loop to log the
OwnerAddress and error when getValidator fails instead of silently continuing,
while preserving the failure behavior. Thread each successfully loaded
*ValidatorData from the evaluation flow into processValidatorEpochV1 and
processValidatorEpochV2 so they reuse it and avoid fetching the same validator
record again.
---
Outside diff comments:
In `@core/kapp/validators/peersUpdate.go`:
- Around line 315-339: Extract the version-enforcement decision from
updatePeerListStatus into a helper such as resolveVersionEnforcedList, accepting
the current state.List, enforcement versionEnforcement, and *ValidatorData.
Preserve the existing demotion, observer/elected guard, and eligible restoration
semantics, then have updatePeerListStatus apply the helper’s result with a
single SetList call for the enforcement path.
- Around line 466-506: Preserve the existing enforcement wiring in
processValidatorEpochV1: keep enforcement passed to updatePeerListStatus in its
current argument position. Retain the getValidator call that reloads the
validator data before reward and status updates, matching the established flow
used by computeVersionEnforcement for electable validators.
- Around line 620-664: Remove the redundant validator fetch in
processValidatorEpochV2 by reusing the *ValidatorData already loaded during
computeVersionEnforcement, while preserving the existing error and downstream
processing behavior. Update the function’s inputs or caller as needed so the
existing val-dependent logic uses that fetched instance instead of calling
getValidator again.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 894d7c41-497b-4b6c-b400-25c285ad8cfa
⛔ Files ignored due to path filters (2)
core/kapp/validators/validatorData.pb.gois excluded by!**/*.pb.go,!**/*.pb.godata/transaction/contracts.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (17)
cmd/node/startup.gocommon/constants.gocommon/mock/forkControllerStub.goconfig/enableEpochs.goconfig/node/enableEpochs.yamlcore/fork/forks.gocore/interface.gocore/kapp/factory/validators.gocore/kapp/kappController/kapp.gocore/kapp/validators/peersUpdate.gocore/kapp/validators/proto/validatorData.protocore/kapp/validators/validators.gocore/kapp/validators/versionAttestation.gocore/kapp/validators/versionAttestation_test.godata/transaction/proto/contracts.protofactory/stateComponents.gointegrationTest/mock/forkControllerStub.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Verify that any new or modified concurrent code (goroutines, channels, mutexes, sync primitives) is free of race conditions. Check for: proper lock/unlock pairing, no goroutine leaks, correct channel lifecycle management, and proper context cancellation propagation.
Verify that errors are not silently discarded. Check for: unchecked error returns, error wrapping with context, proper error propagation up the call chain, and no bare panic() calls outside of init() functions.
Files:
config/enableEpochs.gocommon/constants.gocore/kapp/factory/validators.gocore/interface.gocmd/node/startup.gointegrationTest/mock/forkControllerStub.gocommon/mock/forkControllerStub.gocore/fork/forks.gofactory/stateComponents.gocore/kapp/kappController/kapp.gocore/kapp/validators/versionAttestation.gocore/kapp/validators/validators.gocore/kapp/validators/peersUpdate.gocore/kapp/validators/versionAttestation_test.go
core/kapp/**
⚙️ CodeRabbit configuration file
core/kapp/**: KApps (blockchain application layer) processes on-chain transactions. - Verify correct balance/state changes are atomic and consistent - Check for integer overflow/underflow in financial calculations - Ensure all error paths properly revert state changes - Validate access control and permission checks - Flag any missing input validation on transaction parameters
Files:
core/kapp/factory/validators.gocore/kapp/validators/proto/validatorData.protocore/kapp/kappController/kapp.gocore/kapp/validators/versionAttestation.gocore/kapp/validators/validators.gocore/kapp/validators/peersUpdate.gocore/kapp/validators/versionAttestation_test.go
data/**
⚙️ CodeRabbit configuration file
data/**: Core data structures and state management. - Verify protobuf serialization/deserialization is correct and backwards-compatible - Check for proper nil/empty checks on decoded data - Ensure trie operations maintain data integrity - Validate state transitions are deterministic
Files:
data/transaction/proto/contracts.proto
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions - Race condition coverage (tests should use -race flag patterns) - No hardcoded sleep for synchronization (use channels or sync primitives) - Test isolation (no shared mutable state between tests)
Files:
core/kapp/validators/versionAttestation_test.go
🧠 Learnings (2)
📚 Learning: 2026-04-21T20:12:22.959Z
Learnt from: phcarneirobc
Repo: klever-io/klever-go PR: 38
File: indexer/eventsProcessor.go:188-211
Timestamp: 2026-04-21T20:12:22.959Z
Learning: In Go structs that are JSON-marshaled, if a field is a `bool` and has the `json:"...,omitempty"` tag, then leaving that field at its zero value (`false`) is functionally equivalent (in the resulting JSON) to explicitly setting `Foundation: false`. Reviewers should not flag struct literals that omit such `bool` fields as an inconsistency; they will serialize identically because `omitempty` suppresses `false` values.
Applied to files:
config/enableEpochs.gocommon/constants.gocore/kapp/factory/validators.gocore/interface.gocmd/node/startup.gointegrationTest/mock/forkControllerStub.gocommon/mock/forkControllerStub.gocore/fork/forks.gofactory/stateComponents.gocore/kapp/kappController/kapp.gocore/kapp/validators/versionAttestation.gocore/kapp/validators/validators.gocore/kapp/validators/peersUpdate.gocore/kapp/validators/versionAttestation_test.go
📚 Learning: 2026-05-23T22:52:58.065Z
Learnt from: fbsobreira
Repo: klever-io/klever-go PR: 65
File: data/blockchain/blockchain.go:170-172
Timestamp: 2026-05-23T22:52:58.065Z
Learning: In Go, the pattern `append([]byte(nil), src...)` should be treated as preserving nil identity when `src` is a nil `[]byte`: spreading a nil slice contributes zero variadic arguments, so `append` performs no allocation and returns the original nil destination slice unchanged (i.e., result is nil, not an empty non-nil slice). Do not flag this as an incorrect empty-slice conversion; it intentionally maintains `nil`.
Applied to files:
config/enableEpochs.gocommon/constants.gocore/kapp/factory/validators.gocore/interface.gocmd/node/startup.gointegrationTest/mock/forkControllerStub.gocommon/mock/forkControllerStub.gocore/fork/forks.gofactory/stateComponents.gocore/kapp/kappController/kapp.gocore/kapp/validators/versionAttestation.gocore/kapp/validators/validators.gocore/kapp/validators/peersUpdate.gocore/kapp/validators/versionAttestation_test.go
🪛 ast-grep (0.44.1)
core/kapp/validators/versionAttestation.go
[warning] 201-201: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(satisfied)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🪛 Buf (1.71.0)
core/kapp/validators/proto/validatorData.proto
[error] 29-29: Field name "AttestedVersion" should be lower_snake_case, such as "attested_version".
(FIELD_LOWER_SNAKE_CASE)
[error] 30-30: Field name "AttestedEpoch" should be lower_snake_case, such as "attested_epoch".
(FIELD_LOWER_SNAKE_CASE)
data/transaction/proto/contracts.proto
[error] 165-165: Field name "NodeVersion" should be lower_snake_case, such as "node_version".
(FIELD_LOWER_SNAKE_CASE)
🔇 Additional comments (19)
common/constants.go (1)
77-77: LGTM!core/kapp/validators/proto/validatorData.proto (1)
28-30: LGTM!data/transaction/proto/contracts.proto (1)
162-165: LGTM!config/enableEpochs.go (1)
31-31: LGTM!config/node/enableEpochs.yaml (1)
35-38: LGTM!core/fork/forks.go (1)
29-29: LGTM!Also applies to: 97-100, 141-143
core/interface.go (1)
85-85: LGTM!cmd/node/startup.go (1)
578-586: LGTM!factory/stateComponents.go (1)
26-38: LGTM!Also applies to: 54-60, 98-104
core/kapp/kappController/kapp.go (1)
5-5: LGTM!Also applies to: 58-63, 74-75
core/kapp/factory/validators.go (1)
4-4: LGTM!Also applies to: 18-28
core/kapp/validators/validators.go (1)
14-14: LGTM!Also applies to: 64-74, 85-91, 118-125, 543-558
common/mock/forkControllerStub.go (1)
20-20: LGTM!Also applies to: 61-62, 83-83, 102-102, 171-174
integrationTest/mock/forkControllerStub.go (1)
17-17: LGTM!Also applies to: 128-134
core/kapp/validators/versionAttestation_test.go (1)
17-560: LGTM!core/kapp/validators/versionAttestation.go (3)
1-146: LGTM!
148-190: LGTM!
199-226: LGTM!core/kapp/validators/peersUpdate.go (1)
694-720: LGTM!Also applies to: 722-752
| @@ -0,0 +1,226 @@ | |||
| package validators | |||
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Validator records are fetched twice per epoch for the electable set. computeVersionEnforcement's guard loop and the subsequent per-validator processing loop both call getValidator for the same electable validators within the same end-of-epoch pass; sharing the already-fetched records would remove the duplicate read.
core/kapp/validators/versionAttestation.go#L191-198: cache the fetched*ValidatorDataper owner address here (e.g. into amap[string]*ValidatorDatareturned alongsideversionEnforcement, or stored onv) instead of discarding it once the satisfied-check is done; also log thegetValidatorerror instead of a barecontinueso guard undercounts are diagnosable.core/kapp/validators/peersUpdate.go#L466-506: inprocessValidatorEpochV1, look up the address in the cache built bycomputeVersionEnforcementbefore falling back tov.getValidator(app, addr)at Lines 483-486.core/kapp/validators/peersUpdate.go#L620-664: inprocessValidatorEpochV2, apply the same cache lookup beforev.getValidator(app, addr)at Lines 637-640.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/kapp/validators/versionAttestation.go` at line 1, Update
computeVersionEnforcement to retain each successfully fetched *ValidatorData by
owner address in a cache and log getValidator errors before continuing. Return
or otherwise expose this cache, then have processValidatorEpochV1 and
processValidatorEpochV2 consult it before calling v.getValidator, falling back
only when the address is absent.
|
Addressed the automated review in 8958841:
Declined (with reasoning): caching |
8958841 to
57d05ed
Compare
|
Rewrote the 3 commits to drop the |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/kapp/validators/proto/validatorData.proto`:
- Around line 29-30: Rename the protobuf source fields AttestedVersion and
AttestedEpoch to attested_version and attested_epoch in
core/kapp/validators/proto/validatorData.proto, preserving field numbers and
explicit JSON names; rename NodeVersion to node_version in
data/transaction/proto/contracts.proto, then regenerate all protobuf bindings.
In `@core/kapp/validators/versionAttestation_test.go`:
- Line 642: Update the test setup around LoadPeer to capture its returned error
and assert it with require.NoError before calling peerAcc.SetList, preventing an
opaque nil dereference. Also replace discarded errors from getKApp in the
referenced test setup paths with require.NoError assertions, preserving the
existing values and flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: de4e0174-0e35-4c72-bb42-46b68a73505a
⛔ Files ignored due to path filters (2)
core/kapp/validators/validatorData.pb.gois excluded by!**/*.pb.go,!**/*.pb.godata/transaction/contracts.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (17)
cmd/node/startup.gocommon/constants.gocommon/mock/forkControllerStub.goconfig/enableEpochs.goconfig/node/enableEpochs.yamlcore/fork/forks.gocore/interface.gocore/kapp/factory/validators.gocore/kapp/kappController/kapp.gocore/kapp/validators/peersUpdate.gocore/kapp/validators/proto/validatorData.protocore/kapp/validators/validators.gocore/kapp/validators/versionAttestation.gocore/kapp/validators/versionAttestation_test.godata/transaction/proto/contracts.protofactory/stateComponents.gointegrationTest/mock/forkControllerStub.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Verify that any new or modified concurrent code (goroutines, channels, mutexes, sync primitives) is free of race conditions. Check for: proper lock/unlock pairing, no goroutine leaks, correct channel lifecycle management, and proper context cancellation propagation.
Verify that errors are not silently discarded. Check for: unchecked error returns, error wrapping with context, proper error propagation up the call chain, and no bare panic() calls outside of init() functions.
Files:
core/interface.gofactory/stateComponents.goconfig/enableEpochs.gocore/fork/forks.gocommon/constants.gocore/kapp/kappController/kapp.gocmd/node/startup.gointegrationTest/mock/forkControllerStub.gocore/kapp/factory/validators.gocommon/mock/forkControllerStub.gocore/kapp/validators/validators.gocore/kapp/validators/versionAttestation.gocore/kapp/validators/peersUpdate.gocore/kapp/validators/versionAttestation_test.go
data/**
⚙️ CodeRabbit configuration file
data/**: Core data structures and state management. - Verify protobuf serialization/deserialization is correct and backwards-compatible - Check for proper nil/empty checks on decoded data - Ensure trie operations maintain data integrity - Validate state transitions are deterministic
Files:
data/transaction/proto/contracts.proto
core/kapp/**
⚙️ CodeRabbit configuration file
core/kapp/**: KApps (blockchain application layer) processes on-chain transactions. - Verify correct balance/state changes are atomic and consistent - Check for integer overflow/underflow in financial calculations - Ensure all error paths properly revert state changes - Validate access control and permission checks - Flag any missing input validation on transaction parameters
Files:
core/kapp/validators/proto/validatorData.protocore/kapp/kappController/kapp.gocore/kapp/factory/validators.gocore/kapp/validators/validators.gocore/kapp/validators/versionAttestation.gocore/kapp/validators/peersUpdate.gocore/kapp/validators/versionAttestation_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions - Race condition coverage (tests should use -race flag patterns) - No hardcoded sleep for synchronization (use channels or sync primitives) - Test isolation (no shared mutable state between tests)
Files:
core/kapp/validators/versionAttestation_test.go
🧠 Learnings (2)
📚 Learning: 2026-04-21T20:12:22.959Z
Learnt from: phcarneirobc
Repo: klever-io/klever-go PR: 38
File: indexer/eventsProcessor.go:188-211
Timestamp: 2026-04-21T20:12:22.959Z
Learning: In Go structs that are JSON-marshaled, if a field is a `bool` and has the `json:"...,omitempty"` tag, then leaving that field at its zero value (`false`) is functionally equivalent (in the resulting JSON) to explicitly setting `Foundation: false`. Reviewers should not flag struct literals that omit such `bool` fields as an inconsistency; they will serialize identically because `omitempty` suppresses `false` values.
Applied to files:
core/interface.gofactory/stateComponents.goconfig/enableEpochs.gocore/fork/forks.gocommon/constants.gocore/kapp/kappController/kapp.gocmd/node/startup.gointegrationTest/mock/forkControllerStub.gocore/kapp/factory/validators.gocommon/mock/forkControllerStub.gocore/kapp/validators/validators.gocore/kapp/validators/versionAttestation.gocore/kapp/validators/peersUpdate.gocore/kapp/validators/versionAttestation_test.go
📚 Learning: 2026-05-23T22:52:58.065Z
Learnt from: fbsobreira
Repo: klever-io/klever-go PR: 65
File: data/blockchain/blockchain.go:170-172
Timestamp: 2026-05-23T22:52:58.065Z
Learning: In Go, the pattern `append([]byte(nil), src...)` should be treated as preserving nil identity when `src` is a nil `[]byte`: spreading a nil slice contributes zero variadic arguments, so `append` performs no allocation and returns the original nil destination slice unchanged (i.e., result is nil, not an empty non-nil slice). Do not flag this as an incorrect empty-slice conversion; it intentionally maintains `nil`.
Applied to files:
core/interface.gofactory/stateComponents.goconfig/enableEpochs.gocore/fork/forks.gocommon/constants.gocore/kapp/kappController/kapp.gocmd/node/startup.gointegrationTest/mock/forkControllerStub.gocore/kapp/factory/validators.gocommon/mock/forkControllerStub.gocore/kapp/validators/validators.gocore/kapp/validators/versionAttestation.gocore/kapp/validators/peersUpdate.gocore/kapp/validators/versionAttestation_test.go
🪛 ast-grep (0.44.1)
core/kapp/validators/versionAttestation.go
[warning] 207-207: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(satisfied)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🪛 Buf (1.71.0)
data/transaction/proto/contracts.proto
[error] 165-165: Field name "NodeVersion" should be lower_snake_case, such as "node_version".
(FIELD_LOWER_SNAKE_CASE)
core/kapp/validators/proto/validatorData.proto
[error] 29-29: Field name "AttestedVersion" should be lower_snake_case, such as "attested_version".
(FIELD_LOWER_SNAKE_CASE)
[error] 30-30: Field name "AttestedEpoch" should be lower_snake_case, such as "attested_epoch".
(FIELD_LOWER_SNAKE_CASE)
🔇 Additional comments (19)
core/kapp/validators/versionAttestation_test.go (4)
655-728: Positionalinfos[2]lookup was replaced with anOwnerAddressmatch (Lines 684-688), resolving the earlier flakiness concern. Remaining subtests are correct.
17-129: LGTM!
131-276: LGTM!
330-477: LGTM!common/mock/forkControllerStub.go (1)
20-20: LGTM!Also applies to: 61-62, 83-83, 102-102, 171-174
integrationTest/mock/forkControllerStub.go (1)
17-17: LGTM!Also applies to: 128-134
common/constants.go (1)
77-77: LGTM!factory/stateComponents.go (1)
26-29: LGTM!Also applies to: 31-38, 54-60, 98-104
core/kapp/kappController/kapp.go (1)
5-5: LGTM!Also applies to: 58-63, 74-75
core/kapp/factory/validators.go (1)
4-4: LGTM!Also applies to: 18-20, 22-28
core/kapp/validators/validators.go (1)
14-14: LGTM!Also applies to: 64-74, 85-91, 118-125, 543-557
core/kapp/validators/peersUpdate.go (2)
711-717: DuplicategetValidatorfetch per electable validator — already raised and declined.The prior review flagged that
computeVersionEnforcement's guard loop and the subsequentprocessValidatorEpochV1/V2loop both fetch the same validator record. Per the PR's comments summary, caching was intentionally declined to avoid coupling mutable state across two consensus-critical phases, given the read goes through the cached accounts layer. No action needed here.Also applies to: 740-746
315-347: LGTM! The demote/no-oscillation/restore state machine inresolveVersionEnforcedListandupdatePeerListStatusis internally consistent — verified elected validators only get demoted whendemoteis true, previously-demoted observers stay demoted until a satisfying attestation, and dormancy auto-restores them.config/enableEpochs.go (1)
31-31: LGTM!config/node/enableEpochs.yaml (1)
35-38: LGTM!core/fork/forks.go (1)
29-29: LGTM!Also applies to: 97-100, 141-143
core/interface.go (1)
85-85: LGTM!cmd/node/startup.go (1)
578-587: LGTM!core/kapp/validators/versionAttestation.go (1)
56-232: LGTM! The two-passrequiredVersionForEpochmax-tracking, semver comparison, and supermajority/floor guard math all check out correctly, and the previously-flagged silentgetValidatorfailure and brittle indexing issues are confirmed fixed.
| string AttestedVersion = 27 [json_name = "attestedVersion"]; | ||
| uint32 AttestedEpoch = 28 [json_name = "attestedEpoch"]; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Fix the new protobuf field naming before merge.
The added fields use PascalCase source names and fail Buf's FIELD_LOWER_SNAKE_CASE lint rule. Rename the source identifiers while preserving their field numbers and explicit JSON names, then regenerate protobuf bindings.
core/kapp/validators/proto/validatorData.proto#L29-L30: renameAttestedVersion/AttestedEpochtoattested_version/attested_epoch.data/transaction/proto/contracts.proto#L165-L165: renameNodeVersiontonode_version.
🧰 Tools
🪛 Buf (1.71.0)
[error] 29-29: Field name "AttestedVersion" should be lower_snake_case, such as "attested_version".
(FIELD_LOWER_SNAKE_CASE)
[error] 30-30: Field name "AttestedEpoch" should be lower_snake_case, such as "attested_epoch".
(FIELD_LOWER_SNAKE_CASE)
📍 Affects 2 files
core/kapp/validators/proto/validatorData.proto#L29-L30(this comment)data/transaction/proto/contracts.proto#L165-L165
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/kapp/validators/proto/validatorData.proto` around lines 29 - 30, Rename
the protobuf source fields AttestedVersion and AttestedEpoch to attested_version
and attested_epoch in core/kapp/validators/proto/validatorData.proto, preserving
field numbers and explicit JSON names; rename NodeVersion to node_version in
data/transaction/proto/contracts.proto, then regenerate all protobuf bindings.
Source: Linters/SAST tools
✅ Action performedReview finished.
|
d0acf7e to
4a61fa7
Compare
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/kapp/validators/proto/validatorData.proto (1)
29-30: 📐 Maintainability & Code Quality | 🟠 MajorThe new protobuf source fields still fail the same Buf lint rule.
Rename the source identifiers to lower_snake_case, preserve their field numbers and explicit JSON names, then regenerate the bindings.
core/kapp/validators/proto/validatorData.proto#L29-L30: renameAttestedVersion/AttestedEpochtoattested_version/attested_epoch.data/transaction/proto/contracts.proto#L165-L165: renameNodeVersiontonode_version.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/kapp/validators/proto/validatorData.proto` around lines 29 - 30, Rename the protobuf source fields AttestedVersion and AttestedEpoch to attested_version and attested_epoch in core/kapp/validators/proto/validatorData.proto, preserving field numbers and explicit JSON names; also rename NodeVersion to node_version in data/transaction/proto/contracts.proto. Regenerate all affected protobuf bindings.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/kapp/validators/versionAttestation_test.go`:
- Line 597: Update buildStorage to accept the active *testing.T as its first
parameter and use that parameter for all Fatal and require.NoError calls, rather
than capturing the outer test variable. Update all three buildStorage call sites
to pass their local t before v and the attestation map.
- Around line 330-477: Add a subtest to TestComputeVersionEnforcement that uses
attestationTestSetup, corrupts owner1’s stored validator record with invalid
protobuf bytes through the existing storage stub, and then calls
computeVersionEnforcement for epoch 10. Assert that enforcement.demote is false
while enforcement.active remains true, confirming getValidator failures are
treated as unsatisfied rather than counted.
---
Duplicate comments:
In `@core/kapp/validators/proto/validatorData.proto`:
- Around line 29-30: Rename the protobuf source fields AttestedVersion and
AttestedEpoch to attested_version and attested_epoch in
core/kapp/validators/proto/validatorData.proto, preserving field numbers and
explicit JSON names; also rename NodeVersion to node_version in
data/transaction/proto/contracts.proto. Regenerate all affected protobuf
bindings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b4781f58-f487-4f94-8971-b7970bd5f69e
⛔ Files ignored due to path filters (2)
core/kapp/validators/validatorData.pb.gois excluded by!**/*.pb.go,!**/*.pb.godata/transaction/contracts.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (17)
cmd/node/startup.gocommon/constants.gocommon/mock/forkControllerStub.goconfig/enableEpochs.goconfig/node/enableEpochs.yamlcore/fork/forks.gocore/interface.gocore/kapp/factory/validators.gocore/kapp/kappController/kapp.gocore/kapp/validators/peersUpdate.gocore/kapp/validators/proto/validatorData.protocore/kapp/validators/validators.gocore/kapp/validators/versionAttestation.gocore/kapp/validators/versionAttestation_test.godata/transaction/proto/contracts.protofactory/stateComponents.gointegrationTest/mock/forkControllerStub.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Verify that any new or modified concurrent code (goroutines, channels, mutexes, sync primitives) is free of race conditions. Check for: proper lock/unlock pairing, no goroutine leaks, correct channel lifecycle management, and proper context cancellation propagation.
Verify that errors are not silently discarded. Check for: unchecked error returns, error wrapping with context, proper error propagation up the call chain, and no bare panic() calls outside of init() functions.
Files:
config/enableEpochs.gocore/kapp/factory/validators.gocore/interface.gointegrationTest/mock/forkControllerStub.gocommon/constants.gocmd/node/startup.gofactory/stateComponents.gocommon/mock/forkControllerStub.gocore/kapp/kappController/kapp.gocore/fork/forks.gocore/kapp/validators/validators.gocore/kapp/validators/peersUpdate.gocore/kapp/validators/versionAttestation_test.gocore/kapp/validators/versionAttestation.go
core/kapp/**
⚙️ CodeRabbit configuration file
core/kapp/**: KApps (blockchain application layer) processes on-chain transactions. - Verify correct balance/state changes are atomic and consistent - Check for integer overflow/underflow in financial calculations - Ensure all error paths properly revert state changes - Validate access control and permission checks - Flag any missing input validation on transaction parameters
Files:
core/kapp/factory/validators.gocore/kapp/validators/proto/validatorData.protocore/kapp/kappController/kapp.gocore/kapp/validators/validators.gocore/kapp/validators/peersUpdate.gocore/kapp/validators/versionAttestation_test.gocore/kapp/validators/versionAttestation.go
data/**
⚙️ CodeRabbit configuration file
data/**: Core data structures and state management. - Verify protobuf serialization/deserialization is correct and backwards-compatible - Check for proper nil/empty checks on decoded data - Ensure trie operations maintain data integrity - Validate state transitions are deterministic
Files:
data/transaction/proto/contracts.proto
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions - Race condition coverage (tests should use -race flag patterns) - No hardcoded sleep for synchronization (use channels or sync primitives) - Test isolation (no shared mutable state between tests)
Files:
core/kapp/validators/versionAttestation_test.go
🧠 Learnings (2)
📚 Learning: 2026-04-21T20:12:22.959Z
Learnt from: phcarneirobc
Repo: klever-io/klever-go PR: 38
File: indexer/eventsProcessor.go:188-211
Timestamp: 2026-04-21T20:12:22.959Z
Learning: In Go structs that are JSON-marshaled, if a field is a `bool` and has the `json:"...,omitempty"` tag, then leaving that field at its zero value (`false`) is functionally equivalent (in the resulting JSON) to explicitly setting `Foundation: false`. Reviewers should not flag struct literals that omit such `bool` fields as an inconsistency; they will serialize identically because `omitempty` suppresses `false` values.
Applied to files:
config/enableEpochs.gocore/kapp/factory/validators.gocore/interface.gointegrationTest/mock/forkControllerStub.gocommon/constants.gocmd/node/startup.gofactory/stateComponents.gocommon/mock/forkControllerStub.gocore/kapp/kappController/kapp.gocore/fork/forks.gocore/kapp/validators/validators.gocore/kapp/validators/peersUpdate.gocore/kapp/validators/versionAttestation_test.gocore/kapp/validators/versionAttestation.go
📚 Learning: 2026-05-23T22:52:58.065Z
Learnt from: fbsobreira
Repo: klever-io/klever-go PR: 65
File: data/blockchain/blockchain.go:170-172
Timestamp: 2026-05-23T22:52:58.065Z
Learning: In Go, the pattern `append([]byte(nil), src...)` should be treated as preserving nil identity when `src` is a nil `[]byte`: spreading a nil slice contributes zero variadic arguments, so `append` performs no allocation and returns the original nil destination slice unchanged (i.e., result is nil, not an empty non-nil slice). Do not flag this as an incorrect empty-slice conversion; it intentionally maintains `nil`.
Applied to files:
config/enableEpochs.gocore/kapp/factory/validators.gocore/interface.gointegrationTest/mock/forkControllerStub.gocommon/constants.gocmd/node/startup.gofactory/stateComponents.gocommon/mock/forkControllerStub.gocore/kapp/kappController/kapp.gocore/fork/forks.gocore/kapp/validators/validators.gocore/kapp/validators/peersUpdate.gocore/kapp/validators/versionAttestation_test.gocore/kapp/validators/versionAttestation.go
🪛 ast-grep (0.44.1)
core/kapp/validators/versionAttestation.go
[warning] 207-207: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(satisfied)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🪛 Buf (1.71.0)
data/transaction/proto/contracts.proto
[error] 165-165: Field name "NodeVersion" should be lower_snake_case, such as "node_version".
(FIELD_LOWER_SNAKE_CASE)
core/kapp/validators/proto/validatorData.proto
[error] 29-29: Field name "AttestedVersion" should be lower_snake_case, such as "attested_version".
(FIELD_LOWER_SNAKE_CASE)
[error] 30-30: Field name "AttestedEpoch" should be lower_snake_case, such as "attested_epoch".
(FIELD_LOWER_SNAKE_CASE)
🔇 Additional comments (28)
common/constants.go (1)
77-77: LGTM!config/enableEpochs.go (1)
31-31: LGTM!config/node/enableEpochs.yaml (1)
35-38: LGTM!core/fork/forks.go (1)
29-29: LGTM!Also applies to: 97-99, 141-143
core/interface.go (1)
85-85: LGTM!cmd/node/startup.go (1)
579-586: LGTM!factory/stateComponents.go (1)
26-29: LGTM!Also applies to: 31-38, 53-60, 97-104
core/kapp/kappController/kapp.go (1)
5-5: LGTM!Also applies to: 58-63, 69-76
core/kapp/factory/validators.go (1)
4-4: LGTM!Also applies to: 13-28
core/kapp/validators/validators.go (1)
14-14: LGTM!Also applies to: 63-73, 85-91, 118-125, 543-557
core/kapp/validators/versionAttestation_test.go (6)
507-507:getKApperrors are still discarded here. The earlier round fixed theLoadPeercase but left these two; agetKAppfailure surfaces indirectly through the followinggetValidatorassertion instead of at the real failure point. As per coding guidelines: "Verify that errors are not silently discarded."🛡️ Proposed fix
- app, _ := v.getKApp() + app, err := v.getKApp() + require.NoError(t, err) val, err := v.getValidator(app, ownerAddress)Also applies to: 527-527
Source: Coding guidelines
17-79: LGTM!
81-129: LGTM!
131-276: LGTM!
278-328: LGTM!
656-729: LGTM!core/kapp/validators/versionAttestation.go (6)
11-46: LGTM!
48-87: LGTM!
89-122: LGTM!
124-146: LGTM!
148-208: LGTM!
210-232: LGTM!core/kapp/validators/peersUpdate.go (4)
315-329: LGTM!
333-343: LGTM!
480-480: LGTM!Also applies to: 511-511, 634-634, 669-669
711-714: LGTM!Also applies to: 740-743
common/mock/forkControllerStub.go (1)
20-20: LGTM!Also applies to: 61-62, 83-83, 102-102, 171-175
integrationTest/mock/forkControllerStub.go (1)
17-17: LGTM!Also applies to: 128-135
|
@coderabbitai review |
✅ Action performedReview finished.
|
TestClient_IdleConnectionReclaimedAtPongWait's httptest handler discarded upgrader.Upgrade's error via a bare return — a real upgrade failure would just time out the test with a confusing "idle client was not reclaimed" message instead of the actual cause. The handler runs on the HTTP server's own goroutine, not the test's, so calling t.Fatal there directly (as literally suggested) would violate testing's FailNow-must-run-on-the-test's-own-goroutine contract — the same class of bug already fixed twice elsewhere this session (PR klever-io#86's buildStorage, pre-existing code nick flagged on PR klever-io#96). Fixed properly instead: the handler sends the error on a buffered channel, and the main test goroutine's existing select gains a case that fails via t.Fatalf safely on its own goroutine. Audited; no findings. -race -count=40 on the targeted test and -count=10 on the full package, both clean.
TestClient_IdleConnectionReclaimedAtPongWait's httptest handler discarded upgrader.Upgrade's error via a bare return — a real upgrade failure would just time out the test with a confusing "idle client was not reclaimed" message instead of the actual cause. The handler runs on the HTTP server's own goroutine, not the test's, so calling t.Fatal there directly (as literally suggested) would violate testing's FailNow-must-run-on-the-test's-own-goroutine contract — the same class of bug already fixed twice elsewhere this session (PR klever-io#86's buildStorage, pre-existing code nick flagged on PR klever-io#96). Fixed properly instead: the handler sends the error on a buffered channel, and the main test goroutine's existing select gains a case that fails via t.Fatalf safely on its own goroutine. Audited; no findings. -race -count=40 on the targeted test and -count=10 on the full package, both clean.
* fix: eliminate data race on websocket keepalive timing vars
pingPeriod/pongWait were plain package-level vars, documented as
"nothing in production mutates them" — but tests do mutate them
(setKeepaliveForTest shortens them to exercise idle-client
reclamation quickly), concurrently with a live client's loopIn/loopOut
goroutines still reading the previous value on their way out.
TestClient_IdleConnectionReclaimedAtPongWait only waits for Done()
(ctx cancellation observed by a separate goroutine), not for loopIn/
loopOut to have actually returned, so the deferred restore's write
could race with loopOut's ticker-branch read. Reproduced directly:
`go test ./websocket/... -race -run TestClient_IdleConnectionReclaimedAtPongWait
-count=1` failed with a genuine DATA RACE report roughly 1 in 5-10 runs.
Fixes it at the root: pingPeriod/pongWait are now atomic.Int64-backed
(pingPeriodNs/pongWaitNs), read via getPingPeriod()/getPongWait()
accessors at every call site in client.go, and mutated via Swap/Store
in the test helper. 30 consecutive -race runs of the previously-flaky
test now pass cleanly (previously ~1-in-5-10 failure rate).
Confirmed via GitHub search that no open or closed PR/issue in this
repo already reports or fixes this race before starting the fix.
* fix: address review — handle discarded SetReadDeadline errors in loopIn
Two SetReadDeadline calls in loopIn discarded their error via `_ =`:
once before entering the read loop, once after each successful
ReadMessage. Both now log and exit through the existing deferred
cleanup (return before the loop, break inside it) instead of
continuing silently.
Strictly a hardening improvement, not just cosmetic: the pre-loop call
failing previously left no read deadline armed at all, which could
block the read indefinitely on a broken connection rather than being
reclaimed. The third SetReadDeadline call in this function (inside the
pong handler) already propagated its error correctly and was
untouched.
* fix: address review — swap keepalive ping/pong pair atomically as one unit
pingPeriodNs/pongWaitNs were two independent atomic.Int64 fields,
swapped separately in setKeepaliveForTest. A concurrent reader
(loopIn/loopOut computing a deadline) could observe a torn
combination — the old ping paired with the new pong, or vice versa —
which could momentarily violate the pongWait>pingPeriod invariant
during a test's swap.
Replaced with a single atomic.Pointer[keepaliveTimings] holding an
immutable {ping, pong} struct, swapped as one unit. Every reader now
gets a fully-formed snapshot from one atomic Load; no torn pair is
observable. Audited; no findings. -race -count=40 on the targeted
test and -count=10 on the full package, both clean.
* fix: address review — surface upgrade errors instead of swallowing them
TestClient_IdleConnectionReclaimedAtPongWait's httptest handler
discarded upgrader.Upgrade's error via a bare return — a real upgrade
failure would just time out the test with a confusing "idle client
was not reclaimed" message instead of the actual cause.
The handler runs on the HTTP server's own goroutine, not the test's,
so calling t.Fatal there directly (as literally suggested) would
violate testing's FailNow-must-run-on-the-test's-own-goroutine
contract — the same class of bug already fixed twice elsewhere this
session (PR #86's buildStorage, pre-existing code nick flagged on
PR #96). Fixed properly instead: the handler sends the error on a
buffered channel, and the main test goroutine's existing select gains
a case that fails via t.Fatalf safely on its own goroutine.
Audited; no findings. -race -count=40 on the targeted test and
-count=10 on the full package, both clean.
* fix: address review — resolve keepalive timings once at hub construction
Move pingPeriod/pongWait from a runtime-mutable atomic.Pointer onto
resolvedLimits, set once in Limits.resolve() at NewHub time and never
mutated afterward. Production never touched these values after
construction anyway — the only writer was a test shortening them for a
fast-reclamation check, which now builds a dedicated hub with custom
Limits instead of mutating shared state. This eliminates the race by
construction rather than by synchronization, and removes the
atomic.Pointer/keepaliveTimings/getter machinery entirely.
That machinery's own doc comment also overclaimed what it delivered:
"a reader can never observe a torn combination of an old ping with a new
pong" wasn't actually true as used, since getPingPeriod()/getPongWait()
were two independent Loads consumed by different goroutines (loopIn only
read pong, loopOut only read ping) — a Swap between those two reads could
produce exactly the mixed pair the comment ruled out. The atomic.Pointer
did fix the literal data race but not that claim; removing the machinery
sidesteps the question rather than patching the comment.
Also: downgrade one loopIn log line from Warn to Debug (fires only on a
benign concurrent-close teardown race, not an anomaly — the actual
unexpected-close signal a few lines above is untouched); simplify
TestClient_IdleConnectionReclaimedAtPongWait's upgrade-error handling
from a buffered channel to a direct t.Errorf call, since t.Errorf (unlike
t.Fatal/FailNow) is documented as goroutine-safe; add resolve() tests for
the new fields' defaults, overrides, and the pongWait<=pingPeriod clamp
(mutation-tested).
Validators can attest their running node version on-chain via the ValidatorConfig transaction (new NodeVersion field). At end-of-epoch processing, when the versionAttestation fork is active and the versions.versionsByEpochs config requires a specific version for the new epoch, validators without a satisfying attestation are demoted to the observer list instead of staying electable - no jailing, no rating loss, no unjail fee. They are restored to eligible automatically at the first end-of-epoch after attesting a satisfying version. Safety: demotion is only enforced when at least 2/3 of the active set (elected + eligible + waiting) already attested a satisfying version, so enforcement can never demote enough validators to endanger consensus liveness. The mechanism is fully dormant while the fork flag is unset or the versions config carries the "*" wildcard.
- Guard population: count the 2/3 supermajority over the electable set only (elected + eligible). Waiting validators can neither dilute nor inflate the ratio, closing the waiting-attester amplification vector. - Floor guard: never demote when the attested electable set falls below the nodes shuffler minimum (genesis MinNumberOfNodes, wired through StateComponentsFactoryArgs), so demotion cannot trigger ErrSmallElectedListSize / ErrListSizeZero and halt the network. - Attestation freshness: an attestation only counts if made at or after the start epoch of the previous versionsByEpochs entry. A one-time inflated attestation (e.g. "99.9.9") now expires after one release cycle instead of granting a permanent exemption (AttestedEpoch is now read, not just stored). - Hysteresis: when the requirement is active but the guards lapse, no new demotions happen and unattested observers stay demoted, removing the demote/restore oscillation. Attested observers always restore; disabling the mechanism restores everyone. - Semver: pre-release attestations (1.9.0-rc1) no longer satisfy the release requirement (1.9.0) on equal numeric components. - Documented why pre-fork NodeVersion is ignored rather than rejected (any observable difference would fork state before activation).
- extract resolveVersionEnforcedList helper from updatePeerListStatus (reduces nesting, makes the enforcement decision unit-testable) - log getValidator failures in the enforcement guard loop instead of a bare continue (still fail-closed: undercounting keeps demotion off) - test: locate owner3 by OwnerAddress instead of relying on map-derived slice order in the demote-and-restore round-trip
buildStorage discarded LoadPeer's error before immediately dereferencing the result via SetList — a failure would have surfaced as an opaque nil-pointer panic instead of a clear test failure.
…failure coverage buildStorage closed over the outer test's *testing.T instead of the running subtest's — t.Fatal/require.NoError inside it ran against the wrong test when called from t.Run subtests, which testing's FailNow semantics don't guarantee is safe. Now takes t as an explicit parameter (t.Helper(), all three call sites updated), and no longer discards errors from getKApp/setValidator/setValidatorBuckets. Also adds a regression test pinning computeVersionEnforcement's fail-closed behavior on a getValidator failure (logged + treated as not-satisfied, never counted towards the demotion supermajority) — verified non-tautological by temporarily mutating the production code to count a failed load as satisfied, confirming the new subtest (and only that one) failed, then reverting. Audited (correctness + security) given this touches the same guard that previously caught a chain-halt-class defect in this PR's history; no findings.
b50bb82 to
85f95ca
Compare
MathijsBok
left a comment
MathijsBok
left a comment
There was a problem hiding this comment.
Review of the rebased branch (85f95ca)
Thanks for the rebase, it landed cleanly on the current develop tip and the branch is mergeable again. This is a full review of the feature as it now stands: every changed file read in full, the package tests and complexity metrics run against both this head and the merge base.
Overall: the mechanism is well built at the unit level. The attestation write path is correctly bound to the authenticated sender, AttestedEpoch is taken from the block header rather than the submitter, the pre-fork "ignore, do not reject" reasoning for NodeVersion is sound and avoids a pre-activation state fork, the guards fail closed on unreadable records, the fork wiring is complete across all four ForkController implementers, and the epoch alignment is exact. Test coverage of the new logic is 100% line coverage on every new function, and package coverage rises from 69.2% to 71.4%.
Requesting changes for one blocking item and a small number of correctness and hygiene points below.
Blocking
- F1 (critical): the demotion guards can empty the elected list, which permanently stalls epoch preparation. This is the open issue #132, and it is still unmitigated on this head. Details inline.
Non-blocking findings
| ID | Severity | Location | Finding |
|---|---|---|---|
| F2 | 🟡 Warning | versionAttestation.go:145 |
versionSatisfies ignores prerelease identifiers when the numeric parts are equal |
| F5 | 🟡 Warning | validators.go:547 |
Attestation-only transactions silently reset commission and delegation settings |
| F6 | 🟡 Warning | contracts.pb.go:5 |
Generated files produced with a non-canonical protoc invocation (~2000 lines of churn) |
| F7 | 💡 Suggestion | versionAttestation_test.go:531 |
No test pins that AttestedEpoch is recorded from the block header |
| F8 | 💡 Suggestion | versionAttestation_test.go:528 |
getKApp error discarded in two places |
| F9 | 💡 Suggestion | peersUpdate.go:714 |
The only uncovered lines this PR touches: error propagation in both epoch loops |
| F10 | 💡 Suggestion | versionAttestation.go:56 |
The NOTE about agreeing with headerCheck is not pinned by a test |
Additional notes (no action required now)
- Complexity:
requiredVersionForEpochandcomputeVersionEnforcementboth measure cognitive complexity 13 against this repo's threshold of 15, so each is two increments away from a quality gate finding. Given that both are consensus-critical and likely to gain guards later, extracting the tally loop out ofcomputeVersionEnforcement(roughly 6 of its 13 points) would buy useful headroom.UpdateValidatoris covered separately in F5. - Parameter counts:
processValidatorEpochV1/V2now take 7 parameters andupdatePeerListStatus6. Nothing exceeds a hard limit, but the next per-epoch input will force the issue; bundling the per-epoch invariants into a single struct would be the natural move at that point (not now). - Log levels: the
version demotion skipped: safety guards not metandversion demotion activelines are at Debug. These fire at most once per epoch and describe a state operators will want to see in an incident; Info would fit them better. - Integration coverage:
integrationTest/processorNodeandkvm/mock/worldconstructArgsNewKAppwithout the new fields, so enforcement is dormant there. That is fine for correctness (zero values disable the feature) but it does mean demotion has no integration-level coverage.
Separately, I am sending you a short document by email covering a few further points that we do not discuss in public per our security policy. Nothing there blocks this PR on its own, but one of the items is closely related to F1 and worth reading before you rework the guards.
|
|
||
| hasSupermajority := electable > 0 && | ||
| satisfied*versionEnforcementDenominator >= electable*versionEnforcementNumerator | ||
| holdsFloor := v.minElectableNodes == 0 || uint32(satisfied) >= v.minElectableNodes // #nosec G115 |
There was a problem hiding this comment.
🔴 Critical (F1): the guards cannot prevent an empty elected list, which permanently stalls the network
This is open issue #132, filed on 2026-08-13 and still unmitigated on this head.
Both guards reason about the combined electable set: electable and satisfied count ElectedList and EligibleList into the same counters (lines 183-207), and this floor compares that combined satisfied against minElectableNodes. Nothing constrains the elected sublist on its own.
sharding/nodesCoordinator.go:942-944 aborts the entire epoch computation when the elected list alone is empty:
if len(electedList) == 0 {
return nil, fmt.Errorf("%w elected list size is zero. No validators found", ErrListSizeZero)
}Failure scenario that passes both guards. 10 elected validators, all outdated; 30 eligible validators, all attested. electable = 40, satisfied = 30. Supermajority holds (30*3 = 90 >= 40*2 = 80), and the floor holds for any minElectableNodes <= 30. Demotion proceeds, all 10 elected validators become observers, and the elected list is empty.
Why this does not recover. EpochStartPrepare logs could not compute nodes config from list and returns at sharding/nodesCoordinator.go:762-766, so nodesConfig[newEpoch] is never built. Every subsequent ComputeConsensusGroup for that epoch then fails with ErrEpochNodesConfigDoesNotExist (:511-513), so no header of the new epoch validates on any intake path. Nothing ever closes that window, on any node.
This is not an exotic configuration: the elected set is the long-running incumbent population, which is exactly the group most likely to lag a mandatory upgrade.
Also worth correcting: the doc comment on this function states that the floor means demotion "can never reduce elected+eligible below what EpochStartPrepare needs". EpochStartPrepare needs the elected list specifically to be non-empty, which no combined count can guarantee.
Suggested direction. Either count the elected sublist separately here and refuse to demote when doing so would empty it, or change the coordinator to tolerate an empty elected list when the eligible list can still fill consensusGroupSize (the shuffler runs immediately afterwards at :768-781 and repopulates elected from eligible). Issue #132 deliberately leaves that ownership question open, so please say which side you would rather fix; we are happy to take the coordinator half in a separate PR after this one lands. Whichever side is chosen, the abort branch needs a test.
Ref: CWE-691 (insufficient control flow management)
There was a problem hiding this comment.
Fixed in b5a3212: computeVersionEnforcement now carries a third guard, preservesElectedList, that refuses demotion whenever it would leave the elected sublist with zero satisfying members. The guard is evaluated over a live re-derivation of each validator's current list membership (peerAcc.GetList()) rather than the epoch-start validatorInfos snapshot, so a validator reclassified earlier in the same epoch boundary (e.g. jailed by the ratings pass) can no longer inflate the guards either. Covered by new subtests in TestComputeVersionEnforcement ("demotion refused when it would empty the elected list", "demotion proceeds when at least one elected validator satisfies", "stale snapshot: a since-jailed validator does not inflate the guards"), each mutation-tested.
A follow-up commit, bf65553, further hardens the same function: unreadable validator/peer-account records were being excluded from the guard denominators entirely rather than counted as unsatisfied-but-electable, which could both shrink the supermajority guard's denominator and let the elected-list guard pass vacuously if the sole elected validator's record happened to be unreadable. Both are now fail-closed, pinned by two more subtests.
| } | ||
| } | ||
|
|
||
| return !attestedPre || requiredPre |
There was a problem hiding this comment.
🟡 Warning (F2): prerelease identifiers are never compared, so an older release candidate satisfies a newer one
When the three numeric components are equal, this line decides the outcome purely on the presence of a prerelease suffix on either side. The identifiers themselves (rc1, rc2, alpha) are dropped by parseSemver, so any prerelease satisfies any other prerelease of the same numeric version.
Verified against this head:
versionSatisfies("v1.9.0-rc1", "v1.9.0-rc2") == true
versionSatisfies("v1.9.0-alpha", "v1.9.0-rc1") == true
Both should be false: per semver, 1.9.0-alpha < 1.9.0-rc1 < 1.9.0-rc2. This contradicts the documented contract of the function ("semantic compare (attested >= required)").
Why it is reachable. Required entries are capped at core.MaxSoftwareVersionLengthInBytes (10 bytes), and v1.9.0-rc2 is exactly 10 bytes, so a prerelease requirement is a valid config entry, and our release tags do use rc suffixes. The effect is deterministic across nodes, so this is an enforcement gap rather than a divergence risk: validators still on an older rc of the same version keep their slots when a newer rc is required.
Suggested fix. Either compare prerelease identifiers per semver 11.4, or, simpler and adequate for a 10-byte domain, fall back to exact string match whenever the required version carries a prerelease suffix. The current test table covers prerelease against release in both directions but has no prerelease against prerelease case, which is why this slipped through; worth pinning ("v1.9.0-rc1", "v1.9.0-rc2") == false and ("v1.9.0-rc2", "v1.9.0-rc1") == true once fixed.
There was a problem hiding this comment.
Fixed in 088195c: parseSemver now returns the prerelease identifier string itself (not just a presence flag), and a new comparePrerelease implements semver.org section 11.4 ordering — numeric fields compare numerically, alphanumeric fields as ASCII strings, numeric always outranks lower than alphanumeric, and a longer field list outranks an equal-prefix shorter one when both sides carry a prerelease. versionSatisfies now calls into it instead of just checking presence. Covered by a new TestComparePrerelease table and additional `TestVersionSatisfies" cases including your exact pinned examples (rc1/rc2/alpha).
| // fork: old binaries accept the unknown field as Transaction_Ok, so any observable | ||
| // difference (error code or receipt) would fork state pre-activation. Operators must | ||
| // attest at or after the fork epoch for the attestation to be recorded. | ||
| if len(tc.GetConfig().GetNodeVersion()) > 0 && v.forkController.VersionAttestation() { |
There was a problem hiding this comment.
🟡 Warning (F5): attestation-only transactions silently reset commission and delegation settings
This is about the interaction between the new block and the three unconditional assignments just below it (lines 559-561), which are pre-existing:
val.CanDelegate = tc.GetConfig().GetCanDelegate()
val.Commission = tc.GetConfig().GetCommission()
val.MaxDelegation = tc.GetConfig().GetMaxDelegationAmount()Unlike RewardAddress, Logo, Name and URIs, which are all guarded by len(...) > 0 checks, these three are overwritten unconditionally, so an absent field means zero.
Before this PR that was mostly harmless, because ValidatorConfig transactions were occasional and deliberate. This PR makes them a recurring operational requirement: every validator has to submit one at least once per release cycle to stay electable. An operator, or a script, that sends the minimal attestation transaction (BLSPublicKey plus NodeVersion) will silently set commission to 0, set CanDelegate to false and clear MaxDelegation. There is no error and no receipt indicating it happened, and the economic consequences are real.
Suggested fix. At minimum, document that an attestation transaction must echo the validator's full current config, and make sure the official tooling does so. A cleaner long-term option, behind a future fork, is to treat zero values for these three fields as "no change", matching how the string fields already behave, or to give attestation its own contract type.
Separately, on this same block: it adds 5 points of cognitive complexity to UpdateValidator, taking it from 59 to 64 against this repo's threshold of 15. The added lines are themselves fully covered by the new tests, so this is purely about compounding an already oversized function. Extracting the block into a small helper (for example applyNodeVersionAttestation) removes the whole delta at zero behavior change and gives the attestation logic a directly testable unit. There is recent precedent on develop for exactly this kind of split (cf516ee, done to bring createNodesCoordinator under the threshold).
There was a problem hiding this comment.
Fixed in f1d5148: the inline NodeVersion-attestation block was extracted into its own applyNodeVersionAttestation method (bringing UpdateValidator back under the complexity threshold), and a comment now documents that CanDelegate/Commission/MaxDelegation are unconditionally overwritten on every UpdateValidator call, so callers (including attestation-only transactions) must echo the full config or those fields get zeroed.
| // protoc v5.29.3 | ||
| // source: contracts.proto | ||
| // protoc v6.33.1 | ||
| // source: data/transaction/proto/contracts.proto |
There was a problem hiding this comment.
🟡 Warning (F6): generated files were produced with a non-canonical protoc invocation
Both regenerated files (data/transaction/contracts.pb.go and core/kapp/validators/validatorData.pb.go) embed repo-root-relative source paths and were built with protoc v6.33.1, whereas the checked-in generation directives register the descriptors under bare filenames with protoc v5.29.3:
data/transaction/transaction.go:2://go:generate protoc -I=proto ... contracts.protocore/kapp/validators/validatorData.go:1: same pattern forvalidatorData.proto
Two consequences:
- Every internal symbol was renamed (
file_contracts_proto_*becomesfile_data_transaction_proto_contracts_proto_*), which inflates this diff by roughly 2000 mechanical lines and makes the genuine changes hard to find. The next person who runs the canonicalgo:generatewill revert all of it, producing churn and merge conflicts. - The protobuf registry file path changes. That path is a public identifier for anything resolving these descriptors by name through
protoregistryor reflection. Nothing in this repo does today, but it is an unnecessary change to make silently.
The field additions themselves are correct and wire-compatible: fresh field numbers (ValidatorConfig.NodeVersion = 9, ValidatorData.AttestedVersion = 27, AttestedEpoch = 28), consistent json_name values, and accessors matching the .proto sources.
Suggested fix. Regenerate both files using the existing //go:generate directives, with protoc invoked as -I=proto plus the bare filename, and keep the protoc version aligned with the rest of the team. The diff should then shrink to just the new fields and their accessors.
There was a problem hiding this comment.
Fixed in 3c0eb2d: regenerated both files using the canonical invocation (protoc -I=proto ... <bare-filename>.proto, run from each proto's own package directory, matching the checked-in //go:generate directives), which brought the diffs down to just the intended new fields — 43 lines for validatorData.pb.go, 15 for contracts.pb.go. The protoc version used locally (v33.1) still differs from the original generation's (v5.29.3) in the header comment, which is an accepted cosmetic difference; the registry-path/symbol-renaming churn you flagged is gone.
| // NOTE: this table is the same release-shipped config validated and consumed by | ||
| // headerCheck.headerIntegrityVerifier; both lookups must keep agreeing on which | ||
| // entry covers an epoch. | ||
| func (v *validatorsKApp) requiredVersionForEpoch(epoch uint32) (string, uint32, bool) { |
There was a problem hiding this comment.
💡 Suggestion (F10): the NOTE above this function states an invariant that nothing enforces
The comment correctly flags that this lookup and headerCheck.headerIntegrityVerifier must keep agreeing on which versionsByEpochs entry covers an epoch. They are, however, two independent implementations with different assumptions:
headerCheck.getMatchingVersion(core/process/headerCheck/headerIntegrityVerifier.go:103-120) operates on input thatprepareVersionshas already sorted and validated (first entry at epoch 0, strictly ascending, length checked).requiredVersionForEpochtolerates unsorted input and a missing epoch-0 entry, and additionally tracks the previous entry.
Token-level duplication is zero, so no static analysis will catch drift between them. A single agreement test would turn the comment into an enforced property: feed the same non-trivial versionsByEpochs fixture to both lookups and assert they select the same entry for a range of epochs.
For context, the semver logic itself duplicates nothing: no numeric version comparator existed before this PR (core/statistics/softwareVersion does plain string comparison, headerCheck does byte equality), so parseSemver and versionSatisfies are genuinely new ground.
There was a problem hiding this comment.
Fixed in a555277: added TestRequiredVersionForEpoch_AgreesWithHeaderIntegrityVerifier, which feeds the same (deliberately unsorted) versionsByEpochs table to both requiredVersionForEpoch and headerIntegrityVerifier.GetVersion and asserts they select the same entry across a representative epoch range, turning the NOTE comment into an enforced property.
| app, _ := v.getKApp() | ||
| val, err := v.getValidator(app, ownerAddress) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, "v1.9.0", val.AttestedVersion) |
There was a problem hiding this comment.
💡 Suggestion (F7): nothing pins that AttestedEpoch is recorded from the block header
This test asserts AttestedVersion only. No test in the suite covers the other half of what UpdateValidator writes at validators.go:556:
val.AttestedEpoch = ctx.Block().GetHeader().GetEpoch()That field is what the entire freshness mechanism depends on. The stale-attestation tests construct ValidatorData.AttestedEpoch by hand, so they exercise the comparison but not the recording. A regression in the recording path (storing 0, or reading the wrong header field) would leave the whole suite green while silently disabling per-release re-attestation in production: every attestation would look either permanently fresh or permanently stale, depending on the table.
Suggested fix. Configure a nonzero epoch on the mocked block header in this test's context and assert it round-trips:
| assert.Equal(t, "v1.9.0", val.AttestedVersion) | |
| assert.Equal(t, "v1.9.0", val.AttestedVersion) | |
| assert.Equal(t, expectedEpoch, val.AttestedEpoch) |
There was a problem hiding this comment.
Fixed in 16cba7d: the "attestation is stored when fork is active" subtest now builds its own KappContext with a nonzero block epoch (rather than relying on the shared addContext helper's zero default) and asserts val.AttestedEpoch equals it — with the shared helper's zero epoch, a broken recording and a correct one were indistinguishable, which this fixes.
| require.NoError(t, err) | ||
| require.Equal(t, transaction.Transaction_Ok, resultCode) | ||
|
|
||
| app, _ := v.getKApp() |
There was a problem hiding this comment.
💡 Suggestion (F8): getKApp error discarded here and at line 548
Every other getKApp call in this file checks its error (the restore flow later in the file uses require.NoError), and the project convention is that errors are never silently discarded, test setup included. If getKApp failed here, the following getValidator would fail with a confusing nil-handler error rather than pointing at the real cause.
| app, _ := v.getKApp() | |
| app, err := v.getKApp() | |
| require.NoError(t, err) |
The same applies to the identical line in the "attestation is ignored before the fork" subtest at line 548.
There was a problem hiding this comment.
Fixed in 16cba7d: both call sites now use app, err := v.getKApp() with require.NoError(t, err) instead of discarding the error.
|
|
||
| for _, validatorInfo := range validatorInfos { | ||
| if err := v.processValidatorEpochV1(app, validatorInfo, currentEpoch, minSelfDelegated, minTotalDelegated, totalDelegations); err != nil { | ||
| if err := v.processValidatorEpochV1(app, validatorInfo, currentEpoch, minSelfDelegated, minTotalDelegated, totalDelegations, enforcement); err != nil { |
There was a problem hiding this comment.
💡 Suggestion (F9): these are the only lines this PR touches that no test covers
The return err bodies inside both epoch loops (lines 714-716 here, and 743-745 in ProcessEconomicsEndOfEpochV2) have zero coverage. The gap predates this PR, but because the call lines change here (the new enforcement argument), they now land in the new-code coverage denominator that the quality gate measures. Every other executable line this PR touches in non-generated code is covered.
The volume is small (two statements), so this is a cheap fix rather than an important one: one test per version that makes processValidatorEpochV1/V2 fail, for example through a peer account load error, and asserts the error surfaces from ProcessEconomicsEndOfEpochV1/V2. There is a recent precedent for this shape of test on develop (94bc4b1, pinning SetNodes error propagation).
There was a problem hiding this comment.
Fixed in 7c71910: added TestProcessEconomicsEndOfEpoch_PropagatesPerValidatorError, table-driven over V1/V2, which forces LoadPeerCalled to return an error and asserts it surfaces via require.ErrorIs from ProcessEconomicsEndOfEpoch. Mutation-tested by swallowing the loop error in each version independently to confirm the test actually catches the regression.
…and snapshot staleness Two gaps in computeVersionEnforcement's safety guards, both able to leave the elected list empty at epoch preparation (sharding.computeNodesConfig FromList aborts entirely when that happens, permanently stalling the node until corrected — see issue klever-io#132): - The guards only ever reasoned about the combined elected+eligible count, so demotion could legally remove every currently-elected validator as long as the combined supermajority/floor still held. Added a third guard: refuse demotion whenever it would leave zero satisfying validators in the elected sublist specifically. - validatorInfos is a snapshot taken at epoch-start; a validator can be jailed (or otherwise reclassified) by earlier same-boundary processing before this function runs, so a stale "still electable" snapshot entry could inflate both guards with a validator that will not actually remain in the electable population. The tally now re-checks each validator's live peer account instead of trusting the snapshot's list field. Extracted the tally loop into tallyElectableVersions to keep computeVersionEnforcement's own complexity down while adding the third guard. Added regression tests for both gaps plus the elected-list guard's positive case, and extended the shared test setup to let a test diverge a validator's live peer account from its snapshot entry.
…presence versionSatisfies decided prerelease-against-prerelease purely on whether either side had a suffix at all, so any prerelease satisfied any other prerelease of the same numeric version (e.g. "v1.9.0-rc1" satisfied a "v1.9.0-rc2" requirement) — contradicting the function's own documented "semantic compare" contract, and letting a validator on an older release candidate keep its slot when a newer one is required. parseSemver now returns the actual pre-release identifier string instead of a presence bool, and a new comparePrerelease implements semver.org section 11.4 ordering (numeric fields compare numerically, alphanumeric fields compare as strings, numeric always ranks below alphanumeric, more fields ranks higher when the shared prefix is equal). Release-vs-prerelease behavior at equal numeric core is unchanged.
…extract applyNodeVersionAttestation CanDelegate/Commission/MaxDelegation are overwritten unconditionally from the transaction, unlike RewardAddress/Logo/URIs/Name/NodeVersion which are all guarded by a len(...)>0 check — an absent field means zero. This PR makes ValidatorConfig transactions a recurring operational requirement (attest at least once per release cycle to stay electable), so a minimal attestation-only transaction now silently resets those three fields where before it was an occasional, deliberate action. Documented the behavior at the call site so it can't be missed by the next person reading this code, and it needs to be reflected in operator tooling. Also extracted the NodeVersion attestation block into its own applyNodeVersionAttestation method — no behavior change, keeps UpdateValidator's complexity from growing further.
…cal protoc invocation Both files had been regenerated with a repo-root-relative source path instead of running protoc from the package directory per the checked-in //go:generate directives (-I=proto, bare filename). That renamed every internal symbol (file_contracts_proto_* became file_data_transaction_proto_contracts_proto_*), inflating the diff by ~2000 mechanical lines and changing the protobuf registry's file-path identifier for no reason. Regenerated both using the actual go:generate directives; the diff now contains only the new fields (ValidatorConfig.NodeVersion, ValidatorData.AttestedVersion/AttestedEpoch) and their accessors.
…d getKApp errors The "attestation is stored" test only asserted AttestedVersion; the block header's Epoch was left at its zero-value default via the shared addContext helper, so a broken recording (storing 0, or reading the wrong header field) would leave the whole suite green while silently disabling per-release re-attestation. Override the context with a nonzero epoch and assert it round-trips onto val.AttestedEpoch. Also fixed two getKApp() calls in this test that discarded their error, inconsistent with the rest of the file (and the project convention that errors are never silently discarded, test setup included).
…loops ProcessEconomicsEndOfEpochV1/V2's `return err` bodies inside the per-validator loop had zero coverage. Added one test per fork version that forces a peer-account load failure and asserts it surfaces from ProcessEconomicsEndOfEpoch, instead of being silently absorbed. Mutation- tested both independently (swallowing the error in either loop makes its own subtest fail, confirming each is pinned separately).
…ersion lookup Add a test that feeds the same versionsByEpochs table to both requiredVersionForEpoch and headerIntegrityVerifier.GetVersion and asserts they select the same entry across a range of epochs, turning the existing NOTE comment about the two lookups needing to agree into an enforced property.
requiredVersionForEpoch silently tolerates a malformed or unsorted versionsByEpochs table by just resolving ambiguous entries rather than failing, relying on headerCheck's own validation of the same table having already run first. Validate the table directly in NewValidatorKApp (first entry at epoch 0, strictly increasing StartEpoch, version length cap) so a misconfigured table is caught locally rather than depending on construction-order elsewhere. Also documents that this config becomes consensus-critical once version enforcement is active, not just an input to graceful header-version degradation.
minAttestedEpoch is the start of the release cycle preceding the required one, and the comparison is inclusive, so a single attestation actually covers two release cycles before going stale, not one as the comment previously claimed. Update the comment to match the existing behavior.
NodeVersion is self-declared by the transaction sender and stored without any binding to the software actually running. Document at the write site that attestation is an operability signal for catching operators who haven't attested at all, not a guarantee of what binary a validator runs.
Operators need to see when demotion is skipped or goes active without needing debug-level logging enabled during an incident.
tallyElectableVersions excluded a validator entirely from tally.electable/ tally.elected whenever its record or peer account failed to load, rather than counting it as unsatisfied-but-electable as the surrounding comment claimed. That shrinks the supermajority guard's denominator (an unreadable record could inflate the satisfied ratio instead of lowering it) and can let the elected-list-preservation guard vacuously pass if the sole elected validator's record happens to be unreadable at tally time. Count load failures towards the electable/elected totals using the epoch-start snapshot's classification as a conservative fallback, matching the fail-closed behavior the comment already described. Also fixes comparePrerelease returning a nonzero result for numerically equal fields that differ only in leading zeros (e.g. "01" vs "1"), which broke its documented three-way compare contract, and tightens a couple of doc comments (validateVersionsByEpochs' divergence from headerCheck on an empty table, and the intentional stall trade-off in the elected-list guard).
|
Pushed a round of fixes for the rebased-branch review ( F1 (blocking): F2, F5, F6, F7, F8, F9, F10: all fixed individually, see inline replies for details on each. Log levels: Parameter counts / integration coverage: left as-is per your own note that these aren't blocking now. While re-testing the F1 fix I found (and fixed, with new mutation-tested coverage) a related gap: Full test suite is green ( |
2 participants
feat: validator node version attestation with observer demotion
Problem
Today, "mandatory" node updates are only latently enforced. An epoch flag activates a fork, but a validator running an outdated binary keeps producing identical blocks until the changed code path is actually exercised by a transaction. At that unpredictable moment, all outdated validators fork off simultaneously, miss blocks, bleed rating and get jailed — with an unjail fee — despite the operator having had no on-chain signal that they were at risk. The penalty is real but delayed and concentrated into "one bad day" (recent example: the long-outdated "Jon-Snow" node staying electable through several mandatory updates).
There is currently no provable, chain-data-based way to know a validator's version: p2p handshakes and endpoint checks are unauthenticated and not consensus data, so nothing can be safely gated on them — as correctly pointed out by the team in past discussions. This PR makes the version a piece of signed chain data and uses it leniently.
Approach
1. On-chain version attestation.
ValidatorConfiggets a new optionalNodeVersionfield (field 9). The validator owner attests the running node version with a normal, signed ValidatorConfig transaction; it is stored inValidatorData(AttestedVersion,AttestedEpoch). This answers "don't trust, verify": the requirement source and every attestation are on-chain and owner-signed.2. Observer demotion instead of latent jailing. During end-of-epoch processing (
ProcessEconomicsEndOfEpochV1/V2→updatePeerListStatus), when a specific version is required for the new epoch, validators whose attested version does not satisfy it are moved to the (previously unused for this purpose)observerlist:computeNodesConfigFromListignores observers),eligibleautomatically at the first end-of-epoch after attesting a satisfying version.The operator loses rewards while outdated — nothing else. Jail semantics for actual misbehavior are untouched.
3. Required version comes from the existing
versions.versionsByEpochsconfig — the same release-shipped table already consumed byheaderIntegrityVerifier. A release that ships{ startEpoch: E, version: "vX.Y.Z" }thereby declares the mandatory version for epoch E; the shipped-by-default"*"wildcard keeps the whole mechanism dormant. Satisfaction is semver-aware (attested >= required), so early upgraders are never demoted; non-parsable tags fall back to exact match.4. Liveness guards. Demotion is only enforced when BOTH hold, measured over the electable set (elected + eligible — exactly the population demotion removes from; waiting/inactive/jailed validators neither count nor get demoted):
MinNumberOfNodes, wired throughStateComponentsFactoryArgs), so demotion can never triggerErrSmallElectedListSize/ErrListSizeZero.When the guards don't hold, nothing is demoted (log line only) — and already-demoted observers stay demoted until they attest, so enforcement cannot oscillate. The on-chain attestation share doubles as a rollout signal before the epoch arrives.
4b. Attestation freshness. An attestation only counts if it was made at or after the start epoch of the previous
versionsByEpochsentry — i.e. validators must re-attest once per release cycle. A one-time inflated attestation (99.9.9) cannot grant a permanent exemption, andAttestedEpochis consensus-relevant, not decorative. Pre-release tags (1.9.0-rc1) do not satisfy a release requirement (1.9.0).5. Fork-gated. Everything is behind a new
versionAttestationenable-epoch flag (enableEpochs.yaml). Attestations are not even written to state before the flag epoch, so pre-fork state stays byte-identical for old binaries (proto3 unknown fields are ignored by old nodes; new nodes ignore the field until the flag).What this changes for operators
After updating, run one
ValidatorConfigtx withnodeVersionset (tooling/SDK can automate this on node startup). Not attesting after a mandatory release ⇒ demoted to observer at the mandatory epoch (if 2/3 attested) ⇒ no rewards until attested, then automatic return. Attesting falsely (new version string, old binary) ⇒ same risk profile as today: jailed when the fork path triggers. Incentives favor honesty; honest-but-slow operators are protected from jail.Adversarial review before submission
Two independent audit passes (correctness/logic and security/consensus-safety) were run against the initial implementation; both confirmed determinism, epoch alignment, replay protection, jail-flow isolation and proto wire-compat, and surfaced four real issues that are fixed in this PR's second commit:
MinNumberOfNodesfloor guard.attested >= requiredwith no freshness check made a one-time99.9.9attestation a permanent exemption. Fixed by the per-release-cycle freshness rule.1.9.0-rc1satisfied1.9.0. Fixed (semver pre-release ordering).Determinism & safety notes
versionsByEpochs,enableEpochs) — the same trust model as gas schedule configs. Order-independent: the supermajority is counted in a read-only pre-pass; per-validator demotion depends only on that validator's own data.Transaction_Ok, so any observable difference (error code or receipt) would fork state pre-activation. Operator tooling should attest at/after the fork epoch.ProcessEconomicsEndOfEpoch(0, …)) is safe: nobody has attested at genesis, so the guard keeps enforcement off.ResetValidatorStatisticsAtNewEpoch→SetEpochValidatorsInfo, so a version required at epoch E excludes non-attested validators from epoch E's election exactly at the fork boundary.updatePeerListStatuslifts them back to eligible as soon as they attest a satisfying version, or immediately if the mechanism is disabled (fork off / wildcard). A guard lapse alone deliberately does not restore unattested observers (hysteresis).Changes
data/transaction/proto/contracts.proto:ValidatorConfig.NodeVersion(9)core/kapp/validators/proto/validatorData.proto:AttestedVersion(27),AttestedEpoch(28)core/kapp/validators/versionAttestation.go(new): required-version lookup with freshness epoch, semver compare (pre-release aware), supermajority + floor guardscore/kapp/validators/validators.go: attestation handling inUpdateValidator(fork-gated, validated: utf8, ≤MaxSoftwareVersionLengthInBytes)core/kapp/validators/peersUpdate.go: version branch inupdatePeerListStatus, enforcement threaded through V1/V2 epoch processingconfig/enableEpochs.go,config/node/enableEpochs.yaml,core/fork/forks.go,core/interface.go+ fork controller stubs: newversionAttestationflagcore/kapp/kappController/kapp.go,core/kapp/factory/validators.go,factory/stateComponents.go,cmd/node/startup.go: wireversions.versionsByEpochsand genesisMinNumberOfNodesinto the validators kappcore/kapp/validators/versionAttestation_test.go(new): 40+ cases covering parsing, comparison (incl. pre-release), epoch resolution and freshness, demotion/restore, hysteresis, supermajority boundary (incl. exact 2/3), waiting-attester exclusion, floor guard, stale attestations, fork gating, validation, and fullProcessEconomicsEndOfEpochround-tripsKnown residual risks (accepted by design, documented for review)
numToStaybackfill.versions.versionsByEpochsbecomes a state-relevant config (same trust model as gas schedule configs, and already network-uniform whenever non-wildcard because of the headerSoftwareVersionequality check).requiredVersionForEpochmust keep agreeing withheaderIntegrityVerifier.getMatchingVersion; both rely on the startup validation inprepareVersions.Complementary follow-ups (not in this PR)
versionsByEpochsfor mandatory releases (activates both this mechanism and the existing dormant header-version enforcement).🤖 Generated with Claude Code
Summary
Adds fork-gated validator node-version attestation and an end-of-epoch version enforcement mechanism in the validators KApp, affecting consensus-critical validator peer-list selection and persisting new attestation state.
ValidatorConfigwithnodeVersion(ValidatorConfig.NodeVersion). WhenVersionAttestationfork is inactive,UpdateValidatorintentionally ignoresNodeVersion(not rejected, not recorded) to avoid pre-fork receipt/state divergence. When active,UpdateValidatorvalidates UTF-8 and length (<=core.MaxSoftwareVersionLengthInBytes) and recordsValidatorData.attestedVersionandValidatorData.attestedEpoch. Adds receipt tracking constantErrFieldInvalidNodeVersionfor invalid node version inputs.ValidatorDatawith persistedAttestedVersionandAttestedEpochfields.versionAttestation.goplus peer-list routing viacore/kapp/validators/peersUpdate.go:VersionAttestationfork flag is enabled for the epoch andversionsByEpochsspecifies a non-wildcard required version for that epoch; wildcard/empty/missing => no requirement.observer(not jailed/penalized); they return to eligible once they submit a suitable attested version.MinElectableNodes) so electable coverage cannot drop too far.enableEpochs.versionAttestationand a fork feature flag (ForkController.VersionAttestation()), and wiresVersionsByEpochsplusMinElectableNodesthrough:NewValidatorKApp,NewKappController, state components factory),cmd/node/startup.gosetsMinElectableNodesfrom genesis),EpochConfirmed.getValidator) are logged and treated as unsatisfied to ensure the mechanism errs on the side of not demoting incorrectly.minAttestedEpochfreshness logic, enforcement guard activation (supermajority + floor), demotion/restoration behavior throughupdatePeerListStatus,UpdateValidatorattestation persistence rules (including fork-active vs inactive), and end-of-epoch economics demotion/restore scenarios (including negative/guard-lapse cases).