fix(ws): use uint64 for params.Subscription in incoming notifications#427
Open
ozpool wants to merge 1 commit into
Open
fix(ws): use uint64 for params.Subscription in incoming notifications#427ozpool wants to merge 1 commit into
ozpool wants to merge 1 commit into
Conversation
The websocket pubsub response type was defined as
type params struct {
Result *stdjson.RawMessage `json:"result"`
Subscription int `json:"subscription"`
}
but the validator emits subscription ids as JSON unsigned 64-bit
integers. With the field typed as int the decode in
decodeResponseFromMessage fails on 32-bit builds for any id above
math.MaxInt32, and on 64-bit builds for any id above math.MaxInt64
— the path users hit in practice when the validator hands out a
sufficiently large id (issue solana-foundation#286).
The Subscription field is not consumed downstream — handleMessage
parses the routing key separately via getUint64WithOk — but it has
to be present so encoding/json does not error on the inbound
notification. Switching the type to uint64 keeps that contract and
matches what the validator and handleMessage already use everywhere.
Adds a regression test that decodes a notification with subscription
== math.MaxUint64 to pin the new behavior.
Closes solana-foundation#286.
Author
|
Hi @sonicfromnewyoke — also ~19 days open with no review. Small |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #286.
Bug
The websocket pubsub
responsetype inrpc/ws/types.gowas defined as:The validator emits subscription ids as JSON unsigned 64-bit integers. With the field typed as
intthe decode indecodeResponseFromMessagefails on 32-bit builds for any id abovemath.MaxInt32, and on 64-bit builds for any id abovemath.MaxInt64. That is exactly the path users hit in #286:unable to decode client responsefromSlotSubscribe()(and the same shape for any other ws subscription). The bug was diagnosed in the issue thread by @AliakseiM, who pointed at this exact field.Fix
Switch
Subscriptiontouint64. The field is not consumed downstream —handleMessageparses the routing key separately viagetUint64WithOk— but it has to be present soencoding/jsondoes not error on the inbound notification.uint64matches what the validator andhandleMessagealready use everywhere, and lines up with the recent #400 work that hardened request ids against the same JSON-safe-integer footgun.Tests
TestResponseDecodesLargeSubscriptionID(new) decodes a notification withsubscription == math.MaxUint64to pin the new behavior.Diff size