perf(rpc): share one batch pool across the v8/v9/v10 servers

[NazariiDenha]

User description

share one batch pool across the v8/v9/v10 servers

---

PR Type

Enhancement

---

Description

• Share one goroutine pool across RPC versions

• Add server constructor accepting explicit pool

• Update batch concurrency help text

---

File Walkthrough

	Relevant files
Documentation	juno.go Update batch concurrency usage text                                            cmd/juno/juno.go Updates rpcBatchConcurrencyUsage help text. States batch concurrency limit is shared across RPC versions. +2/-2
Enhancement	server.go Add JSONRPC server constructor with pool                                  jsonrpc/server.go Adds NewServerWithPool constructor. Initializes a JSON-RPC server with an external pool. Preserves existing NewServer behavior. +12/-0    node.go Create shared batch pool for RPC servers                                  node/node.go Creates one shared pool.Pool for batch requests. Passes the shared pool to v8, v9, and v10 RPC servers. Adds pool import from conc. +6/-3

---

[github-actions]

Claude finished @NazariiDenha's task in 2m 34s —— View job

---

Review: rpc: share one batch pool across v8/v9/v10 servers

• Gather context (diff, related code)
• Understand the request (code review)
• Review changed files for correctness/concurrency/perf issues
• Inline comments (none needed)
• Final summary

Summary: The change is correct and does what it says. node/node.go now creates a single pool.New().WithMaxGoroutines(maxGoroutines) and injects it into all three jsonrpc.Server instances (v8/v9/v10) via the new WithPool setter in jsonrpc/server.go:203-207, replacing the previous per-server pool. This correctly caps total batch concurrency across all RPC versions instead of allowing 3 × maxGoroutines concurrent goroutines, which is the intended perf fix. Since all three servers are also reused for the WS routes (same Server instances registered under both HTTP and /ws paths in node/node.go), the shared limit applies there too, consistently.

Concurrency safety: sync.WaitGroup-per-batch (jsonrpc/server.go:479,501-517) is used to track each batch's own tasks, while pool.Pool.Go() is only used for its concurrency-limiting semaphore — conc/pool.Pool is designed to be called concurrently from multiple goroutines, so sharing one pool instance across the three servers' concurrent batch requests is safe.

Backward compatibility: existing tests that call jsonrpc.NewServer(...) directly without WithPool are unaffected — NewServer still creates a default pool (jsonrpc/server.go:184).

No blockers or important issues found.

Nit: In node/node.go:547-576, jsonrpc.NewServer(maxGoroutines, logger) still allocates its own default pool.Pool internally (jsonrpc/server.go:184) before WithPool(batchPool) immediately discards it — harmless (no goroutines spawned by pool.New()), just a small redundant allocation done 3 times. Not worth blocking on.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 04f3242)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🏅 Score: 78

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Wasted Allocation jsonrpc.NewServer(maxGoroutines, logger) presumably still creates its own internal batch pool sized maxGoroutines before WithPool(batchPool) immediately replaces it. This happens for all three servers (v8/v9/v10), so three internal pools are allocated and discarded unused every time node.New runs. Consider adding a way to skip the default pool creation when a shared pool will be supplied. jsonrpcServerV10 := jsonrpc.NewServer(maxGoroutines, logger). WithPool(batchPool). WithValidator(rpcv10.Validator()). WithMaxBatchElements(int(cfg.RPCMaxBatchSize)). WithMaxBatchResponseBytes(maxBatchResponseBytes). DisableBatchRequests(cfg.ForbidRPCBatchRequests) methodsV10, pathV10 := rpcHandler.MethodsV0_10() if err = jsonrpcServerV10.RegisterMethods(methodsV10...); err != nil { return nil, err } jsonrpcServerV09 := jsonrpc.NewServer(maxGoroutines, logger). WithPool(batchPool). WithValidator(rpcv9.Validator()). WithMaxBatchElements(int(cfg.RPCMaxBatchSize)). WithMaxBatchResponseBytes(maxBatchResponseBytes). DisableBatchRequests(cfg.ForbidRPCBatchRequests) methodsV09, pathV09 := rpcHandler.MethodsV0_9() if err = jsonrpcServerV09.RegisterMethods(methodsV09...); err != nil { return nil, err } jsonrpcServerV08 := jsonrpc.NewServer(maxGoroutines, logger). WithPool(batchPool). WithValidator(rpcv8.Validator()). WithMaxBatchElements(int(cfg.RPCMaxBatchSize)). WithMaxBatchResponseBytes(maxBatchResponseBytes). DisableBatchRequests(cfg.ForbidRPCBatchRequests) Missing Nil/Doc Guidance WithPool has no guard or documentation about passing a nil pool, and no comment clarifying that the same pool instance must be safe to share across multiple Server instances (concurrent Submit/Go calls from different RPC versions). If a future caller passes nil or a pool not intended for sharing, batch processing could panic or behave unexpectedly with no indication from the API. // WithPool registers a pool func (s *Server) WithPool(p *pool.Pool) *Server { s.pool = p return s }

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.21%. Comparing base (cb4e100) to head (a43a4f8).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4013      +/-   ##
==========================================
- Coverage   79.32%   79.21%   -0.11%     
==========================================
  Files         465      465              
  Lines       35795    35802       +7     
==========================================
- Hits        28393    28361      -32     
- Misses       7393     7432      +39     
  Partials        9        9              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[rodrodros]

@NazariiDenha commit lint please

[github-actions]

Claude finished @NazariiDenha's task in 3m 36s —— View job

---

Review: rpc: share one batch pool across v8/v9/v10 servers

• Gather context (diff vs origin/main, related code)
• Understand the request (code review)
• Correctness / concurrency / lifecycle analysis
• Build & vet affected packages
• Inline comments (none needed)
• Final summary

Verdict: clean — no blockers or important issues.

The change is correct: node/node.go:545 now creates a single pool.New().WithMaxGoroutines(maxGoroutines) and injects it into the v8/v9/v10 jsonrpc.Server instances via the new WithPool setter (jsonrpc/server.go:203-206), replacing three independent per-server pools. Previously the effective global cap on concurrent batch-call goroutines was 3 × maxGoroutines; now it's maxGoroutines, matching the updated usage string in cmd/juno/juno.go:285-286.

Concurrency/lifecycle check: I specifically verified the shared-pool-lifecycle concern raised by the automated PR-Agent review (that one server's Stop could stop the pool while others are still using it). jsonrpc.Server has no Stop/Shutdown/Close method at all — batches are tracked with a per-call sync.WaitGroup (jsonrpc/server.go:479,501-519), and s.pool.Go() is used purely for its concurrency-limiting semaphore. conc/pool.Pool.Go() is documented safe for concurrent calls from multiple goroutines, and since pool.Wait() is never called on it, there's no risk of one server draining/blocking the others. So that concern doesn't apply here.

Backward compatibility: confirmed via jsonrpc/*_test.go — no existing test calls WithPool, and NewServer still builds its own default pool (jsonrpc/server.go:184), so all existing tests are unaffected.

Build/vet: go build ./node/... and go vet ./jsonrpc/... both pass. (go build ./cmd/juno/... fails to link in this sandbox only because the prebuilt Rust static libs — libjuno_starknet_compiler_rs, libjuno_starknet_rs — aren't present; this is a pre-existing environment limitation unrelated to this diff, and go test execution required approval I didn't have in this run.)

Nit (non-blocking): jsonrpc.NewServer(maxGoroutines, logger) still allocates its own default pool.Pool (jsonrpc/server.go:184) before .WithPool(batchPool) immediately discards it — done 3× in node/node.go. Harmless (pool.New() spawns nothing), just a small redundant allocation.

[rodrodros]

Open question. Should the default max go-routines also increase?

[NazariiDenha]

Open question. Should the default max go-routines also increase?

Probably, not. As I tested in benchmarks: pool begins to show maximum performance on relatively small concurrency values (4-8) even on machine with 16 cores. So even on machine with minimum requirements of 4 cores we will have 8 by default

[github-actions]

Persistent review updated to latest commit 04f3242

[infrmtcs]

I think this is minimal change, but I'm wondering if we should pass the pool here, because it's required here.

[NazariiDenha]

I decided to go with WithPool to be consistent with other fields in Server

[infrmtcs]

I think it's not really consistent.
Other With* functions are setting the optional fields, and they don't need to be set in order to work correctly.
For this one, a nil pool is not working, so we need to set a random number for poolMaxGoroutines in order to satisfy the condition that NewServer returns a working instance, then we set it using WithPool to override the newly created pool. This can cause confusion where reader may try to modify the passed poolMaxGoroutines before realizing that it doesn't change anything at all.

[NazariiDenha]

added NewServerWithPool(pool *pool.Pool, logger log.StructuredLogger) and removeed .WithPool

[github-actions]

Claude finished @NazariiDenha's task in 3m 26s —— View job

---

Review: share batch pool across v8/v9/v10 servers

• Gather context (diff vs origin/main, related code)
• Understand the request
• Review changed files for correctness/concurrency/perf issues
• Post inline comments if needed
• Final summary

Verdict: clean, no blockers or important issues.

Re-reviewed the current head (04f3242, unchanged since the last two automated passes). node/node.go:545 creates a single pool.New().WithMaxGoroutines(maxGoroutines) and injects it into the v8/v9/v10 jsonrpc.Servers via the new WithPool setter (jsonrpc/server.go:203-207), correctly capping total batch concurrency at maxGoroutines instead of 3 × maxGoroutines. The usage string in cmd/juno/juno.go:285-286 was updated to match.

Concurrency check confirmed safe: each batch tracks its own tasks with a local sync.WaitGroup (jsonrpc/server.go:479,501-519); s.pool.Go() is used only for its concurrency-limiting semaphore, and pool.Wait() is never called on the shared pool by any server, so there's no cross-server draining/blocking risk from sharing one *pool.Pool instance. conc/pool.Pool.Go() is safe to call concurrently from multiple goroutines.

Verified go build ./jsonrpc/... and go build ./node/... succeed, and go vet on both is clean. (go test execution wasn't permitted in this sandbox run.)

Nits (non-blocking, already raised in earlier review passes, no action needed):

• jsonrpc.NewServer(maxGoroutines, logger) still allocates a default pool.Pool (jsonrpc/server.go:184) that's immediately discarded by .WithPool(batchPool), done 3× in node/node.go. Harmless since pool.New() spawns no goroutines.
• WithPool (jsonrpc/server.go:203-207) has no doc note that the pool is expected to be shared/safe for concurrent use, and no nil-guard. Not a real risk given the only caller passes a valid pool.
• The WithPool vs. constructor-arg naming question was already discussed and resolved in the review thread — consistent with the existing builder pattern (WithValidator, WithListener, etc.) used elsewhere in jsonrpc/server.go.
  ·

[github-actions]

Standalone PR Review

PR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🏅 Score: 62

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Shared Pool Semantics A single conc/pool.Pool instance is now shared across the v8/v9/v10 servers via WithPool(batchPool). If the batch-handling code (not shown in this diff) does the typical pool.Go()-per-call followed by pool.Wait()-per-batch pattern, sharing one pool across concurrent requests/servers means Wait() for one batch will block until ALL outstanding tasks in the shared pool complete, including unrelated concurrent batches from other RPC versions or clients. This couples latency across independent requests and can cause head-of-line blocking (a slow batch on v8 delays completion signaling for a fast batch on v10). Depending on how the pool's underlying wait-group is implemented, concurrently calling Go() from one goroutine while another goroutine is inside Wait() can also risk sync: WaitGroup misuse panics if the internal counter transiently reaches zero. This is uncertain without seeing the call sites that use s.pool, but given the high potential impact (RPC panics or incorrect batch completion signaling), it should be verified that Wait() is not called per-batch on a pool shared across concurrent requests. batchPool := pool.New().WithMaxGoroutines(maxGoroutines) jsonrpcServerV10 := jsonrpc.NewServer(maxGoroutines, logger). WithPool(batchPool). WithValidator(rpcv10.Validator()). WithMaxBatchElements(int(cfg.RPCMaxBatchSize)). WithMaxBatchResponseBytes(maxBatchResponseBytes). DisableBatchRequests(cfg.ForbidRPCBatchRequests) methodsV10, pathV10 := rpcHandler.MethodsV0_10() if err = jsonrpcServerV10.RegisterMethods(methodsV10...); err != nil { return nil, err } jsonrpcServerV09 := jsonrpc.NewServer(maxGoroutines, logger). WithPool(batchPool). WithValidator(rpcv9.Validator()). WithMaxBatchElements(int(cfg.RPCMaxBatchSize)). WithMaxBatchResponseBytes(maxBatchResponseBytes). DisableBatchRequests(cfg.ForbidRPCBatchRequests) methodsV09, pathV09 := rpcHandler.MethodsV0_9() if err = jsonrpcServerV09.RegisterMethods(methodsV09...); err != nil { return nil, err } jsonrpcServerV08 := jsonrpc.NewServer(maxGoroutines, logger). WithPool(batchPool). Wasted Pool Creation Each jsonrpc.NewServer(maxGoroutines, logger) call likely still constructs an internal per-server pool sized maxGoroutines, which is then immediately discarded/overwritten by the following WithPool(batchPool) call. This isn't a correctness bug but is unnecessary allocation/goroutine-pool setup for objects that are never used. jsonrpcServerV10 := jsonrpc.NewServer(maxGoroutines, logger). WithPool(batchPool). WithValidator(rpcv10.Validator()).

[github-actions]

Claude finished @NazariiDenha's task in 9m 17s —— View job

---

Review: share batch pool across v8/v9/v10 servers

• Gather context (diff vs origin/main, related code)
• Review changed files for correctness/concurrency/perf issues
• Build & vet affected packages
• Post inline comments
• Final summary

Verdict: no blockers or important issues. One nit posted inline.

Re-reviewed the current head (a43a4f8), which now uses a dedicated NewServerWithPool(pool, logger) constructor instead of the earlier WithPool setter — this resolves the API-design discussion in the review thread (a shared pool is a required dependency, not an optional builder field, so baking it into the constructor avoids the wasted default-pool allocation that earlier passes flagged as a nit).

node/node.go:544 creates a single pool.New().WithMaxGoroutines(maxGoroutines) and passes it to all three jsonrpc.NewServerWithPool(...) calls (v8/v9/v10), correctly capping total batch concurrency at maxGoroutines instead of 3 × maxGoroutines. The usage string in cmd/juno/juno.go:285-286 was updated to match.

Concurrency check: confirmed safe. Each batch tracks its own tasks with a local sync.WaitGroup (jsonrpc/server.go:483,508-520); s.pool.Go() is used only for its concurrency-limiting semaphore, and pool.Wait() is never called on the shared pool by any server — so there's no cross-server draining/blocking risk, and no WaitGroup misuse from sharing one *pool.Pool across concurrent Go() calls (documented safe by conc/pool).

Backward compatibility: confirmed — no existing test calls NewServerWithPool; all existing call sites use NewServer, which is untouched.

Build/vet: go build ./jsonrpc/... ./node/... and go vet ./jsonrpc/... ./node/... both pass clean.

Nit (posted inline): the doc comment on the new NewServerWithPool (jsonrpc/server.go:191) still reads // NewServer instantiates a JSONRPC server with pool — leftover from the prior WithPool naming, should reference NewServerWithPool.

[github-actions]

Standalone PR Review

PR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🏅 Score: 85

🧪 No relevant tests

🔒 No security concerns identified

⚡ No major issues detected

[github-actions]

Doc comment still says "NewServer" — leftover from before the WithPool→NewServerWithPool rename. Should read // NewServerWithPool instantiates a JSONRPC server with a shared pool (staticcheck's ST1020 convention expects the comment to start with the function name).

Suggested change

	// NewServer instantiates a JSONRPC server with pool
	// NewServerWithPool instantiates a JSONRPC server with a shared pool