rpc/jsonrpc: reject out-of-int-range float IDs in idFromInterface (#5846)#5861
Open
satyakwok wants to merge 4 commits into
Open
rpc/jsonrpc: reject out-of-int-range float IDs in idFromInterface (#5846)#5861satyakwok wants to merge 4 commits into
satyakwok wants to merge 4 commits into
Conversation
…ometbft#1693) For replay paths that only need validator addresses + voting powers (buildLastCommitInfo, buildExtendedCommitInfo), advancing proposer priorities forward from the most recent stored checkpoint is wasted work. The advance is O(height % valSetCheckpointInterval) and dominates replay time on chains with infrequent validator-set changes (the issue reports ~10x slowdown, this PR's benchmark shows up to 903x at worst case). Add Store.LoadValidatorsFast that returns the ValidatorSet without calling IncrementProposerPriority. The returned set has correct addresses, public keys, and voting powers; ProposerPriority reflects the last stored checkpoint, not the requested height. Callers needing accurate proposer priority must continue to use LoadValidators. Wire buildLastCommitInfoFromStore and buildExtendedCommitInfoFromStore to use the fast path. Both downstream paths only call TM2PB.Validator (types/protobuf.go:41) which reads PubKey.Address() and VotingPower only — no proposer priority access. Bench results (state/store_test.go BenchmarkLoadValidatorsSawtooth) on AMD EPYC, 100-validator set, ValidatorsInfo header at offset queried but ValidatorSet only stored at checkpoint: | Offset | LoadValidators | LoadValidatorsFast | Speedup | |--------------|----------------|--------------------|---------| | at_checkpoint| 165 us | 158 us | 1.04x | | +1 | 149 us | 137 us | 1.09x | | +100 | 270 us | 134 us | 2.0x | | +10000 | 12.4 ms | 132 us | 94x | | +99999 | 126 ms | 140 us | 903x | Refactors LoadValidators internals into a shared loadValidatorsAtHeight helper so both fast and full paths share the lookup logic. Closes cometbft#1693
…metbft#5846) json.Unmarshal decodes JSON numbers as float64. A naive int(id) for values above math.MaxInt or below math.MinInt saturates to math.MinInt on amd64 (cvttsd2si), so distinct large numeric IDs collide on the same negative value with no error. This breaks request/response correlation for any client keying on the numeric id. Add a float64 -> int -> float64 round-trip check on the float64 arm and reject NaN / +-Inf explicitly. The round-trip catches the tricky 2^63 boundary that a naive 'id > math.MaxInt' check misses (because float64(math.MaxInt64) rounds up to 2^63). Tests cover in-range int, fractional, NaN, +-Inf, the 2^63 boundary, -2^63 - 1, very large finite (1e20), and an oversized-collision case that previously produced the same math.MinInt for five distinct inputs. Closes cometbft#5846
Contributor
|
PR author is not in the allowed authors list. |
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
1 participant
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 #5846.
json.Unmarshaldecodes JSON numbers asfloat64. A naiveint(id)for values abovemath.MaxIntor belowmath.MinIntsaturates tomath.MinInton amd64 (cvttsd2si), so distinct large numeric IDs collide on the same negative integer with no error. This silently breaks request/response correlation for any client keying on the numeric id.Fix
Add a
float64 -> int -> float64round-trip check on thefloat64arm ofidFromInterfaceand reject NaN / +-Inf explicitly. The round-trip catches the tricky2^63boundary that a naiveid > math.MaxIntcheck misses (becausefloat64(math.MaxInt64)rounds up to2^63).Tests
The
oversized_ids_do_not_collidecase is the regression pin — five distinct oversized inputs that previously all producedJSONRPCIntID(math.MinInt)must now each return an error.Existing
TestResponses/TestUnmarshallResponsesstill pass.Compatibility