common, db, cl, execution, rpc, node: consolidate pool and retry utilities#22170
Open
yperbasis wants to merge 11 commits into
Open
common, db, cl, execution, rpc, node: consolidate pool and retry utilities#22170yperbasis wants to merge 11 commits into
yperbasis wants to merge 11 commits into
Conversation
Four byte-identical copies of the pooled 512KB bufio reader/writer helpers lived in db/etl, db/recsplit, db/seg and db/datastruct/btindex. Consolidate them into a new db/bufiopool package, keeping the buffer size and the Reset(nil)-before-Put semantics unchanged.
common/hasher.go and common/crypto/crypto.go each kept their own sync.Pool of fastkeccak states. Keep a single pool of raw KeccakState in the common package (common/crypto imports common, so the pool must live there) and delegate crypto's NewKeccakState/ReturnToPool to it. Get/Reset/Put semantics are unchanged: both implementations reset on Get and put back without reset. The pool now holds raw KeccakStates instead of Hasher wrappers, so common.NewHasher allocates a 16-byte wrapper per call; the hotter crypto path (Keccak256, Keccak256Hash, RlpHash) stays allocation-free.
Seven packages each declared their own sync.Pool of bytes.Buffer with the same get/reset/put pattern. Consolidate them into common/pool (GetBuffer resets on get, matching every existing call site) and add a 1MB capacity cap on put so one oversized use cannot pin memory now that unrelated subsystems share the pool. The sibling pools in cl/persistence/base_encoding (plain uint64/byte/ pattern slices) and the zstd encoder/decoder pools are left as-is.
Replace the hand-rolled recursive retry helper in retryConnects with cenkalti/backoff/v4 (constant 1s backoff, context-bound), preserving the existing semantics exactly: 500ms per-attempt timeout within a 20s overall budget, retry on dial errors and deadline expiries, immediate return on any other error, a per-attempt timeout following a dial error reports that dial error, and overall-deadline expiry reports the last dial error (nil when attempts only ever timed out). The new retry_test.go pins those semantics; it was written against the old implementation and passes unchanged against the new one.
yperbasis
requested review from
AskAlexSharov,
domiwei,
lupin012,
mh0lt and
sudeepdino008
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates several previously duplicated pooling and retry utilities into shared packages, reducing copy-paste drift risk while preserving existing behavior (including a characterized retry “nil-on-timeout” quirk).
Changes:
- Consolidates repeated
bufio.Reader/Writerpooling intodb/bufiopooland updates DB utilities to use it. - Consolidates repeated
*bytes.Bufferpooling intocommon/pool(with a 1MB cap on pooled buffers) and updates CL/DB/execution call sites. - Unifies
KeccakStatepooling intocommonand updatescommon/cryptoto delegate; replacesrpc/requestsretry logic withcenkalti/backoff/v4and adds characterization tests.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rpc/requests/retry_test.go | Adds characterization tests for retryConnects semantics. |
| rpc/requests/request_generator.go | Replaces custom retry loop with cenkalti/backoff/v4 while preserving semantics. |
| execution/types/receipt.go | Switches typed receipt encoding buffer reuse to common/pool. |
| execution/types/hashing.go | Removes the now-duplicated local buffer pool. |
| db/snapshotsync/freezeblocks/caplin_snapshots.go | Switches decompression staging buffer reuse to common/pool. |
| db/snapshotsync/freezeblocks/beacon_block_reader.go | Removes local buffer pool and uses common/pool. |
| db/seg/parallel_compress.go | Switches bufio pooling to db/bufiopool. |
| db/seg/compress.go | Removes local bufio pool and uses db/bufiopool. |
| db/recsplit/recsplit.go | Removes local bufio pool and uses db/bufiopool. |
| db/etl/dataprovider.go | Switches writer pooling to db/bufiopool. |
| db/datastruct/btindex/btree_index.go | Switches writer pooling to db/bufiopool. |
| db/datastruct/btindex/btree_index_test.go | Updates test helper to use db/bufiopool. |
| db/bufiopool/bufiopool.go | Introduces shared 512KB bufio reader/writer pooling. |
| common/pool/pool.go | Introduces shared *bytes.Buffer pooling with a max-cap drop policy. |
| common/hasher.go | Unifies pooled KeccakState ownership in common. |
| common/crypto/crypto.go | Delegates keccak pooling to common. |
| cl/persistence/state/historical_states_reader/historical_states_reader.go | Switches buffer pooling to common/pool. |
| cl/persistence/format/snapshot_format/blocks.go | Switches buffer pooling to common/pool. |
| cl/persistence/beacon_indicies/indicies.go | Switches buffer pooling to common/pool. |
| cl/persistence/base_encoding/uint64_diff.go | Switches buffer pooling to common/pool. |
| cl/antiquary/state_antiquary.go | Removes local buffer pool (now shared). |
| cl/antiquary/beacon_states_collector.go | Switches diff buffer pooling to common/pool. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Author
|
Filed the follow-up for the retry quirk preserved by the characterization tests here: #22182 (retryConnects reports success when every connect attempt timed out). |
… buffer copies The CL read paths (historical-states dumps/diffs, beacon-block decompression) only wrapped an in-memory slice in a pooled buffer to read it back; feed bytes.NewReader to the zstd reader instead, dropping both the copy and the pool round-trip. These blobs routinely exceed common/pool's 1MB Put cap on mainnet (a full balances dump is ~16MB raw), so pooling bought nothing there anyway. The antiquary's multi-MB diff writer now reuses the collector-owned buffer like the sibling helpers in the same file. Also remove the dead plainBytesBufferPool in base_encoding.
Two error-swallowing fixes, each pinned by a test written red-first: - Overall-deadline expiry after only per-attempt timeouts returned nil, so callers saw success with an unpopulated result. Return context.DeadlineExceeded instead. - The final deadline-to-dial-error remap matched any error wrapping DeadlineExceeded, including a permanent one (e.g. a non-dial net.OpError joined with the deadline sentinel), replacing it with the stale dial error or nil. Require the overall context to have actually expired.
node/rpcstack's gzBufPool duplicated common/pool exactly, down to the 1MB drop cap. Its cap-threshold test moves to common/pool as unit tests of the shared pool's reset-on-get and drop-oversized semantics. gzDstPool stays: it pools []byte for libdeflate, not bytes.Buffer.
yperbasis
changed the title
pull Bot
pushed a commit
to Dustin4444/erigon
that referenced
this pull request
Jul 3, 2026
…erigontech#22171) `cmd/downloader`, `txnprovider/txpool`, `node/privateapi`, and `p2p/sentry` each re-inlined the interceptor/keepalive option block that `grpcutil.NewServer` already builds — two of them complete with its dead commented-out metrics hooks — and all four repeated the same listen + health-server + serve-goroutine tail. `NewServer(rateLimit, creds)` now delegates to a new `NewServerWithOpts(creds, extraOpts...)` (nil-safe creds, no forced `MaxConcurrentStreams`), and `StartServer`/`StartServerOnListener` own the listen/health/serve tail. The four sites convert onto these. Option preservation was the review focus, per site: downloader and txpool still run **without** `MaxConcurrentStreams` (as before); txpool keeps its explicit `ReadBufferSize(0)`/`WriteBufferSize(0)`; privateapi/sentry keep health registration gated on their `healthCheck` flag while downloader/txpool keep it unconditional; existing `NewServer` callers see byte-identical behavior. Two deliberate exceptions to strict preservation: - Both converted `StartGrpc`s now take `credentials.TransportCredentials` by value (as `privateapi.StartGrpc` already does) instead of pointer-to-interface, dropping the identical unwrap shim at each site; their sole callers pass literal `nil` and are unchanged. - The txpool serve-failure log now reads `txpool gRPC server fail` instead of the `private RPC server fail` string copy-pasted from privateapi — in the standalone txpool daemon, `--private.api.addr` names the remote node it connects to, so the old label pointed triage at the wrong process. Nothing greps or asserts these strings. Otherwise no behavior change — existing tests are the safety net (txpool, privateapi, sentry suites green). Full `make lint` clean, `make erigon integration` build. Net −117 lines. Part of a dedup series; siblings erigontech#22165–erigontech#22170.
This unit test exercises gzipResponseWriter.Flush(); the buffer's origin is irrelevant to it, so borrowing from the shared common/pool (without returning it) added coupling for no benefit. Use a plain bytes.Buffer.
…wrapped error On overall-deadline expiry retryConnects reports the last dial error that backoff.Retry discarded. The guard used errors.Is(err, DeadlineExceeded), which also matches a permanent (non-connection) error that merely wraps DeadlineExceeded — so once a dial error had been seen and the overall deadline had expired, such a permanent error was swallowed and the older dial error returned instead. backoff.Retry surfaces the context's error verbatim (the bare sentinel) only when it gives up on the overall deadline; a permanent error comes back as its own wrapped value. Compare by identity to tell the two apart, which also makes the now-redundant ctx.Err() conjunct unnecessary. Pinned by a red-first test.
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.
Four independent consolidations of copy-pasted utility code, plus review follow-ups:
db/bufiopool.getBufioWriter/putBufioWriter(+reader twins in two of them) were byte-identical — including comments — indb/etl,db/recsplit,db/seg, anddb/datastruct/btindex. One package now owns them; 512KB size and Reset(nil)-before-Put semantics preserved verbatim.common/hasher.goandcommon/crypto/crypto.goeach pooledkeccak.NewFastKeccak(). The single pool of rawKeccakStatelives incommon(import direction forces it);crypto.NewKeccakState/ReturnToPooldelegate. Reset-on-Get semantics identical on both old paths; no exported signature changed. The no-longer-pooledHasherwrapper is free in practice:NewHashernow inlines (the old pooled version exceeded the inlining budget) and&Hasher{}does not escape at any call site, so no per-call heap allocation appeared.common/pool. Eight privatesync.Pool{New: …&bytes.Buffer{}}copies across cl/persistence, cl/antiquary, db/snapshotsync, execution/types, and node/rpcstack (gzip responses) replaced byGetBuffer/PutBuffer; Put drops buffers over 1MB so one oversized use can't pin memory (the rpcstack copy already used exactly this cap; the others had none). Call sites that routinely exceed 1MB on mainnet no longer use a pool at all: the CL read paths (historical-states dumps/diffs, beacon-block decompression) only wrapped an in-memory slice to read it back and now feedbytes.NewReaderstraight to the zstd reader — zero-copy, cheaper than the old uncapped pools, which copied the whole blob per call — and the antiquary's multi-MB diff writer reuses the collector-owned buffer like its sibling helpers. The pool's remaining users are typically-sub-1MB write targets. Reset-on-get and drop-oversized semantics are unit-tested; a deadplainBytesBufferPoolin base_encoding removed along the way.context.DeadlineExceededinstead of nil (callers used to see success with an unpopulated result); and on overall-deadline expiry the last dial error is substituted only for the context's own deadline error, never for a permanent error that merely wrapsDeadlineExceeded. Becausebackoff.Retrysurfaces the overall context's error verbatim when it gives up, the substitution keys offerr == context.DeadlineExceeded(identity) rather thanerrors.Is, so abackoff.Permanenterror that wrapsDeadlineExceededis left intact.Net −146 production lines plus +180 of tests. Re-verified after merging latest
main:go teston every touched tree,make lintclean,go build ./...,make erigon integration.Part of a dedup series; siblings #22165–#22169.