-
Notifications
You must be signed in to change notification settings - Fork 75
937 follow-ups: getTransactions cursor guards, getHealth first-commit gate, snapshot gauge, method-table completeness test #953
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
f0cd4b3
Export open RocksDB snapshots as a gauge
karthikiyer56 0e3cb62
Pin the per-method tables together with a completeness test
karthikiyer56 b0d379c
getTransactions: never return a cursor below the request's cursor
karthikiyer56 b8d6a3a
Gate v2 getHealth on this run's first commit
karthikiyer56 99e5f04
Merge remote-tracking branch 'origin/feature/full-history' into karth…
karthikiyer56 37d8424
getLedgers: echo a caught-up cursor instead of rejecting it
karthikiyer56 661ed78
CHANGELOG: say rpcv1 and rpcv2, not full-history daemon
karthikiyer56 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
57 changes: 57 additions & 0 deletions
57
cmd/stellar-rpc/internal/rpcv2/methods_completeness_test.go
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| package rpcv2 | ||
|
|
||
| import ( | ||
| "reflect" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/stellar/stellar-rpc/cmd/stellar-rpc/internal/rpcv2/config" | ||
| ) | ||
|
|
||
| // configMethodNames maps each MethodsConfig method field's Go name to its wire | ||
| // name (the toml tag). The two methods-wide default-tier fields are pointers, | ||
| // not structs, so the kind check skips them. | ||
| func configMethodNames(t *testing.T) map[string]string { | ||
| t.Helper() | ||
| typ := reflect.TypeFor[config.MethodsConfig]() | ||
| names := map[string]string{} | ||
| for field := range typ.Fields() { | ||
| if field.Type.Kind() != reflect.Struct { | ||
| continue | ||
| } | ||
| wireName, _, _ := strings.Cut(field.Tag.Get("toml"), ",") | ||
| require.NotEmpty(t, wireName, "method field %s needs a toml tag", field.Name) | ||
| names[field.Name] = wireName | ||
| } | ||
| return names | ||
| } | ||
|
|
||
| func TestLimitsByMethod_CoversEveryConfiguredMethod(t *testing.T) { | ||
| limits := limitsByMethod(validCfg(1, 1, "genesis").Service.Methods) | ||
|
|
||
| wireNames := configMethodNames(t) | ||
| for _, name := range wireNames { | ||
| assert.Contains(t, limits, name) | ||
| } | ||
| assert.Len(t, limits, len(wireNames), | ||
| "limitsByMethod has a key with no MethodsConfig field behind it") | ||
| } | ||
|
|
||
| func TestValidateService_ChecksEveryConfiguredMethod(t *testing.T) { | ||
| for fieldName, wireName := range configMethodNames(t) { | ||
| t.Run(wireName, func(t *testing.T) { | ||
| cfg := validCfg(1, 1, "genesis") | ||
| methodField := reflect.ValueOf(&cfg.Service.Methods).Elem().FieldByName(fieldName) | ||
| zero := uint(0) | ||
| methodField.FieldByName("QueueLimit").Set(reflect.ValueOf(&zero)) | ||
|
|
||
| err := validateService(cfg.Service) | ||
| require.Error(t, err, | ||
| "validateService's method list is missing %s", wireName) | ||
| assert.Contains(t, err.Error(), wireName) | ||
| }) | ||
| } | ||
| } |
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While verifying the cursor guard here I checked how the other two paginated methods behave for the same caught-up poll. getEvents is fine: an empty page returns a max cursor at the last scanned ledger, which stays valid when resent at the tip. getLedgers still breaks the polling loop, and it fails with an error instead of an empty page:
cursor: "T".cursor: "T".parseCursorcomputesstart = T + 1and rejects it, becauseIsLedgerWithinRangerequiresstart <= T(get_ledgers.go, line 154).-32602: "cursor ('T') must be between the oldest ledger: X and the latest ledger: T". Every poll fails until the next ledger closes.This self-heals within one ledger interval, so nobody gets permanently stuck the way #745 clients did. But it is the steady state for anyone polling faster than the close interval, and the code is the same
-32602a malformed cursor gets, so a client cannot tell "wait and retry" apart from "my cursor is garbage" without parsing the message text. The handler already contains the intended behavior: the empty-result branch echoesrequest.Pagination.Cursor(line 106), but validation rejects the caught-up request before that branch can run, so it is unreachable for exactly the case it was written for.Since the changelog entry in this PR states the contract generally ("the returned cursor now always does what the docs promise"), I think we should bring getLedgers in line, either as one more commit here or as an immediate follow-up, ideally landing in the same release so the caught-up contract changes once for both methods. The shape I have in mind mirrors the getTransactions fix:
parseCursorkeeps only the lower-bound rejection (a cursor below the oldest ledger is real data loss and should stay an error), and the handler returns early for a cursor at or past the tip, beforeendis computed orfetchLedgersruns:Two details worth pinning in tests: an explicit
startLedgerabove the tip should stay an error, the same asymmetry getTransactions has (only the server-issued token gets the echo, an explicit out-of-range start is a client mistake), and the cursor"4294967295"makesstartwrap to 0 inparseCursor, which the retained lower-bound check needs to catch. Placing the early return beforeend := start + uint32(limit) - 1also keeps that addition from ever running with an above-tip start.If you'd rather defer this, a row on #937 plus scoping the changelog sentence to getTransactions would keep the docs honest in the meantime.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair callout — done in 37d8424, making getLedgers toe the same cursor-semantics line as getTransactions.