New DKG flow#115
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## jesse/Orbis-module #115 +/- ##
======================================================
+ Coverage 48.16% 48.68% +0.52%
======================================================
Files 301 302 +1
Lines 17423 17471 +48
======================================================
+ Hits 8392 8506 +114
+ Misses 8174 8089 -85
- Partials 857 876 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
x/orbis/keeper/reshare_sign_bytes_test.go (1)
40-72: ⚡ Quick winConsider a negative case to guard the canonicalization.
The test only asserts equality for canonically-identical states. An implementation that over-collapses (or returns a constant) would still pass. Adding an assertion that a genuine sign-state change (e.g., a different
ThresholdorXNewThreshold) yields different bytes would meaningfully harden this test.🤖 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 `@x/orbis/keeper/reshare_sign_bytes_test.go` around lines 40 - 72, Add a negative-case assertion to ensure the canonicalization isn't collapsing distinct states: create a variant of the existing test state (use ringForReshareFinalization and ringReshareFinalizeSignBytes) but modify a meaningful field such as Threshold or XNewThreshold (e.g., change Threshold from 2 to 3 or NewThreshold from 1 to 2), compute its finalized sign bytes via ringReshareFinalizeSignBytes and assert that these bytes are not equal to the original signBytes (use require.NotEqual) to verify different sign-states produce different canonical bytes.
🤖 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 `@api/sourcehub/orbis/query.pulsar.go`:
- Line 7405: The protobuf field number 1 was reused for Pagination (Pagination
*v1beta1.PageRequest `protobuf:"bytes,1...`) after removing namespace which
breaks wire compatibility; update the protobuf definition so Pagination (and the
related id/pagination fields) use the original field number 2 instead of 1, and
explicitly reserve the removed namespace tag 1 (e.g., add a reserved 1 entry) in
the .proto to prevent reuse; apply the same change for the other occurrences
referenced (the other Pagination/id fields at the indicated locations) so no
removed tag is reassigned.
In `@proto/sourcehub/orbis/tx.proto`:
- Around line 61-66: The PR incorrectly claims a protobuf compile blocker for
MsgFinalizeRing — remove that claim and instead add non-empty doc comments for
the messages required by buf lint: MsgFinalizeRing, MsgFinalizeRingResponse,
MsgFinalizeRingReshareByThresholdSignature, and
MsgFinalizeRingReshareByThresholdSignatureResponse; ensure each message has a
short descriptive comment above it, and verify that numeric field tags for
MsgCreateRing, RingReshareFinalizeSignDoc, MsgStoreDocument, and
MsgStoreKeyDerivation were not changed (restore original tags if they were) to
avoid a public API break.
In `@x/orbis/keeper/msg_server.go`:
- Around line 186-192: Update UpdateRingByAcp flow to enforce that every key in
msg.NewPeerNodeKeys has registered node info via k.GetNodeInfo(...) before
accepting/copying them (similar invariant enforced in CreateRing); specifically,
inside the UpdateRingByAcp handler call validateRingUpdate/validateRing as
before but add an existence check: iterate msg.NewPeerNodeKeys and call
k.GetNodeInfo(ctx, pkHex) for each, returning an error if any are missing, then
proceed to set ring.NewPeerNodeKeys = append(...). Also update FinalizeRing
authorization to consult k.GetNodeInfo(...) when validating signerPublicKeyHex
against ring.PeerNodeKeys (or otherwise fail) so unregistered keys cannot
confirm; reference functions/paths: UpdateRingByAcp, validateRingUpdate,
validateRing, FinalizeRing, CreateRing, and k.GetNodeInfo.
- Around line 119-130: The handler currently deletes the ring then returns
types.ErrRingPkConflict which causes KV writes to be rolled back; change the
conflict branch so after emitting EventRingDeleted and calling k.DeleteRing you
do NOT return types.ErrRingPkConflict but instead return a successful response
(i.e., a non-error result for the Msg confirm handler) so the deletion persists.
Update the branch that iterates ring.Confirmations (checking c.RingPk vs
msg.RingPk), the ctx.EventManager().EmitTypedEvent(types.EventRingDeleted) call,
and k.DeleteRing invocation to return a successful MsgConfirm response (and nil
error) rather than ErrRingPkConflict.
In `@x/orbis/keeper/validation.go`:
- Around line 66-76: The current guard only compares new_peer_node_keys vs
newThreshold when both are provided, allowing a threshold-only reshare to set
newThreshold larger than the existing committee; add a check in the validation
path (same function where newThreshold and newPeerNodeKeys are inspected) that
if newPeerNodeKeys is empty and newThreshold != nil you validate *newThreshold
<= uint32(len(existing.PeerNodeKeys)) and return
errorsmod.Wrapf(types.ErrInvalidRing, "new_threshold (%d) cannot exceed existing
committee size (%d)", *newThreshold, len(existing.PeerNodeKeys)) when it
violates this; this prevents ringForReshareFinalization/validateRing from later
rejecting the change and leaving ErrReshareInProgress.
---
Nitpick comments:
In `@x/orbis/keeper/reshare_sign_bytes_test.go`:
- Around line 40-72: Add a negative-case assertion to ensure the
canonicalization isn't collapsing distinct states: create a variant of the
existing test state (use ringForReshareFinalization and
ringReshareFinalizeSignBytes) but modify a meaningful field such as Threshold or
XNewThreshold (e.g., change Threshold from 2 to 3 or NewThreshold from 1 to 2),
compute its finalized sign bytes via ringReshareFinalizeSignBytes and assert
that these bytes are not equal to the original signBytes (use require.NotEqual)
to verify different sign-states produce different canonical bytes.
🪄 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: b96f86cd-f5da-4e62-b6e8-e817de93e980
⛔ Files ignored due to path filters (13)
api/sourcehub/orbis/document.pb.gois excluded by!**/*.pb.goapi/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/document.pb.gois excluded by!**/*.pb.gox/orbis/types/events.pb.gois excluded by!**/*.pb.gox/orbis/types/key_derivation.pb.gois excluded by!**/*.pb.gox/orbis/types/node_info.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 (30)
api/sourcehub/orbis/events.pulsar.goapi/sourcehub/orbis/key_derivation.pulsar.goapi/sourcehub/orbis/node_info.pulsar.goapi/sourcehub/orbis/query.pulsar.goproto/sourcehub/orbis/document.protoproto/sourcehub/orbis/events.protoproto/sourcehub/orbis/key_derivation.protoproto/sourcehub/orbis/node_info.protoproto/sourcehub/orbis/query.protoproto/sourcehub/orbis/ring.protoproto/sourcehub/orbis/tx.protox/orbis/keeper/acp.gox/orbis/keeper/grpc_query.gox/orbis/keeper/keeper.gox/orbis/keeper/msg_server.gox/orbis/keeper/msg_server_test.gox/orbis/keeper/optional.gox/orbis/keeper/reshare_sign_bytes.gox/orbis/keeper/reshare_sign_bytes_test.gox/orbis/keeper/store.gox/orbis/keeper/validation.gox/orbis/module/autocli.gox/orbis/types/acp.gox/orbis/types/errors.gox/orbis/types/keys.gox/orbis/types/keys_test.gox/orbis/types/message_create_ring.gox/orbis/types/message_finalize_ring.gox/orbis/types/message_store_document.gox/orbis/types/message_store_key_derivation.go
💤 Files with no reviewable changes (2)
- x/orbis/types/keys_test.go
- x/orbis/keeper/keeper.go
📜 Review details
🧰 Additional context used
🪛 Buf (1.69.0)
proto/sourcehub/orbis/query.proto
[error] 31-31: cannot find google.api.http in this scope
(COMPILE)
[error] 36-36: cannot find google.api.http in this scope
(COMPILE)
[error] 41-41: cannot find google.api.http in this scope
(COMPILE)
[error] 46-46: cannot find google.api.http in this scope
(COMPILE)
[error] 51-51: cannot find google.api.http in this scope
(COMPILE)
[error] 80-80: cannot find cosmos.base.query.v1beta1.PageRequest in this scope
(COMPILE)
[error] 96-96: cannot find Document in this scope
(COMPILE)
[error] 100-100: cannot find cosmos.base.query.v1beta1.PageRequest in this scope
(COMPILE)
[error] 116-116: cannot find KeyDerivation in this scope
(COMPILE)
[error] 120-120: cannot find cosmos.base.query.v1beta1.PageRequest in this scope
(COMPILE)
proto/sourcehub/orbis/tx.proto
[error] 62-62: cannot find cosmos.msg.v1.signer in this scope
(COMPILE)
[error] 71-71: cannot find cosmos.msg.v1.signer in this scope
(COMPILE)
[error] 115-115: cannot find cosmos.msg.v1.signer in this scope
(COMPILE)
[error] 165-165: cannot find cosmos.msg.v1.signer in this scope
(COMPILE)
🔇 Additional comments (26)
x/orbis/keeper/msg_server_test.go (1)
54-177: LGTM!Also applies to: 179-538, 540-616, 618-808, 810-837, 839-1017, 1019-1093
proto/sourcehub/orbis/events.proto (1)
9-10: LGTM!Also applies to: 15-16, 21-22, 27-28, 43-47
proto/sourcehub/orbis/key_derivation.proto (1)
9-14: Confirm no persisted/consumedKeyDerivationdata relies on the old field numbers.Renumbering existing tags (
creator_did3→2,ring_id4→3, …) changes the wire format. Any previously serializedKeyDerivationrecords or external decoders would misread these fields. If this module is already deployed or has downstream consumers, prefer reserving the removed tag and keeping stable numbers instead of shifting:message KeyDerivation { string id = 1; reserved 2; // formerly namespace string creator_did = 3; string ring_id = 4; string derivation = 5; string policy_id = 6; string resource = 7; string permission = 8; }If the module is still pre-release with no persisted state or consumers, the renumbering is fine to keep.
proto/sourcehub/orbis/node_info.proto (1)
10-10: LGTM!proto/sourcehub/orbis/query.proto (1)
29-52: LGTM!Also applies to: 79-120
x/orbis/types/message_create_ring.go (1)
11-34: LGTM!x/orbis/types/message_finalize_ring.go (1)
9-22: LGTM!x/orbis/types/message_store_document.go (1)
11-21: LGTM!x/orbis/types/message_store_key_derivation.go (1)
11-20: LGTM!proto/sourcehub/orbis/document.proto (1)
9-19: 🏗️ Heavy liftDon’t renumber
Documentfields here; current tags are already stable.
proto/sourcehub/orbis/document.protocurrently definescreator_did = 2,ring_id = 3, …timestamp = 10with nonamespacefield (and noreserveddeclarations). The generatedx/orbis/types/document.pb.gomatches these same tag numbers, so the proposed direction that reserves field2and renumbers would actually change wire tags (e.g.,creator_did2→3) and risk incompatibility.> Likely an incorrect or invalid review comment.proto/sourcehub/orbis/ring.proto (1)
9-20: 🏗️ Heavy liftRing field numbers are already preserved; the claimed wire-format tag renumbering doesn’t match this schema.
proto/sourcehub/orbis/ring.protocurrently definesRingwith tagscreator_did=2,ring_pk=3,peer_node_keys=4,threshold=5,new_peer_node_keys=6,new_threshold=7,pss_interval=8,block_number_nonce=9,policy_id=10, andconfirmations=11, with nonamespacefield inRing. The protobuf compatibility break described (e.g., old tag3/4decoding into different fields) is not supported for this message as written.> Likely an incorrect or invalid review comment.proto/sourcehub/orbis/tx.proto (1)
45-55: 🏗️ Heavy liftPreserve protobuf field numbers in public tx messages when removing
namespace.
proto/sourcehub/orbis/tx.protono longer contains anamespacefield inMsgCreateRing(45-55),RingReshareFinalizeSignDoc(93-101), orMsgStoreDocument/MsgStoreKeyDerivation(114-141), and there are noreservedentries to keep old field tags unused. Ifnamespacewas removed previously, the old field number(s) must be left unused/reserved and the remaining fields must not be compacted/renumbered, since protobuf wire compatibility (including signed tx payloads) is driven by field numbers.api/sourcehub/orbis/key_derivation.pulsar.go (1)
838-844: 🏗️ Heavy liftRe-evaluate KeyDerivation wire-compatibility claim (no
namespacefield in current schema).
proto/sourcehub/orbis/key_derivation.protodefinesKeyDerivationasid=1,creator_did=2,ring_id=3,derivation=4,policy_id=5,resource=6,permission=7, andapi/sourcehub/orbis/key_derivation.pulsar.gomatches those field numbers. This repo’s current proto/generated code contains nonamespacefield, so the “fields shifted down after removingnamespace” premise isn’t supported here. The compatibility impact for already-persistedKeyDerivationrecords must be checked against the proto used by the previously deployed/encoded data; only if an earlier release actually had anamespacefield number should it be reserved in the source proto and regeneration performed.x/orbis/keeper/acp.go (1)
13-72: LGTM!x/orbis/keeper/validation.go (1)
12-52: LGTM!Also applies to: 81-177
x/orbis/types/acp.go (1)
1-10: LGTM!x/orbis/types/errors.go (1)
28-34: LGTM!x/orbis/module/autocli.go (1)
25-43: LGTM!Also applies to: 60-62, 79-87
x/orbis/keeper/msg_server.go (2)
28-81: LGTM!
215-256: LGTM!Also applies to: 258-308, 310-382, 406-450
x/orbis/keeper/grpc_query.go (1)
41-61: LGTM!Also applies to: 63-77, 79-99, 101-115, 130-150
x/orbis/keeper/store.go (1)
33-45: LGTM!Also applies to: 47-65, 75-93
x/orbis/types/keys.go (2)
42-73: LGTM!
31-39: Clarify intended semantics for peer-node key ordering vs. ring ID and reshare signing
GenerateRingIDhashespeerNodeKeysin the caller-provided order (x/orbis/types/keys.go), andvalidateRingonly enforces uniqueness/format (x/orbis/keeper/validation.go)—so the same peer set in a different order can produce a differentringIDand bypass theErrRingAlreadyExistscheck (x/orbis/keeper/msg_server.go). Meanwhile,ringReshareSignStatesortsPeerNodeKeys/NewPeerNodeKeysbefore hashing sign state (x/orbis/keeper/reshare_sign_bytes.go), making reshare sign-state order-insensitive.Confirm whether peer ordering is semantically meaningful (e.g. share/committee index assignment):
- If not meaningful,
GenerateRingIDshould canonicalize order too.- If meaningful, sorting in
ringReshareSignStateshould be removed or the index mapping should be encoded in the sign state/hash.x/orbis/keeper/optional.go (1)
13-19: LGTM!x/orbis/keeper/reshare_sign_bytes.go (1)
12-77: Ensure off-chain threshold signers match the updated ring reshare sign-bytes
ringReshareFinalizeSignBytesnow signs aRingReshareFinalizeSignDocwhereCurrentRingSha256/FinalizedRingSha256aresha256of the canonicalRingReshareSignState(withPeerNodeKeys/NewPeerNodeKeyssorted).RingReshareFinalizeSignDocin this repo has noNamespacefield; external signers must reproduce the exact protobuf-marshaled bytes of the new doc.- In-repo tests cover the new construction, but the external/off-chain signer code is not present here—needs manual lockstep update.
|
|
||
| Namespace string `protobuf:"bytes,1,opt,name=namespace,proto3" json:"namespace,omitempty"` | ||
| Pagination *v1beta1.PageRequest `protobuf:"bytes,2,opt,name=pagination,proto3" json:"pagination,omitempty"` | ||
| Pagination *v1beta1.PageRequest `protobuf:"bytes,1,opt,name=pagination,proto3" json:"pagination,omitempty"` |
There was a problem hiding this comment.
Avoid reusing removed protobuf field number 1 (wire compatibility break).
These requests now reuse tag 1 for different fields after removing namespace. That is a breaking protobuf change: older payloads can be misdecoded (namespace interpreted as id) or fail decode (namespace parsed as PageRequest). Keep existing field numbers (id/pagination as 2) and reserve removed namespace = 1 in the source .proto definitions.
Also applies to: 7483-7483, 7553-7553, 7631-7631, 7701-7701
🤖 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 `@api/sourcehub/orbis/query.pulsar.go` at line 7405, The protobuf field number
1 was reused for Pagination (Pagination *v1beta1.PageRequest
`protobuf:"bytes,1...`) after removing namespace which breaks wire
compatibility; update the protobuf definition so Pagination (and the related
id/pagination fields) use the original field number 2 instead of 1, and
explicitly reserve the removed namespace tag 1 (e.g., add a reserved 1 entry) in
the .proto to prevent reuse; apply the same change for the other occurrences
referenced (the other Pagination/id fields at the indicated locations) so no
removed tag is reassigned.
There was a problem hiding this comment.
this is not live yet no need for backwards compatability
There was a problem hiding this comment.
@JesseAbram, got it — if this API/schema hasn’t gone live yet, then there’s no backwards-compatibility requirement here. Reusing the removed namespace tag for the new id/pagination fields is fine in that context, and this shouldn’t block the PR.
🐇💭
| message MsgFinalizeRing { | ||
| option (cosmos.msg.v1.signer) = "creator"; | ||
| string creator = 1; | ||
| string ring_id = 2; | ||
| string ring_pk = 3; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="proto/sourcehub/orbis/tx.proto"
echo "== tx.proto imports + MsgFinalizeRing (lines ~50-90) =="
nl -ba "$FILE" | sed -n '1,120p'
echo
echo "== Search for cosmos.msg.v1.signer usage in repo =="
rg -n "cosmos\.msg\.v1\.signer" -S .
echo
echo "== Show tx.proto git diff hunks for the mentioned messages =="
git diff -U5 -- "$FILE" | sed -n '/MsgFinalizeRing/,+40p'
echo "----"
git diff -U5 -- "$FILE" | sed -n '/MsgCreateRing/,+80p'
echo "----"
git diff -U5 -- "$FILE" | sed -n '/RingReshareFinalizeSignDoc/,+120p'
echo "----"
git diff -U5 -- "$FILE" | sed -n '/MsgStoreDocument/,+120p'
echo "----"
git diff -U5 -- "$FILE" | sed -n '/MsgStoreKeyDerivation/,+120p'Repository: sourcenetwork/sourcehub
Length of output: 184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="proto/sourcehub/orbis/tx.proto"
echo "== Show tx.proto imports + MsgFinalizeRing context =="
awk 'NR>=1 && NR<=140 {printf "%d:%s\n", NR, $0}' "$FILE"
echo
echo "== Search for cosmos.msg.v1.signer usage in repo =="
rg -n "cosmos\.msg\.v1\.signer" -S . || true
echo
echo "== Show git diff hunks for the mentioned messages (if any) =="
git diff -U5 -- "$FILE" | sed -n '1,240p' || true
echo "----"
git diff -U5 -- "$FILE" | rg -n "(MsgFinalizeRing|MsgCreateRing|RingReshareFinalizeSignDoc|MsgStoreDocument|MsgStoreKeyDerivation)" -n || trueRepository: sourcenetwork/sourcehub
Length of output: 8367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== List buf config files =="
fd -t f "buf.*(ya?ml)" . | head -n 50 || true
echo
echo "== Search buf config for cosmos proto modules/extensions =="
rg -n "buf\.build|cosmos\/msg\/v1\/msg\.proto|cosmos\.msg\.v1\.signer|module:" -S . || true
echo
echo "== Search for compilation/CI scripts that might mention signer resolution =="
fd -t f "(Makefile|CI*|*.sh|*.yml|*.yaml)" . | head -n 200 || true
rg -n "buf (build|lint|generate|check)|buf\.lock|protovalidate|protoc|signer" -S . || trueRepository: sourcenetwork/sourcehub
Length of output: 50878
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== proto/buf.yaml =="
sed -n '1,200p' proto/buf.yaml
echo
echo "== proto/buf.work.yaml =="
sed -n '1,200p' proto/buf.work.yaml
echo
echo "== proto/buf.gen.*.yaml (summaries) =="
for f in proto/buf.gen.*.yaml; do
echo "--- $f ---"
sed -n '1,120p' "$f"
done
echo
echo "== Check if buf CLI is available =="
command -v buf >/dev/null && buf --version || echo "buf not found"
echo
echo "== Locate cosmos/msg/v1/msg.proto in local cache (GOPATH pkg/mod) =="
GOPATH="$(go env GOPATH 2>/dev/null || true)"
if [ -n "$GOPATH" ]; then
echo "GOPATH=$GOPATH"
# find proto file candidates
rg -n "option \\(cosmos\\.msg\\.v1\\.signer\\)" -S "$GOPATH/pkg/mod" --no-mmap --hidden --glob '*.proto' --glob '*msg.proto' 2>/dev/null | head -n 20 || true
# find any file containing the signer extension symbol
rg -n "cosmos\\.msg\\.v1\\.signer" -S "$GOPATH/pkg/mod" --no-mmap --hidden 2>/dev/null | head -n 50 || true
else
echo "go not available / GOPATH not found"
fiRepository: sourcenetwork/sourcehub
Length of output: 699
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== root buf.work.yaml =="
[ -f buf.work.yaml ] && sed -n '1,200p' buf.work.yaml || echo "buf.work.yaml not found"
echo
echo "== Check buf CLI =="
command -v buf >/dev/null && buf --version || echo "buf not found"
echo
echo "== Check Go toolchain (for module/proto cache search) =="
command -v go >/dev/null && go version || echo "go not found"
echo
echo "== Locate cosmos/msg/v1/msg.proto in buf module cache (protodeps) if present =="
# Try protodeps cache locations used by buf; ignore if missing.
for d in "${HOME}/.cache/buf" "${HOME}/.buf" "${HOME}/.local/share/buf" "/tmp"; do
if [ -d "$d" ]; then
echo "-- search base: $d --"
rg -n "message Msg" "$d" >/dev/null 2>&1 || true
fi
done
echo
echo "== Directly search for the signer extension definition in local filesystem =="
# This is best-effort; keep output small.
rg -n "extension\\s+google\\.protobuf\\.MessageOptions\\s*\\{[[:space:][:graph:]]*signer" -S "$HOME" 2>/dev/null | head -n 20 || true
rg -n "cosmos\\.msg\\.v1\\.signer" -S "$HOME" 2>/dev/null | head -n 20 || true
echo
echo "== If buf is available: run buf lint (output filtered to signer error) =="
if command -v buf >/dev/null; then
# Use workspace file if present; otherwise default.
# Filter output to lines mentioning "signer" or the reported file/line.
buf lint --error-format=compact 2>&1 | rg -n "tx\.proto:6[1-9]|cosmos\.msg\.v1\.signer|signer in this scope|MsgFinalizeRing" || true
fiRepository: sourcenetwork/sourcehub
Length of output: 2836
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== buf build (filter errors relevant to signer / orbis/tx.proto) =="
buf build 2>&1 | rg -n "orbis/tx\.proto|cosmos\.msg\.v1\.signer|cannot find|in this scope|signer" || true
echo
echo "== buf lint (filter errors relevant to signer / orbis/tx.proto) =="
buf lint --error-format=compact 2>&1 | rg -n "orbis/tx\.proto|cosmos\.msg\.v1\.signer|cannot find|in this scope|signer" || true
echo
echo "== git status / diff summary =="
git status --porcelain || true
git diff --stat || true
git diff --name-only || trueRepository: sourcenetwork/sourcehub
Length of output: 236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status =="
git status --porcelain -b || true
echo
echo "== buf build exit code + stderr head =="
set +e
buf build 2> /tmp/buf_build_err.txt
rc=$?
set -e
echo "buf build rc=$rc"
wc -l /tmp/buf_build_err.txt 2>/dev/null || true
head -n 80 /tmp/buf_build_err.txt 2>/dev/null || true
echo "--- last 80 lines ---"
tail -n 80 /tmp/buf_build_err.txt 2>/dev/null || true
echo
echo "== buf lint exit code + stderr head =="
set +e
buf lint --error-format=compact 2> /tmp/buf_lint_err.txt
rc2=$?
set -e
echo "buf lint rc=$rc2"
wc -l /tmp/buf_lint_err.txt 2>/dev/null || true
head -n 80 /tmp/buf_lint_err.txt 2>/dev/null || true
echo "--- last 80 lines ---"
tail -n 80 /tmp/buf_lint_err.txt 2>/dev/null || true
echo
echo "== git diff name-only (if any) =="
git diff --name-only || true
echo
echo "== git diff --stat (if any) =="
git diff --stat || trueRepository: sourcenetwork/sourcehub
Length of output: 2921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== buf lint (scoped to orbis tx.proto) =="
set +e
buf lint --path proto/sourcehub/orbis/tx.proto 2>&1 | rg -n "MsgFinalizeRing|cosmos\.msg\.v1\.signer|cannot find|in this scope|signer" || true
echo "rc=$?"
set -e
echo
echo "== buf build (scoped to orbis tx.proto) =="
set +e
buf build --path proto/sourcehub/orbis/tx.proto 2>&1 | rg -n "MsgFinalizeRing|cosmos\.msg\.v1\.signer|cannot find|in this scope|signer" || true
echo "rc=$?"
set -eRepository: sourcenetwork/sourcehub
Length of output: 697
Remove the “MsgFinalizeRing protobuf compile blocker” claim and fix lint docs
proto/sourcehub/orbis/tx.protobuilds successfully withbuf build;(cosmos.msg.v1.signer)is accepted onMsgFinalizeRing(lines 61-66), so there’s no compile-time signer option failure.buf lintrequires non-empty documentation comments forMsgFinalizeRing,MsgFinalizeRingResponse,MsgFinalizeRingReshareByThresholdSignature,MsgFinalizeRingReshareByThresholdSignatureResponse(lines 61, 68, 83, 91).- If this PR changed numeric field tags for
MsgCreateRing,RingReshareFinalizeSignDoc,MsgStoreDocument, orMsgStoreKeyDerivation, keep them stable to avoid a public API break.
🧰 Tools
🪛 Buf (1.69.0)
[error] 62-62: cannot find cosmos.msg.v1.signer in this scope
(COMPILE)
🤖 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 `@proto/sourcehub/orbis/tx.proto` around lines 61 - 66, The PR incorrectly
claims a protobuf compile blocker for MsgFinalizeRing — remove that claim and
instead add non-empty doc comments for the messages required by buf lint:
MsgFinalizeRing, MsgFinalizeRingResponse,
MsgFinalizeRingReshareByThresholdSignature, and
MsgFinalizeRingReshareByThresholdSignatureResponse; ensure each message has a
short descriptive comment above it, and verify that numeric field tags for
MsgCreateRing, RingReshareFinalizeSignDoc, MsgStoreDocument, and
MsgStoreKeyDerivation were not changed (restore original tags if they were) to
avoid a public API break.
| // Check for a conflicting ring_pk from a prior confirmation by a | ||
| // different node. This is a genuine BFT violation — delete the ring. | ||
| for _, c := range ring.Confirmations { | ||
| if c.RingPk != msg.RingPk { | ||
| _ = ctx.EventManager().EmitTypedEvent(&types.EventRingDeleted{ | ||
| RingId: ring.Id, | ||
| Reason: "ring_pk_conflict", | ||
| }) | ||
| k.DeleteRing(goCtx, ring.Id) | ||
| return nil, types.ErrRingPkConflict | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Cosmos SDK v0.50, are state changes (KVStore writes) made inside a message handler rolled back if the handler returns an error?
💡 Result:
Yes, in Cosmos SDK v0.50, state changes (KVStore writes) made inside a message handler are automatically rolled back if the handler returns an error [1]. The Cosmos SDK is designed to ensure atomicity for transaction execution [1]. When a transaction is processed, the state changes are applied to a cached or branched version of the store [1]. If the message handler successfully completes (returns nil error), these changes are committed to the persistent state [2][1]. If the handler returns an error, those pending state changes are discarded, effectively rolling back the operation [1]. This mechanism provides a safety guarantee for developers, ensuring that failed transactions do not leave the chain in a partial or inconsistent state [1]. Note that this error-handling protection applies specifically to message execution and does not extend to other lifecycle hooks like BeginBlocker or EndBlocker, where panics or errors may have different impacts on chain stability [1].
Citations:
- 1: https://osec.io/blog/2025-06-10-cosmos-security/
- 2: [Bug]: state changes during posthandler are ignored if tx is failed cosmos/cosmos-sdk#22396
Fix ring deletion on ring_pk conflict: returning types.ErrRingPkConflict rolls back the state change in Cosmos SDK v0.50.
In Cosmos SDK v0.50, KVStore writes performed during message execution (cached/brached context) are discarded when the handler returns an error, so k.DeleteRing(goCtx, ring.Id) cannot persist if you return types.ErrRingPkConflict. Make the conflict path return success to commit the deletion.
🐛 Proposed fix: commit the deletion by returning success
for _, c := range ring.Confirmations {
if c.RingPk != msg.RingPk {
- _ = ctx.EventManager().EmitTypedEvent(&types.EventRingDeleted{
+ k.DeleteRing(goCtx, ring.Id)
+ if err := ctx.EventManager().EmitTypedEvent(&types.EventRingDeleted{
RingId: ring.Id,
Reason: "ring_pk_conflict",
- })
- k.DeleteRing(goCtx, ring.Id)
- return nil, types.ErrRingPkConflict
+ }); err != nil {
+ return nil, err
+ }
+ return &types.MsgFinalizeRingResponse{}, nil
}
}🤖 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 `@x/orbis/keeper/msg_server.go` around lines 119 - 130, The handler currently
deletes the ring then returns types.ErrRingPkConflict which causes KV writes to
be rolled back; change the conflict branch so after emitting EventRingDeleted
and calling k.DeleteRing you do NOT return types.ErrRingPkConflict but instead
return a successful response (i.e., a non-error result for the Msg confirm
handler) so the deletion persists. Update the branch that iterates
ring.Confirmations (checking c.RingPk vs msg.RingPk), the
ctx.EventManager().EmitTypedEvent(types.EventRingDeleted) call, and k.DeleteRing
invocation to return a successful MsgConfirm response (and nil error) rather
than ErrRingPkConflict.
| if err := validateRingUpdate(msg.NewPeerNodeKeys, newThreshold, ring); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if len(msg.NewPeerIds) > 0 { | ||
| ring.NewPeerIds = append([]string(nil), msg.NewPeerIds...) | ||
| if len(msg.NewPeerNodeKeys) > 0 { | ||
| ring.NewPeerNodeKeys = append([]string(nil), msg.NewPeerNodeKeys...) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect validateRingUpdate and whether it checks node info existence for new peer keys.
rg -nP --type=go -C4 'func\s+validateRingUpdate\s*\('
# Also check whether GetNodeInfo is referenced from validation.go
rg -nP --type=go -C3 'GetNodeInfo' -g 'x/orbis/keeper/validation.go'Repository: sourcenetwork/sourcehub
Length of output: 721
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect full validateRingUpdate body
echo "---- x/orbis/keeper/validation.go: validateRingUpdate ----"
nl -ba x/orbis/keeper/validation.go | sed -n '35,160p'
# 2) Search for node-info related checks in validation.go (broaden beyond GetNodeInfo)
echo "---- rg in x/orbis/keeper/validation.go for likely node-info terms ----"
rg -n --hidden --no-ignore --type=go 'NodeInfo|node info|peer node|registered|registered node|GetNodeInfo' x/orbis/keeper/validation.go || true
# 3) Inspect msg_server.go around CreateRing and UpdateRingByAcp to see what invariants are enforced in update path
echo "---- x/orbis/keeper/msg_server.go: around CreateRing & UpdateRingByAcp ----"
nl -ba x/orbis/keeper/msg_server.go | sed -n '1,260p'
# 4) Search msg_server.go for validateRingUpdate call site + any node-info enforcement in update handler
echo "---- rg in msg_server.go for validateRingUpdate and node-info terms ----"
rg -n --type=go 'validateRingUpdate|UpdateRingByAcp|CreateRing|GetNodeInfo|NodeInfo|node info|registered' x/orbis/keeper/msg_server.goRepository: sourcenetwork/sourcehub
Length of output: 187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Helper to print numbered lines using awk (since nl isn't available)
print_range() {
local file="$1"
local start="$2"
local end="$3"
awk -v s="$start" -v e="$end" '{if(NR>=s && NR<=e) printf "%d:%s\n", NR, $0}' "$file"
}
echo "---- validation.go: inspect validateRingUpdate body (w/ line numbers) ----"
print_range x/orbis/keeper/validation.go 35 200
echo "---- validation.go: broader node-info related searches ----"
rg -n --type=go 'node info|NodeInfo|GetNodeInfo|registered|registration|peer node|PeerNodeKeys' x/orbis/keeper/validation.go || true
echo "---- msg_server.go: inspect msg_server around CreateRing & UpdateRingByAcp ----"
print_range x/orbis/keeper/msg_server.go 1 260
echo "---- msg_server.go: search for validateRingUpdate call sites and node-info terms ----"
rg -n --type=go 'validateRingUpdate|UpdateRingByAcp|CreateRing|NodeInfo|GetNodeInfo|node info|registered|registration|peer node|PeerNodeKeys' x/orbis/keeper/msg_server.go || trueRepository: sourcenetwork/sourcehub
Length of output: 17974
🏁 Script executed:
#!/bin/bash
set -euo pipefail
awk 'NR>=1 && NR<=120 {print NR ":" $0}' x/orbis/keeper/validation.goRepository: sourcenetwork/sourcehub
Length of output: 4240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- locate signerPublicKeyHex ----"
rg -n --type=go 'func\s+signerPublicKeyHex\s*\(' x/orbis/keeper || true
echo "---- locate GetNodeInfo usage in finalize / reshare paths ----"
rg -n --type=go 'GetNodeInfo|FinalizeRing|FinalizeRingReshare' x/orbis/keeper/msg_server.go x/orbis/keeper -S || true
echo "---- show signerPublicKeyHex implementation (if found) ----"
# If signerPublicKeyHex exists, print a small surrounding window
f="$(rg -l --type=go 'func\s+signerPublicKeyHex\s*\(' x/orbis/keeper || true | head -n 1)"
if [ -n "${f:-}" ]; then
rg -n --type=go -C5 'func\s+signerPublicKeyHex\s*\(' "$f"
fiRepository: sourcenetwork/sourcehub
Length of output: 7599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print full signerPublicKeyHex
awk 'NR>=360 && NR<=430 {print NR ":" $0}' x/orbis/keeper/msg_server.go
# Locate and print verifyThresholdSignatureForRingUpdate
rg -n --type=go 'verifyThresholdSignatureForRingUpdate|ringReshareFinalizeSignBytes|ringReshare' x/orbis/keeper || true
vf="$(rg -l --type=go 'func\s+verifyThresholdSignatureForRingUpdate\s*\(' x/orbis/keeper || true | head -n 1)"
if [ -n "${vf:-}" ]; then
echo "---- verifyThresholdSignatureForRingUpdate body ----"
awk 'NR>=1 && NR<=260 {print NR ":" $0}' "$vf"
fi
# Search for GetNodeInfo usage anywhere in keepers (beyond those already found)
echo "---- GetNodeInfo usage across x/orbis/keeper (broad) ----"
rg -n --type=go 'GetNodeInfo' x/orbis/keeper || trueRepository: sourcenetwork/sourcehub
Length of output: 8748
Ensure ring update/reshare new peer node keys require registered node info
CreateRingrejects anymsg.PeerNodeKeyswithoutk.GetNodeInfo(...).UpdateRingByAcpvalidates/copiesmsg.NewPeerNodeKeysviavalidateRingUpdate+validateRingonly (format/uniqueness/threshold); neither path checksGetNodeInfofor the new keys.FinalizeRingauthorizes confirmations by comparing the tx signer’s public key (signerPublicKeyHex) toring.PeerNodeKeys; it does not consultGetNodeInfo, so missing node-info registration alone doesn’t block confirmations in this code path.
Enforce the same node-info existence check for msg.NewPeerNodeKeys during UpdateRingByAcp (or document why this invariant is intentionally not required for reshare-installed peers).
🤖 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 `@x/orbis/keeper/msg_server.go` around lines 186 - 192, Update UpdateRingByAcp
flow to enforce that every key in msg.NewPeerNodeKeys has registered node info
via k.GetNodeInfo(...) before accepting/copying them (similar invariant enforced
in CreateRing); specifically, inside the UpdateRingByAcp handler call
validateRingUpdate/validateRing as before but add an existence check: iterate
msg.NewPeerNodeKeys and call k.GetNodeInfo(ctx, pkHex) for each, returning an
error if any are missing, then proceed to set ring.NewPeerNodeKeys =
append(...). Also update FinalizeRing authorization to consult
k.GetNodeInfo(...) when validating signerPublicKeyHex against ring.PeerNodeKeys
(or otherwise fail) so unregistered keys cannot confirm; reference
functions/paths: UpdateRingByAcp, validateRingUpdate, validateRing,
FinalizeRing, CreateRing, and k.GetNodeInfo.
| if newThreshold != nil && *newThreshold < 1 { | ||
| return errorsmod.Wrap(types.ErrInvalidRing, "new_threshold must be at least 1") | ||
| } | ||
| if len(newPeerIDs) > 0 && newThreshold != nil && uint32(len(newPeerIDs)) < *newThreshold { | ||
| if len(newPeerNodeKeys) > 0 && newThreshold != nil && uint32(len(newPeerNodeKeys)) < *newThreshold { | ||
| return errorsmod.Wrapf( | ||
| types.ErrInvalidRing, | ||
| "new_peer_ids count (%d) is less than new_threshold (%d)", | ||
| len(newPeerIDs), | ||
| "new_peer_node_keys count (%d) is less than new_threshold (%d)", | ||
| len(newPeerNodeKeys), | ||
| *newThreshold, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Validate newThreshold against existing committee size for threshold-only reshares.
The count-vs-threshold guard only fires when both newPeerNodeKeys and newThreshold are provided. A threshold-only reshare (membership unchanged) can set newThreshold greater than len(existing.PeerNodeKeys). This passes here, but at finalization ringForReshareFinalization keeps the existing committee and validateRing then rejects threshold > committee size, leaving the ring unfinalizable and blocked from starting another reshare (ErrReshareInProgress).
🛡️ Proposed guard for threshold-only reshare
if len(newPeerNodeKeys) > 0 && newThreshold != nil && uint32(len(newPeerNodeKeys)) < *newThreshold {
return errorsmod.Wrapf(
types.ErrInvalidRing,
"new_peer_node_keys count (%d) is less than new_threshold (%d)",
len(newPeerNodeKeys),
*newThreshold,
)
}
+ if len(newPeerNodeKeys) == 0 && newThreshold != nil && uint32(len(existing.PeerNodeKeys)) < *newThreshold {
+ return errorsmod.Wrapf(
+ types.ErrInvalidRing,
+ "new_threshold (%d) exceeds existing committee size (%d)",
+ *newThreshold,
+ len(existing.PeerNodeKeys),
+ )
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if newThreshold != nil && *newThreshold < 1 { | |
| return errorsmod.Wrap(types.ErrInvalidRing, "new_threshold must be at least 1") | |
| } | |
| if len(newPeerIDs) > 0 && newThreshold != nil && uint32(len(newPeerIDs)) < *newThreshold { | |
| if len(newPeerNodeKeys) > 0 && newThreshold != nil && uint32(len(newPeerNodeKeys)) < *newThreshold { | |
| return errorsmod.Wrapf( | |
| types.ErrInvalidRing, | |
| "new_peer_ids count (%d) is less than new_threshold (%d)", | |
| len(newPeerIDs), | |
| "new_peer_node_keys count (%d) is less than new_threshold (%d)", | |
| len(newPeerNodeKeys), | |
| *newThreshold, | |
| ) | |
| } | |
| if len(newPeerNodeKeys) > 0 && newThreshold != nil && uint32(len(newPeerNodeKeys)) < *newThreshold { | |
| return errorsmod.Wrapf( | |
| types.ErrInvalidRing, | |
| "new_peer_node_keys count (%d) is less than new_threshold (%d)", | |
| len(newPeerNodeKeys), | |
| *newThreshold, | |
| ) | |
| } | |
| if len(newPeerNodeKeys) == 0 && newThreshold != nil && uint32(len(existing.PeerNodeKeys)) < *newThreshold { | |
| return errorsmod.Wrapf( | |
| types.ErrInvalidRing, | |
| "new_threshold (%d) exceeds existing committee size (%d)", | |
| *newThreshold, | |
| len(existing.PeerNodeKeys), | |
| ) | |
| } |
🤖 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 `@x/orbis/keeper/validation.go` around lines 66 - 76, The current guard only
compares new_peer_node_keys vs newThreshold when both are provided, allowing a
threshold-only reshare to set newThreshold larger than the existing committee;
add a check in the validation path (same function where newThreshold and
newPeerNodeKeys are inspected) that if newPeerNodeKeys is empty and newThreshold
!= nil you validate *newThreshold <= uint32(len(existing.PeerNodeKeys)) and
return errorsmod.Wrapf(types.ErrInvalidRing, "new_threshold (%d) cannot exceed
existing committee size (%d)", *newThreshold, len(existing.PeerNodeKeys)) when
it violates this; this prevents ringForReshareFinalization/validateRing from
later rejecting the change and leaving ErrReshareInProgress.
2 participants
No description provided.