Skip to content

Stable hashing return first node >= topicHash#1972

Closed
fbac wants to merge 1 commit intomainfrom
fbac/honor-comment
Closed

Stable hashing return first node >= topicHash#1972
fbac wants to merge 1 commit intomainfrom
fbac/honor-comment

Conversation

@fbac
Copy link
Copy Markdown
Collaborator

@fbac fbac commented Apr 13, 2026

Fix binary search predicate in StableHashingNodeSelectorAlgorithm.GetNode to return first node >= topicHash

Fixes an inverted predicate in the binary search in stable.go. The condition was changed from topicHash < nodeLocations[i] to topicHash >= nodeLocations[i], so the algorithm now correctly selects the first node whose virtual position is greater than or equal to the topicHash. Risk: this changes which node is selected for every topic and banlist combination, affecting routing in production.

📊 Macroscope summarized d02f8ee. 1 file reviewed, 1 issue evaluated, 0 issues filtered, 1 comment posted

🗂️ Filtered Issues

@fbac fbac requested a review from a team as a code owner April 13, 2026 17:57
@octane-security-app
Copy link
Copy Markdown

Summary by Octane

New Contracts

No new contracts were added.

Updated Contracts

  • stable.go: Revised binary search logic to include topicHash in node selection, enhancing node matching efficiency.

🔗 Commit Hash: d02f8ee

Comment on lines 69 to 71
idx := sort.Search(len(nodeLocations), func(i int) bool {
return topicHash < nodeLocations[i]
return topicHash >= nodeLocations[i]
})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High selectors/stable.go:69

The binary search predicate topicHash >= nodeLocations[i] is inverted, causing sort.Search to always return idx = 0. Since nodeLocations[0] = 0 and any topicHash >= 0 is always true for unsigned integers, all topics route to the first node regardless of hash value, defeating the stable hashing distribution. The correct predicate should find the first node location strictly greater than topicHash.

-	idx := sort.Search(len(nodeLocations), func(i int) bool {
-		return topicHash >= nodeLocations[i]
-	})
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file pkg/api/payer/selectors/stable.go around lines 69-71:

The binary search predicate `topicHash >= nodeLocations[i]` is inverted, causing `sort.Search` to always return `idx = 0`. Since `nodeLocations[0] = 0` and any `topicHash >= 0` is always true for unsigned integers, all topics route to the first node regardless of hash value, defeating the stable hashing distribution. The correct predicate should find the first node location strictly greater than `topicHash`.

Evidence trail:
pkg/api/payer/selectors/stable.go lines 60-71 (REVIEWED_COMMIT) - Shows nodeLocations[i] = uint32(i) * spacing with nodeLocations[0] = 0, and the predicate `topicHash >= nodeLocations[i]` which is always true when i=0 since any uint32 >= 0. Go sort.Search documentation at https://pkg.go.dev/sort#Search confirms it returns the first index where f(i) is true, assuming false→true monotonicity.

@macroscopeapp
Copy link
Copy Markdown
Contributor

macroscopeapp bot commented Apr 13, 2026

Approvability

Verdict: Needs human review

1 blocking correctness issue found. This change modifies the binary search predicate in the stable hashing algorithm. An open review comment identifies that the new predicate topicHash >= nodeLocations[i] will always be true at index 0 (since nodeLocations[0]=0 and uint32 is always >=0), causing all topics to route to the first node and breaking hash distribution. This potential bug in core routing logic requires human review.

You can customize Macroscope's approvability policy. Learn more.

Copy link
Copy Markdown
Collaborator

@mkysel mkysel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the original code is correct

@fbac fbac closed this Apr 13, 2026
@octane-security-app
Copy link
Copy Markdown

Overview

Vulnerabilities found: 9                                                                                
Severity breakdown: 2 High, 3 Medium, 3 Low, 1 Informational

Detailed findings

pkg/api/message/subscribe_topics.go

  • Missing gRPC payload size batching in SubscribeTopics causes stream termination/DoS. See more

pkg/api/payer/selectors/stable.go

  • Comparator bug in StableHashingNodeSelectorAlgorithm (payer gateway) causes routing collapse and availability degradation. See more
  • Comparator inversion in stable hashing node selector causes multi-origin catch-up starvation. See more
  • Incorrect binary-search predicate in payer stable hashing selector causes temporary omission of topic results in topic-based reads. See more

pkg/db/subscription.go

  • Blocking unbuffered send without context select in DBSubscription during shutdown causes goroutine retention and stuck funnel. See more

pkg/interceptors/server/rate_limit.go

  • Unthrottled legacy ReplicationApi endpoints in API server cause DoS via rate-limit bypass. See more

pkg/payerreport/workers/settlement.go

  • Writing SubmittedReportIndex=0 on AlreadySubmitted in payer report submitter causes premature settlement/usage clearing or settlement stall. See more

pkg/payerreport/workers/submitter.go

  • Missing ActiveNodeIDs verification and report ID re-derivation in payer report verifier causes settlement stalls and gas waste. See more

pkg/sync/originator_stream.go

  • Missing originator signature verification and inner/outer originator binding in sync receiver allows tampered envelopes and local accounting/retention distortion. See more

🔗 Commit Hash: d02f8ee
🛡️ Octane Dashboard: All vulnerabilities

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants