Conversation
Re-reviewed after 4d4f1ce. The
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
![roomote-v0[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
devnet/go.mod
Outdated
| // Local development - uncomment these for local testing | ||
| // Comment lines with github.com/LumeraProtocol/ before releasing | ||
| // github.com/LumeraProtocol/lumera => .. | ||
| github.com/LumeraProtocol/lumera => .. |
There was a problem hiding this comment.
This local replace directive (github.com/LumeraProtocol/lumera => ..) is uncommented, which means the devnet module resolves lumera from the parent directory instead of a published version. If merged to master, anyone cloning the repo and running go build or go test in the devnet/ directory will get the local source, which is the intended behavior for local dev but breaks reproducible builds from a clean checkout if the go.sum no longer contains the published module hash (the corresponding go.sum entry for LumeraProtocol/lumera v1.10.0 was also removed in this PR). The existing comment says "uncomment these for local testing" / "Comment lines with github.com/LumeraProtocol/ before releasing" -- this should be re-commented before merge unless the intent is to permanently switch to a monorepo-style build.
Fix it with Roo Code or mention @roomote and request a fix.
There was a problem hiding this comment.
Pull request overview
Implements LEP-5 “Cascade Availability Commitment” by extending Cascade metadata with a chunk-Merkle commitment + challenge indices, and enforcing SuperNode-provided Merkle proofs during Cascade finalization.
Changes:
- Add LEP-5 protobuf types/fields (HashAlgo, AvailabilityCommitment, ChunkProofs) and module params (svc_challenge_count, svc_min_chunks_for_challenge).
- Introduce Merkle + challenge helper packages and keeper-side proof verification wired into Cascade finalization/metadata updates.
- Add unit/E2E tests and documentation for the LEP-5 flow.
Reviewed changes
Copilot reviewed 22 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| x/action/v1/types/params.pb.go | Regenerated Params bindings to include SVC params. |
| proto/lumera/action/v1/params.proto | Adds LEP-5 governance parameters for challenge count/min chunks. |
| x/action/v1/types/metadata.pb.go | Regenerated metadata bindings: HashAlgo enum, AvailabilityCommitment, ChunkProof, CascadeMetadata extensions. |
| proto/lumera/action/v1/metadata.proto | Defines LEP-5 enum/messages and extends CascadeMetadata. |
| x/action/v1/types/metadata_proto_test.go | Round-trip/back-compat protobuf tests for new metadata fields. |
| x/action/v1/types/events.go | Adds SVC-related event types + attributes. |
| x/action/v1/types/errors.go | Adds SVC-specific error codes for proof verification failures. |
| x/action/v1/merkle/merkle.go | Implements BLAKE3 chunk Merkle tree + proof generation/verification. |
| x/action/v1/merkle/merkle_test.go | Validates Merkle root/proofs (vectors, tamper, scale, edge cases). |
| x/action/v1/challenge/challenge.go | Implements deterministic challenge-index derivation helper. |
| x/action/v1/challenge/challenge_test.go | Test vectors and determinism/uniqueness checks for challenge derivation. |
| x/action/v1/keeper/svc.go | Adds keeper verification for chunk proofs + evidence/pass events. |
| x/action/v1/keeper/svc_test.go | Unit tests for keeper proof verification and evidence events. |
| x/action/v1/keeper/finalize_svc_test.go | Integration-style test: finalize Cascade with valid proofs reaches DONE. |
| x/action/v1/keeper/cascade_commitment_test.go | Registration-time commitment + challenge indices validation tests. |
| x/action/v1/keeper/action_cascade.go | Enforces commitment validation on request/register; verifies proofs on finalize; persists commitment/proofs. |
| docs/leps/new-feature-lep5.md | Full LEP-5 technical specification document. |
| docs/leps/lep5.md | LEP-5 testing guide for devnet flows. |
| devnet/tests/validator/lep5_test.go | Devnet E2E tests for successful and failing proof finalization (+ variable chunk sizes). |
| devnet/go.mod | Updates devnet module deps and enables local replace for lumera. |
| devnet/go.sum | Updates devnet sums to match module dependency changes. |
| devnet/config/config.json | Disables Hermes in devnet configuration. |
| buf.lock | Updates Buf dependency lock (cosmos-sdk entry changed). |
| app/proto_bridge.go | Registers new HashAlgo enum for proto bridge usage. |
| .gitignore | Adds ignores for various local planning/docs artifacts. |
Comments suppressed due to low confidence (4)
docs/leps/lep5.md:3
- This link points to
../new-feature-lep5.md(i.e.,docs/new-feature-lep5.md), but the technical specification file added in this PR isdocs/leps/new-feature-lep5.md. Update the relative link so it resolves correctly from withindocs/leps/.
> **Full design:** [LEP-5 Technical Specification](../new-feature-lep5.md)
devnet/tests/validator/lep5_test.go:70
- Typo in the subtest name:
TineFile_4B_ChunkSize_1Breads like it should beTinyFile_4B_ChunkSize_1B(or similar). Renaming makes logs and-runfilters clearer.
name: "TineFile_4B_ChunkSize_1B",
devnet/config/config.json:37
- This change disables Hermes in the devnet config (
enabled: false). Since this can affect IBC-related devnet flows and tests, please confirm it's intentional for LEP-5 and (if so) consider documenting the reason in the devnet docs or making it configurable via env/CLI so other devnet users aren't surprised.
"hermes": {
"enabled": false
}
x/action/v1/keeper/action_cascade.go:45
- In
validateAvailabilityCommitment, the error message for a hash algo mismatch uses%qwith an enum value (cascadeCommitmentHashAlgo).%qwill format the underlying integer as a quoted rune (e.g.,'\x01'), which is confusing and not stable. Use%v(or%swith.String()) and ideally include both expected and actual values in the message.
if commitment.HashAlgo != cascadeCommitmentHashAlgo {
return fmt.Errorf("availability_commitment.hash_algo must be %q", cascadeCommitmentHashAlgo)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 30 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
devnet/tests/validator/lep5_test.go:70
- The function name has a typo: "TineFile" should be "TinyFile". This affects test readability and naming consistency.
name: "TineFile_4B_ChunkSize_1B",
tests/systemtests/lep5_action_test.go:101
- The comment has a typo: "systemex" should be "systemtest" or "system". This documentation typo affects clarity when developers read the test code.
// For the systemex test, we verify the CLI can construct and submit
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This commit deletes the challenge package, including the DeriveIndices function and its test cases. The removal is part of a refactoring effort to streamline the codebase and eliminate unused components.
2) Refactor SVC parameters
…put, overriding `go.mod` detection. - Updated `release.yml` to use Go version `1.25.6`.
| params := k.GetParams(ctx) | ||
| challengeCount, minChunks := GetSVCParams(params) | ||
|
|
||
| if commitment.NumChunks < minChunks { | ||
| // Small files are out of challenge scope. | ||
| return nil | ||
| } | ||
|
|
||
| expectedCount := challengeCount | ||
| if expectedCount > commitment.NumChunks { | ||
| expectedCount = commitment.NumChunks | ||
| } |
There was a problem hiding this comment.
VerifyChunkProofs derives expectedCount from the current governance params (lines 60-71), then checks that both the submitted proof count and the stored commitment.ChallengeIndices length match it (lines 73 and 88). If governance changes svc_challenge_count between registration and finalization (e.g. from 8 to 16), in-flight actions become unfinalizable: the commitment was registered with 8 indices, but finalization now expects 16 proofs. The stored commitment's ChallengeIndices is the authoritative source -- it was already validated at registration time. Using len(commitment.ChallengeIndices) as expectedCount here instead of re-deriving from current params would avoid this.
| params := k.GetParams(ctx) | |
| challengeCount, minChunks := GetSVCParams(params) | |
| if commitment.NumChunks < minChunks { | |
| // Small files are out of challenge scope. | |
| return nil | |
| } | |
| expectedCount := challengeCount | |
| if expectedCount > commitment.NumChunks { | |
| expectedCount = commitment.NumChunks | |
| } | |
| params := k.GetParams(ctx) | |
| _, minChunks := GetSVCParams(params) | |
| if commitment.NumChunks < minChunks { | |
| // Small files are out of challenge scope. | |
| return nil | |
| } | |
| // Use the stored commitment's challenge indices as the authoritative count. | |
| // These were validated at registration time against the params that were | |
| // active then; re-deriving from current params would break in-flight | |
| // actions if governance changes svc_challenge_count. | |
| expectedCount := uint32(len(commitment.ChallengeIndices)) |
Fix it with Roo Code or mention @roomote and request a fix.
| params := k.GetParams(ctx) | ||
| challengeCount := keeper.SVCChallengeCount | ||
| minChunks := keeper.SVCMinChunksForChallenge |
There was a problem hiding this comment.
params is fetched from the keeper on line 29 but the SVC values on lines 30-31 use the hardcoded fallback constants directly instead of keeper.GetSVCParams(params). If governance updates svc_challenge_count or svc_min_chunks_for_challenge, the simulation would generate commitments with the wrong number of challenge indices, causing validateAvailabilityCommitment to reject the registration and the simulation to silently no-op.
| params := k.GetParams(ctx) | |
| challengeCount := keeper.SVCChallengeCount | |
| minChunks := keeper.SVCMinChunksForChallenge | |
| params := k.GetParams(ctx) | |
| challengeCount, minChunks := keeper.GetSVCParams(params) |
Fix it with Roo Code or mention @roomote and request a fix.
- Add svc_challenge_count (field 12) and svc_min_chunks_for_challenge (field 13) to action module Params proto with defaults 8 and 4 - Keeper reads SVC params from on-chain Params with zero-value fallback to package defaults (backward compatible with pre-migration state) - Remove redundant availability commitment validation from Process(); keep single validation gate in RegisterAction() which reads params from the keeper Proto regeneration pending (params.pb.go hand-edited to match proto schema; run buf generate when toolchain is available).
|
High-signal correctness issue in
That makes already-registered actions non-deterministically fail if governance updates either SVC param between registration and finalization. The registration commitment is immutable ( Concrete failure mode:
Suggested fix direction:
|
https://www.notion.so/pastelnetwork/LEP5-Cascade-Availability-Commitment-302df11fee14808e9cb8d745aa71465f?source=copy_link