Repository navigation
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
8715914 to
b6d65ee
Compare
670361a to
0bfa01d
Compare
0bfa01d to
6df8786
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Commit failures can leave the ledger cache inconsistent, and the dependency switch breaks the documented Handler.Close behavior.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Switches the JSON-RPC server to a fork that bypasses the in-memory client round trip, reducing response serialization overhead.
Changes:
- Migrates server-side JSON-RPC usage to
stellar-experimental/jrpc2. - Disables jrpc2’s semaphore in favor of existing request limiters.
- Updates SQLite oldest-ledger cache publication during trimming.
| File | Description |
|---|---|
go.mod |
Updates JSON-RPC dependencies. |
go.sum |
Records dependency checksums. |
cmd/stellar-rpc/internal/jsonrpc/jsonrpc.go |
Uses the forked HTTP bridge and disables its concurrency limit. |
cmd/stellar-rpc/internal/jsonrpc/specs.go |
Migrates handler types to the fork. |
cmd/stellar-rpc/internal/jsonrpc/specs_test.go |
Updates specification tests. |
cmd/stellar-rpc/internal/network/backlogQ.go |
Migrates queue limiter types. |
cmd/stellar-rpc/internal/network/backlogQ_test.go |
Updates queue limiter tests. |
cmd/stellar-rpc/internal/network/requestdurationlimiter.go |
Migrates duration limiter types. |
cmd/stellar-rpc/internal/network/requestdurationlimiter_test.go |
Updates duration limiter tests. |
cmd/stellar-rpc/internal/methods/differential_test.go |
Updates differential test handlers. |
cmd/stellar-rpc/internal/methods/get_events.go |
Migrates the events handler. |
cmd/stellar-rpc/internal/methods/get_events_differential_test.go |
Updates events differential tests. |
cmd/stellar-rpc/internal/methods/get_fee_stats.go |
Migrates the fee-statistics handler. |
cmd/stellar-rpc/internal/methods/get_health.go |
Migrates the health handler. |
cmd/stellar-rpc/internal/methods/get_latest_ledger.go |
Migrates the latest-ledger handler. |
cmd/stellar-rpc/internal/methods/get_latest_ledger_test.go |
Updates latest-ledger tests. |
cmd/stellar-rpc/internal/methods/get_ledger_entries.go |
Migrates the ledger-entry handler. |
cmd/stellar-rpc/internal/methods/get_ledgers.go |
Migrates the ledger-range handler. |
cmd/stellar-rpc/internal/methods/get_network.go |
Migrates the network handler. |
cmd/stellar-rpc/internal/methods/get_transaction.go |
Migrates the transaction handler. |
cmd/stellar-rpc/internal/methods/get_transactions.go |
Migrates the transaction-list handler. |
cmd/stellar-rpc/internal/methods/get_transactions_test.go |
Updates transaction-list tests. |
cmd/stellar-rpc/internal/methods/get_transactions_differential_test.go |
Updates transaction differential tests. |
cmd/stellar-rpc/internal/methods/get_version_info.go |
Migrates the version handler. |
cmd/stellar-rpc/internal/methods/handler.go |
Uses the forked handler adapter. |
cmd/stellar-rpc/internal/methods/handler_test.go |
Updates handler-adapter tests. |
cmd/stellar-rpc/internal/methods/send_transaction.go |
Migrates transaction submission. |
cmd/stellar-rpc/internal/methods/simulate_transaction.go |
Migrates transaction simulation. |
cmd/stellar-rpc/internal/methods/simulate_transaction_test.go |
Updates simulation tests. |
cmd/stellar-rpc/internal/rpcv1/sqlitedb/db.go |
Publishes oldest-ledger cache values during commits. |
cmd/stellar-rpc/internal/rpcv1/sqlitedb/ledger.go |
Extracts oldest-ledger lookup logic. |
cmd/stellar-rpc/internal/rpcv1/sqlitedb/ledger_test.go |
Tests trim-time cache publication. |
cmd/stellar-rpc/internal/rpcv2/eventsapi/get_events_v1.go |
Migrates the v1 events adapter. |
cmd/stellar-rpc/internal/rpcv2/eventsapi/query_events.go |
Migrates event queries. |
cmd/stellar-rpc/internal/rpcv2/eventsapi/query_events_test.go |
Updates event-query tests. |
cmd/stellar-rpc/internal/rpcv2/eventsapi/v1_parity_test.go |
Updates parity tests. |
cmd/stellar-rpc/internal/rpcv2/jsonrpc.go |
Migrates RPC v2 handler types. |
cmd/stellar-rpc/internal/rpcv2/jsonrpc_test.go |
Updates RPC v2 tests. |
cmd/stellar-rpc/internal/rpcv2/ledger_reads_bench_test.go |
Updates ledger benchmarks. |
cmd/stellar-rpc/internal/rpcv2/rpcv2test/rpcv2test.go |
Migrates RPC v2 test infrastructure. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if err := w.tx.Commit(); err != nil { | ||
| return err | ||
| } |
| "github.com/stellar-experimental/jrpc2" | ||
| "github.com/stellar-experimental/jrpc2/handler" | ||
| "github.com/stellar-experimental/jrpc2/jhttp" |
6df8786 to
4490bc6
Compare
4490bc6 to
731856f
Compare
… POST state Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>


What
Switches stellar-rpc from
github.com/creachadair/jrpc2to thegithub.com/stellar-experimental/jrpc2fork, pinned at commit5b4c6315, the head of itsraw-message-passthroughbranch (stellar-experimental/jrpc2#1), which is based on upstream's v1.3.5 release. The fork'sjhttp.Bridgedispatches HTTP requests straight to the handlers and streams their results to the socket instead of routing every call through an in-memoryserver.Localclient/server pair. It also passes a non-emptyjson.RawMessagehandler result through verbatim, which #1060 relies on for its memoizedgetLatestLedgerbody.Changes in this repo:
go.modreplacescreachadair/jrpc2v1.3.3 with the fork. Between v1.3.3 and v1.3.5, upstream changed behavior only in anomitzerotag onrpc.serverInfo, which stellar-rpc disables. The rest isWaitGroup.Goandreflect.Pointermodernization.go-stellar-sdkmoves to3e6c13bb, the head of stellar/go-stellar-sdk#6020, which movesclients/rpcclientto the fork as well.creachadair/jrpc2leaves the module graph, so the integration tests'errors.As(err, &*jrpc2.Error)assertions match the type the server returns. The SDK move also brings in ingest: resume a failed bucket download and keep the hash check over every returned record go-stellar-sdk#6017 (bucket download resume) and dropshashicorp/golang-lru.jsonrpc.NewHandlersetsServerOptions.Concurrency: math.MaxInt, so jrpc2's handler semaphore (defaultruntime.NumCPU()) never blocks. The per-method backlog and duration limiters still wrap every handler.jsonrpc.NewHandlerstill callsjhttp.NewBridge.Handler.Close's doc no longer promises that the handler stops accepting requests.What the fork changes for this server (full list in the fork PR):
max-request-execution-durationfires. The old bridge ran them on the bridge server's background context, and therpc.cancelpath was unreachable withDisableBuiltin: true.Response.WriteTo.Content-Lengthis still set, from a dry run into a counting writer. Per-request allocation in the bridge no longer scales with result size. stellar-rpc's ownMakeHTTPRequestDurationLimiterstill buffers the whole body once.jhttptests: statically invalid batch entries are answered first, duplicate ids are each answered, empty batches and notification-only bodies return 204.jhttp.Bridge.Closenow only closes the GETGetter. stellar-rpc does not configure one, so it does nothing here.Handler.Closestill calls it.bytes_readandbytes_writtenexpvars no longer count bridge traffic. stellar-rpc does not read them.json_reqlog field now carries the caller's JSON-RPC id. The old bridge's client renumbered requests, so it used to log an internal counter.Why
Every response used to be marshaled in
invoke, encoded onto an in-memory pipe, parsed twice by the bridge's client, and compacted again byjson.Marshal(rsp)before reaching the socket. For the 2.8 MB pubnetgetLatestLedgerresult that is about 35 ms and 33 MB of allocation per call throughjsonrpc.NewHandler, with the handler itself under 6% of it.getLedgerspages scale the same way (limit 20, 56 MB: ~0.7 s). The bridge, not the handler, bounded large-response throughput.The concurrency cap mattered on the perf-eval box as well. The semaphore is acquired in
invokebefore the wrapped handler, so while the ingestion commit stall (#1036) parkedNumCPUhandlers on the cache lock, every other request queued behind them regardless of method.Measured in the fork, one call returning a 3 MB result over loopback HTTP, at the client:
An earlier draft (#1003) got the same result with an in-tree dispatcher that replaced
jhttp.Bridge. Doing it in the library keeps this repo's transport code unchanged.Known limitations
json.RawMessageis trusted: the bytes are not validated or HTML-escaped. Only [DRAFT] Optimize query-path caching/memoization #1060 returns one.Handler.Closeno longer stops the handler from accepting requests. Shutdown is covered byhttp.Server.Shutdown, which runs first in both daemons.