feat(arbiter): Implement Byzantine Swarm Protection - #705
Conversation
Reviewer's GuideAdds a new Arbiter component to handle reputation-weighted quorum decisions and replaces the previous minimax test with tests for quorum calculation. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ca1266ea5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if totalReputation == 0 { | ||
| return false | ||
| } | ||
| return (positiveReputation / totalReputation) >= a.QuorumThreshold |
There was a problem hiding this comment.
CalculateQuorum fails open when QuorumThreshold is left at its Go zero value (0): with any authorized nodes present, positiveReputation/totalReputation can be 0, and 0 >= 0 returns true. That means a caller that forgets to initialize the threshold can approve quorum even when nobody voted yes, which defeats the safety property this arbiter is meant to enforce.
Useful? React with 👍 / 👎.
| totalReputation += node.Reputation | ||
| if vote, ok := votes[node.ID]; ok && vote { | ||
| positiveReputation += node.Reputation |
There was a problem hiding this comment.
The quorum calculation sums raw Reputation values directly into both numerator and denominator without validating sign, so negative weights can distort the ratio (e.g., make it exceed 1, go negative, or let a single negative-weight vote force or block quorum). Since reputation is a free float64, this creates incorrect consensus outcomes whenever any authorized node’s score drops below zero.
Useful? React with 👍 / 👎.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider validating or normalizing
QuorumThreshold(e.g., ensuring it’s within [0,1]) when constructing or updating anArbiter, soCalculateQuorumdoesn’t silently accept out-of-range thresholds. - It may be worth documenting and/or asserting the behavior when
totalReputation == 0(no authorized nodes or all zero reputation), since always returningfalsein that case could mask configuration errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider validating or normalizing `QuorumThreshold` (e.g., ensuring it’s within [0,1]) when constructing or updating an `Arbiter`, so `CalculateQuorum` doesn’t silently accept out-of-range thresholds.
- It may be worth documenting and/or asserting the behavior when `totalReputation == 0` (no authorized nodes or all zero reputation), since always returning `false` in that case could mask configuration errors.
## Individual Comments
### Comment 1
<location path="phoenix_os/arbiter/src/arbiter.go" line_range="23-25" />
<code_context>
+}
+
+// Arbiter handles quorum and reputation
+type Arbiter struct {
+ Nodes []Node
+ QuorumThreshold float64
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Validate or constrain QuorumThreshold to avoid surprising behavior
If QuorumThreshold is set <= 0 or > 1, quorum checks could effectively always fail or always pass. Please either clamp this to [0,1] or enforce validation (e.g., in a constructor/setter) so invalid thresholds are rejected early.
Suggested implementation:
```golang
// Arbiter handles quorum and reputation
type Arbiter struct {
Nodes []Node
QuorumThreshold float64
}
// NewArbiter constructs an Arbiter with a validated quorum threshold.
// QuorumThreshold must be in the (0,1] range; values outside this range will
// cause NewArbiter to return an error.
func NewArbiter(nodes []Node, quorumThreshold float64) (*Arbiter, error) {
if quorumThreshold <= 0 || quorumThreshold > 1 {
return nil, fmt.Errorf("quorumThreshold must be in the range (0,1], got %f", quorumThreshold)
}
return &Arbiter{
Nodes: nodes,
QuorumThreshold: quorumThreshold,
}, nil
}
```
1. Ensure `fmt` is imported at the top of `phoenix_os/arbiter/src/arbiter.go`, e.g.:
`import "fmt"`, or added to the existing import block.
2. Update any existing code that instantiates `Arbiter` via struct literals to instead call `NewArbiter` (or, if you must keep struct literals, make sure they always set a valid `QuorumThreshold`).
3. If there are other constructors or setters for `Arbiter` elsewhere in the file, align them with the same validation logic so `QuorumThreshold` is always constrained to `(0,1]`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| type Arbiter struct { | ||
| Nodes []Node | ||
| QuorumThreshold float64 |
There was a problem hiding this comment.
suggestion (bug_risk): Validate or constrain QuorumThreshold to avoid surprising behavior
If QuorumThreshold is set <= 0 or > 1, quorum checks could effectively always fail or always pass. Please either clamp this to [0,1] or enforce validation (e.g., in a constructor/setter) so invalid thresholds are rejected early.
Suggested implementation:
// Arbiter handles quorum and reputation
type Arbiter struct {
Nodes []Node
QuorumThreshold float64
}
// NewArbiter constructs an Arbiter with a validated quorum threshold.
// QuorumThreshold must be in the (0,1] range; values outside this range will
// cause NewArbiter to return an error.
func NewArbiter(nodes []Node, quorumThreshold float64) (*Arbiter, error) {
if quorumThreshold <= 0 || quorumThreshold > 1 {
return nil, fmt.Errorf("quorumThreshold must be in the range (0,1], got %f", quorumThreshold)
}
return &Arbiter{
Nodes: nodes,
QuorumThreshold: quorumThreshold,
}, nil
}- Ensure
fmtis imported at the top ofphoenix_os/arbiter/src/arbiter.go, e.g.:
import "fmt", or added to the existing import block. - Update any existing code that instantiates
Arbitervia struct literals to instead callNewArbiter(or, if you must keep struct literals, make sure they always set a validQuorumThreshold). - If there are other constructors or setters for
Arbiterelsewhere in the file, align them with the same validation logic soQuorumThresholdis always constrained to(0,1].
There was a problem hiding this comment.
Pull request overview
This PR aims to add Byzantine-resistance primitives to the Phoenix Arbiter by introducing node authorization and a reputation-weighted quorum check.
Changes:
- Added
NodeandArbitertypes to represent swarm participants and quorum configuration. - Implemented
Arbiter.CalculateQuorumto compute quorum using reputation-weighted votes from authorized nodes. - Replaced the existing minimax unit test with a new quorum unit test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| phoenix_os/arbiter/src/arbiter.go | Adds node/arbiter data structures and implements reputation-weighted quorum calculation. |
| phoenix_os/arbiter/src/arbiter_test.go | Adds tests for the new quorum calculation logic and removes the previous minimax test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, node := range a.Nodes { | ||
| if !node.Authorized { | ||
| continue | ||
| } | ||
| totalReputation += node.Reputation | ||
| if vote, ok := votes[node.ID]; ok && vote { | ||
| positiveReputation += node.Reputation |
| if totalReputation == 0 { | ||
| return false | ||
| } | ||
| return (positiveReputation / totalReputation) >= a.QuorumThreshold | ||
| } |
| // Node represents a participant in the swarm | ||
| type Node struct { | ||
| ID string | ||
| Reputation float64 | ||
| Authorized bool | ||
| } | ||
|
|
||
| // Arbiter handles quorum and reputation | ||
| type Arbiter struct { | ||
| Nodes []Node | ||
| QuorumThreshold float64 | ||
| } |
| func TestCalculateQuorum(t *testing.T) { | ||
| arbiter := Arbiter{ | ||
| Nodes: []Node{ | ||
| {"node1", 1.0, true}, | ||
| {"node2", 2.0, true}, | ||
| {"node3", 1.0, false}, | ||
| }, | ||
| QuorumThreshold: 0.6, | ||
| } |
Closes #673. Implements Proof-of-Authority, node reputation scoring, and weighted quorum consensus for swarm coordination.
Summary by Sourcery
Introduce reputation-weighted quorum calculation for arbiter-based swarm coordination.
New Features:
Tests: