orbis module extras#120
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (6)
📜 Recent review details
|
| Layer / File(s) | Summary |
|---|---|
Proto schema changes proto/sourcehub/orbis/*.proto |
Defines DemeritConfig, adds it to Ring and Params, makes pss_interval required, adds EventReportAccepted, CancelPendingRing, SubmitReport, ReportEnvelope, NodeDemerits, and genesis node demerit state. |
Types, keys, and validation helpers x/orbis/types/*.go |
Adds minimum PSS validation, demerit config defaults and validation, ring ID hashing changes, new store key prefixes, updated sentinel errors, new message constructors and ValidateBasic logic, and interface registration updates. |
Keeper storage, report validation, and msg/query handlers x/orbis/keeper/*.go, x/orbis/module/genesis.go, x/orbis/module/autocli.go, app/app_config.go |
Adds accepted-report and node-demerit storage, canonical report validation and demerit calculation, SubmitReport, CancelPendingRing, FinalizeRing outcome handling, NodeDemerits, ring validation updates, genesis import/export wiring, end-block expiry cleanup, and CLI descriptor changes. |
Generated protobuf bindings api/sourcehub/orbis/*.pulsar.go |
Regenerates fast-protobuf reflection, marshal/unmarshal logic, and registration metadata for EventReportAccepted, Params.DefaultDemeritConfig, QueryNodeDemeritsRequest/QueryNodeDemeritsResponse, and NodeDemeritEntry genesis bindings. |
Tests x/orbis/keeper/*_test.go, x/orbis/types/*_test.go, x/orbis/module/genesis_test.go, testutil/network/network.go |
Adds canonical report and demerit tests, CancelPendingRing and FinalizeRing outcome coverage, ring creation and mutation updates for PssInterval and DemeritConfig, genesis round-trip coverage, and validation tests for PSS intervals and demerit config. |
Sequence Diagram(s)
sequenceDiagram
participant Client
participant MsgServer
participant validateSubmittedReport
participant KeeperStore
participant ThresholdSig
Client->>MsgServer: MsgSubmitReport
MsgServer->>validateSubmittedReport: envelope + block context
validateSubmittedReport->>KeeperStore: HasAcceptedReport (replay check)
validateSubmittedReport->>KeeperStore: GetRing (ring existence + PK + digest)
validateSubmittedReport->>ThresholdSig: verifyThresholdSignature (BLS/FROST)
ThresholdSig-->>validateSubmittedReport: ok / error
validateSubmittedReport-->>MsgServer: validated report bundle
MsgServer->>KeeperStore: SetAcceptedReport
MsgServer->>KeeperStore: IncrementNodeDemerits
MsgServer-->>Client: MsgSubmitReportResponse
Estimated code review effort
🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
- sourcenetwork/sourcehub#113: Adds adjacent Orbis module plumbing for the same reporting and demerit area, including keeper/query and event support.
- sourcenetwork/sourcehub#118: Touches the same ring PSS interval and lifecycle path in
x/orbis/keeper/msg_server.go. - sourcenetwork/sourcehub#121: Introduces the same
EventReportAcceptedprotobuf surface and generated binding updates.
Suggested reviewers
- jsimnz
- Lodek
- iverc
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 help to get the list of available commands.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #120 +/- ##
==========================================
+ Coverage 49.20% 51.30% +2.09%
==========================================
Files 302 317 +15
Lines 17525 18491 +966
==========================================
+ Hits 8623 9486 +863
+ Misses 8010 8004 -6
- Partials 892 1001 +109 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // These integration tests only use validator 0. Keeping the default network | ||
| // single-validator also avoids a CometBFT v0.38.21 shutdown race where a | ||
| // peer-reactor goroutine can access the block store after it has been closed. | ||
| cfg.NumValidators = 1 |
There was a problem hiding this comment.
double check this should not effect anything prod, just on the testing this should fix the flakey faucet tests
* Add reporting * clean * add settable demerits * add querys * window demerit reset * proto * more tests * node demerits rpc change * coderabbit
![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
🤖 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 `@x/orbis/keeper/msg_server.go`:
- Around line 136-145: The ring-deletion flow in MsgFinalizeRing/MsgCreateRing
only removes the Orbis KV record via DeleteRing, but it leaves the ACP
registration behind, so reusing the same deterministic ring ID can fail. Update
the ring cleanup path to also unregister or delete the corresponding
types.ACPResourceRing from ACP whenever a ring is deleted, and apply the same
cleanup in both conflict-delete branches so repeated create attempts can
succeed.
In `@x/orbis/keeper/report.go`:
- Around line 117-124: The protocol-version validation in report processing is
using the submit block time instead of the report’s observed time, which can
incorrectly reject still-valid reports. Update the effective version check in
the report handling path to base `effectiveReportProtocolVersion` on
`payload.ObservedAt` (or the report’s observed timestamp) rather than `now`, and
keep the existing `payload.originProtocolVersion` comparison and error path in
`report.go` unchanged otherwise.
In `@x/orbis/keeper/validation.go`:
- Around line 35-37: validateRing currently checks demerit config but never
enforces the new PSS interval invariant, so add the new helper call there as
well. Update validateRing in validation.go to invoke the PSS interval validation
helper alongside types.ValidateDemeritConfig, ensuring any persisted types.Ring
is rejected when pss_interval is below MinPSSIntervalSeconds. Keep the fix
localized to validateRing and the new helper introduced near the PSS validation
logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a2b6def-e464-4e70-8179-9d8898e3a155
⛔ Files ignored due to path filters (10)
api/sourcehub/orbis/query_grpc.pb.gois excluded by!**/*.pb.goapi/sourcehub/orbis/ring.pb.gois excluded by!**/*.pb.goapi/sourcehub/orbis/tx.pb.gois excluded by!**/*.pb.goapi/sourcehub/orbis/tx_grpc.pb.gois excluded by!**/*.pb.gox/orbis/types/events.pb.gois excluded by!**/*.pb.gox/orbis/types/params.pb.gois excluded by!**/*.pb.gox/orbis/types/query.pb.gois excluded by!**/*.pb.gox/orbis/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/orbis/types/ring.pb.gois excluded by!**/*.pb.gox/orbis/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (39)
api/sourcehub/orbis/events.pulsar.goapi/sourcehub/orbis/params.pulsar.goapi/sourcehub/orbis/query.pulsar.goproto/sourcehub/orbis/events.protoproto/sourcehub/orbis/params.protoproto/sourcehub/orbis/query.protoproto/sourcehub/orbis/ring.protoproto/sourcehub/orbis/tx.prototestutil/network/network.gox/orbis/keeper/demerits.gox/orbis/keeper/grpc_query.gox/orbis/keeper/msg_server.gox/orbis/keeper/msg_server_baseapp_test.gox/orbis/keeper/msg_server_cancel_pending_ring_test.gox/orbis/keeper/msg_server_reshare_sig_test.gox/orbis/keeper/msg_server_test.gox/orbis/keeper/optional.gox/orbis/keeper/params.gox/orbis/keeper/report.gox/orbis/keeper/report_test.gox/orbis/keeper/reshare_sign_bytes_test.gox/orbis/keeper/ring_upgrade_test.gox/orbis/keeper/store.gox/orbis/keeper/store_test.gox/orbis/keeper/validation.gox/orbis/keeper/validation_test.gox/orbis/module/autocli.gox/orbis/types/codec.gox/orbis/types/errors.gox/orbis/types/genesis_upgrade_test.gox/orbis/types/keys.gox/orbis/types/keys_test.gox/orbis/types/message_cancel_pending_ring.gox/orbis/types/message_create_ring.gox/orbis/types/message_mutations_test.gox/orbis/types/message_set_ring_pss_interval_by_acp.gox/orbis/types/message_submit_report.gox/orbis/types/params.gox/orbis/types/pss.go
💤 Files with no reviewable changes (1)
- x/orbis/keeper/optional.go
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test
🧰 Additional context used
🪛 ast-grep (0.44.0)
api/sourcehub/orbis/query.pulsar.go
[warning] 7629-7629: 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: int32(wire >> 3)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 8078-8078: 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: int32(wire >> 3)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
x/orbis/keeper/report.go
[warning] 423-423: 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(len(value))
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 436-436: 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(len(values))
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
x/orbis/keeper/store.go
[warning] 115-115: 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(len(ringID))
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
api/sourcehub/orbis/events.pulsar.go
[warning] 5470-5470: 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: int32(wire >> 3)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🪛 Buf (1.71.0)
proto/sourcehub/orbis/params.proto
[error] 6-6: imported file does not exist
(COMPILE)
[error] 12-12: cannot find amino.name in this scope
(COMPILE)
[error] 13-13: cannot find gogoproto.equal in this scope
(COMPILE)
[error] 15-15: cannot find DemeritConfig in this scope
(COMPILE)
[error] 15-15: cannot find gogoproto.nullable in this scope
(COMPILE)
proto/sourcehub/orbis/query.proto
[error] 61-61: cannot find google.api.http in this scope
(COMPILE)
proto/sourcehub/orbis/ring.proto
[error] 18-18: cannot find gogoproto.equal in this scope
(COMPILE)
[error] 39-39: cannot find gogoproto.nullable in this scope
(COMPILE)
[error] 40-40: cannot find gogoproto.nullable in this scope
(COMPILE)
proto/sourcehub/orbis/tx.proto
[error] 10-10: imported file does not exist
(COMPILE)
[error] 65-65: cannot find DemeritConfig in this scope
(COMPILE)
[error] 91-91: cannot find cosmos.msg.v1.signer in this scope
(COMPILE)
[error] 149-149: cannot find cosmos.msg.v1.signer in this scope
(COMPILE)
[error] 151-151: cannot find gogoproto.nullable in this scope
(COMPILE)
🔇 Additional comments (38)
x/orbis/keeper/store.go (1)
43-53: LGTM!Also applies to: 55-121
x/orbis/keeper/store_test.go (1)
42-101: LGTM!x/orbis/keeper/grpc_query.go (1)
8-8: LGTM!Also applies to: 131-153
x/orbis/keeper/report.go (1)
1-116: LGTM!Also applies to: 127-550
x/orbis/keeper/demerits.go (1)
1-21: LGTM!x/orbis/keeper/msg_server.go (1)
45-70: LGTM!Also applies to: 156-158, 182-214, 220-223, 326-335, 516-561
x/orbis/keeper/params.go (1)
12-20: LGTM!Also applies to: 24-37
x/orbis/module/autocli.go (1)
51-74: LGTM!Also applies to: 107-112
x/orbis/keeper/report_test.go (1)
27-693: LGTM!x/orbis/keeper/msg_server_baseapp_test.go (1)
87-127: 🎯 Functional CorrectnessRe-read state from a fresh BaseApp context after
SimDeliver.
ctxis created before delivery and then reused for the post-txGetRingassertion. With BaseApp, that can end up asserting against a pre-delivery snapshot rather than the tx result. Please verify the SDK semantics here and consider creating a fresh context for the post-SimDeliverreads.Also applies to: 200-231
x/orbis/keeper/msg_server_cancel_pending_ring_test.go (1)
15-189: LGTM!x/orbis/keeper/validation_test.go (1)
11-28: 🎯 Functional CorrectnessVerify this doesn't leave a
validateRingbypass for the new minimum PSS interval.This test locks in that
validateRingacceptspss_interval = 0and values belowtypes.MinPSSIntervalSeconds. If any create/import/mutation path still callsvalidateRingwithoutvalidateRingPSSInterval, the new minimum remains bypassable.testutil/network/network.go (1)
102-105: 📐 Maintainability & Code QualityPlease confirm no callers still rely on the default multi-validator topology.
DefaultConfigWithOptionsnow forces every consumer onto a single-validator network. That removes consensus/peer-path coverage unless callers explicitly overridecfg.NumValidatorsafterward, so it's worth verifying this helper is only used by tests that intentionally target validator 0.x/orbis/types/genesis_upgrade_test.go (1)
10-37: LGTM!x/orbis/types/message_mutations_test.go (1)
12-48: LGTM!Also applies to: 50-79, 91-91, 103-106, 120-132
x/orbis/keeper/msg_server_test.go (1)
66-84: LGTM!Also applies to: 111-111, 176-278, 279-323, 340-352, 362-368, 375-386, 395-406, 415-426, 437-470, 494-500, 522-528, 551-585, 587-623, 625-655, 657-673, 709-721, 744-763, 766-823, 825-842, 881-897, 915-935, 960-975, 1010-1022, 1050-1067, 1090-1109, 1130-1152, 1180-1232, 1240-1286, 1289-1310, 1355-1368
x/orbis/keeper/msg_server_reshare_sig_test.go (1)
37-43: LGTM!Also applies to: 123-129
x/orbis/keeper/reshare_sign_bytes_test.go (1)
25-28: LGTM!Also applies to: 40-43, 60-63, 75-78
x/orbis/keeper/ring_upgrade_test.go (1)
43-43: LGTM!Also applies to: 119-119, 129-129, 207-207, 234-255, 368-386
x/orbis/types/keys_test.go (1)
14-15: LGTM!x/orbis/types/params.go (1)
21-37: LGTM!Also applies to: 47-57
proto/sourcehub/orbis/tx.proto (1)
10-10: 🎯 Functional CorrectnessNo unresolved proto symbols here.
sourcehub/orbis/ring.protoexists, andDemeritConfig,cosmos.msg.v1.signer, andgogoproto.nullableare all already in scope.> Likely an incorrect or invalid review comment.proto/sourcehub/orbis/params.proto (1)
6-15: 🎯 Functional CorrectnessDrop this concern.
sourcehub/orbis/ring.protoexists, definesDemeritConfig, andparams.protoalready imports theaminoandgogoprotooptions needed here.> Likely an incorrect or invalid review comment.proto/sourcehub/orbis/ring.proto (1)
16-21: 🎯 Functional CorrectnessNo issue here —
gogoprotois already imported.proto/sourcehub/orbis/ring.protoincludesimport "gogoproto/gogo.proto";, so(gogoproto.equal)and(gogoproto.nullable)resolve.> Likely an incorrect or invalid review comment.api/sourcehub/orbis/params.pulsar.go (1)
148-152: 🩺 Stability & AvailabilityRemove the nil-guard suggestion.
default_demerit_configis a protobuf message field, and(*DemeritConfig).ProtoReflect()already handles a nil receiver, so thisGet()path does not panic on an unset field.> Likely an incorrect or invalid review comment.proto/sourcehub/orbis/events.proto (1)
71-78: LGTM!proto/sourcehub/orbis/query.proto (2)
144-151: LGTM!
59-62: 🎯 Functional CorrectnessNo action needed here.
proto/sourcehub/orbis/query.protoalready importsgoogle/api/annotations.proto, andbuf.yamlincludesbuf.build/googleapis/googleapis, sogoogle.api.httpis in scope.> Likely an incorrect or invalid review comment.x/orbis/types/pss.go (1)
5-12: LGTM!x/orbis/types/keys.go (1)
19-24: LGTM!Also applies to: 37-46
x/orbis/types/message_set_ring_pss_interval_by_acp.go (1)
18-19: LGTM!x/orbis/types/errors.go (1)
31-43: 🗄️ Data Integrity & IntegrationNo error-code renumbering here. These additions use fresh codes (
1238-1240), and the surrounding registrations remain stable.> Likely an incorrect or invalid review comment.x/orbis/types/message_cancel_pending_ring.go (1)
9-19: LGTM!x/orbis/types/message_create_ring.go (1)
11-19: LGTM!Also applies to: 35-42
x/orbis/types/message_submit_report.go (1)
9-50: LGTM!x/orbis/types/codec.go (1)
14-20: LGTM!api/sourcehub/orbis/events.pulsar.go (1)
4999-5674: LGTM!Also applies to: 6160-6227, 6299-6322, 6337-6337, 6349-6349, 6485-6504
api/sourcehub/orbis/query.pulsar.go (1)
7254-7737: LGTM!Also applies to: 7738-8141, 8732-8809, 8920-9024, 9039-9101, 9310-9341
| if claimedReportID != reportID { | ||
| return nil, errorsmod.Wrap(types.ErrInvalidReport, "report_id does not match canonical report bytes") | ||
| } | ||
| if k.HasAcceptedReport(goCtx, reportID) { |
There was a problem hiding this comment.
nit: I think it could be more efficient to check/dedupe it per ring/node/type/time so one outage only counts once
There was a problem hiding this comment.
good find, I like this idea, I chose to solve it on the orbis side mainly as orbis should drive a lot of the security here. This is done by having a session id, a session id is created on every request so only one report per session stop duplicate sessions. The idea is there is one reason a session could fail, that creates a report. This also allows an extra check on the orbis side like hey this session failed a report request should be coming in, which I like
| return err | ||
| } | ||
|
|
||
| if signingCommittee.threshold < 2 { |
There was a problem hiding this comment.
nit: please add more tests around this logic
| store.Delete([]byte(ringID)) | ||
| } | ||
|
|
||
| func (k *Keeper) SetAcceptedReport(ctx context.Context, reportID string) { |
There was a problem hiding this comment.
I think we should expire these in EndBlock after 120s
| return nil, types.ErrInvalidRingCanceller | ||
| } | ||
|
|
||
| k.DeleteRing(goCtx, ring.Id) |
There was a problem hiding this comment.
rings are hard deleted but acp objects are not, should we archive instead?
There was a problem hiding this comment.
So my concern is that an acp policy can be used in multiple rings, so if a ring was deleted and the policy archived it would brick other rings?
iverc
left a comment
iverc
left a comment
There was a problem hiding this comment.
Left a few comments, mostly nits, except for the genesis.go import/export one. LGTM otherwise 👍
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@x/orbis/types/genesis.go`:
- Around line 22-44: Add validation in the genesis checking logic that verifies
every NodeDemerits entry references an existing ring. In the validation loop in
genesis.go, build a set of Ring IDs from gs.Rings first, then reject any entry
whose RingId is not present before proceeding with the existing empty-field,
points, and duplicate checks. Keep the fix localized to the genesis validation
path so InitGenesis only ever persists demerits for known rings.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bbbba87-e555-4803-8960-20ec0bad352b
⛔ Files ignored due to path filters (1)
x/orbis/types/genesis.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
api/sourcehub/orbis/genesis.pulsar.goproto/sourcehub/orbis/genesis.protox/orbis/keeper/store.gox/orbis/module/genesis.gox/orbis/module/genesis_test.gox/orbis/types/genesis.gox/orbis/types/genesis_upgrade_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test
🧰 Additional context used
🪛 ast-grep (0.44.0)
api/sourcehub/orbis/genesis.pulsar.go
[warning] 1575-1575: 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: int32(wire >> 3)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🪛 Buf (1.71.0)
proto/sourcehub/orbis/genesis.proto
[error] 37-37: cannot find gogoproto.nullable in this scope
(COMPILE)
[error] 38-38: cannot find amino.dont_omitempty in this scope
(COMPILE)
🔇 Additional comments (2)
api/sourcehub/orbis/genesis.pulsar.go (1)
221-2029: LGTM!proto/sourcehub/orbis/genesis.proto (1)
36-38: 🎯 Functional CorrectnessCheck the
node_demeritsfield optionsEnsure
gogoproto.nullableandamino.dont_omitemptyare imported and resolve forproto/sourcehub/orbis/genesis.proto, or drop them fromnode_demeritsif they are not meant to be used.
3 participants
This PR adds a reporting system to orbis so nodes can report on each other with a threshold signature and node demerit points are assigned and tracked based on that.
It also adds a way for a node to delete a pending ring