fix(svm): apply node quorum to SVM getTransaction event fetches - #1522
fix(svm): apply node quorum to SVM getTransaction event fetches#1522droplet-rl wants to merge 5 commits into
Conversation
QuorumFallbackSolanaRpcFactory._getQuorum only quorumed getBlock/getBlockTime, so getSignaturesForAddress and getTransaction — the sole source of all Solana SpokePool events — resolved at quorum 1. A single compromised/malicious RPC provider in a nodeQuorumThreshold>1 set could therefore forge or hide deposits/fills and drive bundle data (mirrors the eth_getLogs quorum treatment in RetryProvider on the EVM side). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e99ca6b3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // (FundsDeposited, FilledRelay, RequestedSlowFill, ...). They are historical/deterministic, so | ||
| // they must be quorumed — mirroring the eth_getLogs treatment in RetryProvider — otherwise a | ||
| // single compromised RPC provider could forge or hide deposits/fills and drive bundle data. | ||
| case "getSignaturesForAddress": |
There was a problem hiding this comment.
When providers are at different confirmed or finalized tips, getSignaturesForAddress legitimately returns different leading signatures, but compareSvmRpcResults requires the entire responses to be deeply equal. The event client does not pin the first request to toSlot; it fetches the latest page and only filters by slot afterward in eventsClient.ts:145-168, so even a historical update can now fail quorum and block all SVM event ingestion. Quorum a stable range or reconcile signatures by identity instead of comparing this unbounded page exactly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and this was the most important one — fixed by dropping getSignaturesForAddress from quorum entirely.
You are right that queryAllEvents never pins the first request: it fetches the newest page at limit: 1000 and filters by slot only afterwards, so the page tracks the confirmed tip. Any new signature landing between two providers' responses shifts the whole window, so deep equality would have failed routinely and stalled all SVM event ingestion.
The precedent this PR claimed to mirror actually argues your way: RetryProvider._getQuorum deliberately excludes latest/pending from eth_getBlockByNumber quorum for exactly this reason. eth_getLogs is quorum-safe because its range is explicitly bounded; the unpinned signature page is not.
I considered your "quorum a stable range" suggestion — restricting the comparison to the slot range both responses cover — but truncating to the common leading edge is gameable in the hide direction: a provider that withholds recent signatures shrinks the compared window and trivially passes. That is the exact attack this PR set out to stop, so a weak comparator there is worse than none.
Net effect: the forgery vector is closed (fabricated event payloads cannot survive getTransaction quorum), the omission vector on the signature page is not. I called that out explicitly in the code comment and in the PR description rather than papering over it; it needs a range-reconciling comparator, which is its own change.
| // they must be quorumed — mirroring the eth_getLogs treatment in RetryProvider — otherwise a | ||
| // single compromised RPC provider could forge or hide deposits/fills and drive bundle data. | ||
| case "getSignaturesForAddress": | ||
| case "getTransaction": |
There was a problem hiding this comment.
With heterogeneous RPC node versions, two valid getTransaction responses can differ only in optional or unavailable metadata such as meta.computeUnitsConsumed, stackHeight, or blockTime, while containing the same transaction and event instructions. This path compares the complete response using raw isEqual, so a two-provider deployment—or fallbacks that all serialize different optional fields—now throws a quorum error and prevents otherwise valid events from being processed. Compare stable transaction/event fields or normalize optional metadata before applying quorum.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — compareSvmRpcResults ignored its method argument entirely (_method) and fell through to a raw isEqual, while the EVM compareRpcResults has had an IGNORED_FIELDS normalisation layer for years. Enabling quorum on a payload as rich as getTransaction without that layer was the gap.
Fixed: compareSvmRpcResults is now method-aware, with SVM_IGNORED_FIELDS.getTransaction = ["computeUnitsConsumed", "stackHeight", "logMessages", "rewards"]. Because the volatile fields are nested (meta.computeUnitsConsumed, meta.innerInstructions[].instructions[].stackHeight) the existing shallow deleteIgnoredKeys helper was not usable, so there is a deleteIgnoredKeysDeep alongside it. I added logMessages to your list — it is truncated at a node-configurable byte limit, which is another mixed-version divergence.
One partial disagreement: I did not ignore blockTime. arch/svm/eventsClient.ts:300 reads it as depositTimestamp, so normalising it away would let a provider skew deposit timestamps with no cross-check. It is ledger-derived and stable once the slot is confirmed, so keeping it should not cause the flakiness you are worried about. There is a test asserting a blockTime divergence still fails quorum.
I verified each ignored field is unread by the SDK before dropping it: logMessages and stackHeight have no consumers, rewards is EVM-only in gasPriceOracle, and the computeUnitsConsumed hit in svmQuery.ts comes from estimateComputeUnits (simulation), not this path. The fields an attacker would have to alter — meta.err, meta.innerInstructions, meta.loadedAddresses, transaction.message.accountKeys — are all still compared.
| // single compromised RPC provider could forge or hide deposits/fills and drive bundle data. | ||
| case "getSignaturesForAddress": | ||
| case "getTransaction": | ||
| return this.nodeQuorumThreshold; |
There was a problem hiding this comment.
When nodeQuorumThreshold exceeds rpcFactories.length, the constructor accepts the configuration and requiredFactories contains only the available endpoints; the all-equal early return at line 124 then returns without checking that the threshold was actually met. For example, with threshold 2 and one SVM RPC URL, both newly quorum-enabled event methods still trust that single provider, silently defeating the security purpose of this change. Reject impossible thresholds during construction or require values.length >= quorumThreshold before the early return.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed — this was a real pre-existing hole that my change would have made actively misleading.
Traced it through: with threshold 2 and one RPC URL, requiredFactories = rpcFactories.slice(0, 2) yields a single factory, values.length === 1, and values.slice(1).every(...) is vacuously true on an empty array, so the early return hands back that one provider's answer having checked nothing. The operator sets nodeQuorumThreshold: 2, sees no error, and gets quorum 1.
RetryProvider already had the guard (retryProvider.ts:68) and the SVM factory simply never copied it, so I mirrored it verbatim rather than inventing a second convention. I also took your second suggestion as belt-and-braces, since the early return is the path that fails silently:
if (allValuesAgree && values.length >= quorumThreshold && !missingNeedsCorroboration) {Redundant given the constructor check, but it is the guard that keeps a future refactor of the slice from silently reintroducing this.
| // they must be quorumed — mirroring the eth_getLogs treatment in RetryProvider — otherwise a | ||
| // single compromised RPC provider could forge or hide deposits/fills and drive bundle data. | ||
| case "getSignaturesForAddress": | ||
| case "getTransaction": |
There was a problem hiding this comment.
When an RPC lacks historical transaction data or has not reached the requested commitment, getTransaction can fulfill successfully with null rather than rejecting. The quorum grouper counts that absence like any other result, so with threshold 2 two lagging or non-archival endpoints can outvote an archival endpoint returning the real transaction; eventsClient.ts:208-209 then treats the returned null as having no events and silently drops the associated deposit or fill. Require quorum among non-null transaction payloads and fail or retry when that quorum cannot be reached.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — without this, quorum on getTransaction only protected the forge direction and left the hide direction wide open, which undercuts half the point of the PR.
Implemented as "absence is provider-local": a null result means this provider does not have the transaction (pruned ledger, lagging commitment), which is not the claim that the transaction does not exist. Since processEventFromTx returns [] for a null result, letting absence win a vote silently drops a deposit or fill.
Two changes:
- Unanimous
nullamong the required providers no longer short-circuits while fallbacks remain. Previously two pruned nodes hit the all-equal early return and the archival provider was never consulted at all — that was the silent path. - In the tally, only providers that actually returned the transaction get a vote. If nobody has it, the absence is genuine and
nullis still returned; if someone has it but the result cannot reach the threshold, it throws instead of resolving tonull.
That last distinction matters for your "fail or retry" point: I kept genuine unanimous absence returning null rather than throwing, so an old backfill past every provider's archive horizon does not become a hard failure. Only contested absence fails loudly. All three SVM getTransaction callers are event-reading paths in eventsClient where the signature is already known to exist, so nothing legitimately polls for a maybe-nonexistent transaction here.
Two tests cover it: contested absence throws (and asserts the archival fallback was actually consulted), unanimous absence returns null.
Addresses the Codex review on #1522. - Reject nodeQuorumThreshold > provider count in the constructor. `requiredFactories` is a slice, so an impossible threshold silently degraded to the providers that exist and the all-agree early return was then trivially satisfied by a single provider -- reporting quorum while providing none. Mirrors RetryProvider. - Drop getSignaturesForAddress from quorum. eventsClient fetches the newest page and filters by slot afterwards rather than pinning the request, so the page tracks the confirmed tip and two honest providers a slot apart return different leading signatures. Deep equality would have stalled all SVM event ingestion. Mirrors RetryProvider excluding "latest"/"pending" from eth_getBlockByNumber. - Make compareSvmRpcResults method-aware and normalise getTransaction fields that are optional/node-version-gated (computeUnitsConsumed, stackHeight, logMessages, rewards). blockTime is deliberately kept -- eventsClient reads it as depositTimestamp. - Do not let a missing transaction win quorum. A null getTransaction means the queried provider lacks it, not that it does not exist; eventsClient decodes null as "no events", so pruned/lagging nodes could silently erase a real deposit or fill. Absence now only resolves once every consulted provider agrees. - Move the explanatory comment out from between case labels; it tripped eslint no-fallthrough and broke lint on the previous commit. Tests cover each case. Lint, prettier, tsc and the SVM suites pass.
droplet-rl
changed the title
|
@codex — all four findings were valid and are addressed in The important one was P1 #1. Quorumming I looked at your "quorum a stable range" suggestion and did not take it: truncating to the common leading edge is gameable in the hide direction, since a provider that withholds recent signatures shrinks the compared window and trivially passes. That is the attack this PR set out to stop, so a weak comparator is worse than none. The other three are fixed as suggested:
Two places I partly pushed back, both in the interest of not weakening the check:
Residual gap, stated plainly: the forgery vector is closed, the omission vector is not. A malicious provider serving the signature page can still withhold signatures, and One thing your review missed, found while verifying: the reviewed commit Also worth flagging for whoever reviews: this multiplies Verification: 16 tests in |
Summary
Testing
|
Leaving getSignaturesForAddress at quorum 1 is the central design decision of this
PR, but it was defended only by a comment. This asserts the behaviour: two providers
one slot apart return different leading signature pages, and the call still succeeds
off the first provider without consulting the second.
Verified the guard actually bites -- re-adding `case "getSignaturesForAddress"` to
_getQuorum makes it fail with the exact production symptom ("Not enough providers
agreed to meet quorum").
|
@codex heads up — The PR head is The permalinks in your summary are pinned to One thing your version had that mine didn't, so I took it: a test covering the moving signature page. Your six tests and mine overlapped on five; where you had that one, I had a forged-instruction-data test. The carve-out is the most consequential decision in this PR and it was defended only by a code comment, so I've added it in I checked the guard actually bites rather than passing vacuously: re-adding State now: 17 tests passing, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4443791237
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (absenceIsProviderLocal && votingValues.length === 0) { | ||
| // Nobody we consulted has it, so the absence is a property of the chain rather than of one provider. | ||
| return allValues[0][1]; |
There was a problem hiding this comment.
When the quorum providers return null but an additional fallback rejects—for example, two pruned nodes plus a temporarily unavailable archival node—the rejection is filtered out of fallbackValues, leaving votingValues empty and causing this branch to return null. The unavailable provider did not agree that the transaction is absent, so event ingestion can silently discard a real deposit or fill instead of retrying or surfacing the incomplete quorum; only return absence when every consulted fallback fulfilled with a missing result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — valid, and it reintroduced exactly the silent drop the absence handling was added to prevent. Fixed in 4752c28.
You traced it correctly: fallbackResults.filter(isPromiseFulfilled) discards the rejection, so two pruned providers plus an unreachable archival node left votingValues empty and fell into the unanimous-absence return. The archival node never agreed the transaction was missing — it just failed to answer — so treating that as consensus is wrong.
Now gated on every consulted fallback having actually answered:
const everyFallbackAnswered = fallbackResults.every(isPromiseFulfilled);
if (absenceIsProviderLocal && votingValues.length === 0 && everyFallbackAnswered) {With a rejection present, votingValues is empty, getHighestCountResult yields a count of 0, and it throws — so the caller retries instead of decoding null as "no events".
I did consider whether this is too strict, since a single flaky fallback now turns a genuinely-absent transaction into a hard failure rather than a null. I kept the strict behaviour: for a fund-safety path a loud failure that retries is clearly better than a silently dropped fill, and genuine absence still returns null whenever every provider actually responds. That trade-off is noted in the code comment.
Test added, and I confirmed it fails without the guard rather than passing vacuously.
| return isEqual(rpcResultA, rpcResultB); | ||
| } | ||
|
|
||
| return isEqual(deleteIgnoredKeysDeep(ignoredKeys, rpcResultA), deleteIgnoredKeysDeep(ignoredKeys, rpcResultB)); |
There was a problem hiding this comment.
When Redis caching is enabled and the provider-specific getTransaction entries were populated by different requests—such as concurrent event queries or separate processes—CachedSolanaRpcFactory stores each entire raw JSON-RPC envelope, including the per-request id, although that ID is not part of the cache key. Identical transaction payloads can therefore be loaded with different stale IDs, and this deep equality treats them as a provider disagreement, causing every subsequent quorum call for that cached transaction to fail; compare the result payloads or discard the top-level envelope ID before comparison.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and more reachable than the concurrency scenario you described. Fixed in 4752c28.
I verified the response shape rather than assuming it, since the fix depends on it:
@solana/rpc-transport-httpends inreturn await response.json(), so the transport hands the quorum layer the raw JSON-RPC envelope. The transformer chain that extracts.resultruns later, at the RPC-client layer — which is also whycachedRpcFactorycan callgetThrowSolanaErrorResponseTransformer()on the transport result at all.createRpcMessagein@solana/rpc-spec-typessetsid: getNextMessageId(), backed by a module-level_nextMessageId = 0n. Per-process, so ids diverge across restarts and across processes sharing a Redis namespace.CachedSolanaRpcFactorystores the whole envelope atNumber.POSITIVE_INFINITYTTL under a key that excludes the id.
The part that makes this near-certain rather than a corner case: the cache only writes once a transaction is finalized, and providers reach finality at slightly different moments. So the ordinary sequence is one provider serving a cached envelope while another fetches fresh, ids differing on that alone — and with no TTL, the disagreement is permanent for that transaction. Within a single call ids do match, since the same payload object is passed to every provider, so this only ever surfaces through the cache.
I did not take the "compare the result payloads" option, deliberately. Error envelopes resolve rather than reject at this layer, so reducing to result would give two different errors an identical absent result and collapse a real disagreement into false agreement. Stripping just the id is strictly narrower:
if (!("id" in envelope) || !("jsonrpc" in envelope)) return response;Top-level only — it never recurses, because a result payload may legitimately carry its own id field — gated on jsonrpc sitting alongside, with everything else including error preserved. I added a test pinning that down: two envelopes differing only in id now reach quorum, while two differing in error still report a mismatch.
Four tests added across both findings in this batch (20 total). I confirmed both new guards fail without their fix rather than passing vacuously. eslint, prettier --check and tsc --project tsconfig.build.json clean.
One observation worth carrying forward: both findings in this batch came from the quorum/cache interaction, not from quorum logic alone. getTransaction is the only cached SVM method, so quorumming it is the first thing that ever compares two cached envelopes — that combination is where the remaining risk in this PR sits.
Addresses the second Codex review batch on #1522. Both findings were reachable. - Drop the top-level JSON-RPC `id` before comparing SVM results. The transports resolve to the raw envelope (@solana/rpc-transport-http returns response.json()), and `id` is a per-process counter echoed by the provider. CachedSolanaRpcFactory caches whole envelopes with no TTL under a key that excludes the id, and only once a transaction is finalized -- so two providers finalizing at different moments cache different ids for the identical transaction and would disagree forever. Stripping is top-level only and gated on `jsonrpc` being present, so a result payload carrying its own `id` is untouched; `error` is deliberately preserved so differing error responses still count as a mismatch. - Only accept unanimous absence when every consulted fallback actually answered. A rejected fallback -- the archival node holding the transaction being briefly unreachable -- never agreed the transaction was missing, so returning null there reintroduced the silent event drop this handling exists to prevent. Four tests added, including one guarding against over-normalising the envelope. Verified both guards fail without their fix.
|
@codex both findings in review 4942269932 were valid and reachable — fixed in Envelope
That makes this worse than the concurrency scenario you described. The cache only writes once a transaction is I did not take the "compare the Absence with a failed fallback. Correct, and it reintroduced the silent drop the handling exists to prevent — the rejection was filtered out of Four tests added (20 total). I checked both new guards fail without their fix rather than passing vacuously. Worth flagging that both of these came from the quorum/cache interaction rather than from quorum logic alone — quorum on |
Summary
Testing
|
An `id` occurring inside a result payload is data, not the provider-local request bookkeeping the envelope id is, so stripJsonRpcEnvelopeId deliberately does not recurse. That was documented but untested: swapping it for a deep strip over ["id"] let two disagreeing payloads reach quorum, which this now catches.
|
The PR head is Your 21st test was the one thing I was missing, so I took it — added as State: 21 tests passing, Not tagging you for a re-review, since this turn added only a test and nothing about the diff's behaviour changed. Two things remain genuinely open and are better targets than another pass over what has already landed: the omission vector on the unquorumed signature page, and the quorum/cache interaction more broadly — |
|
Ask again to re-run the unfinished work. |
1 participant
QuorumFallbackSolanaRpcFactory._getQuorumonly appliednodeQuorumThresholdtogetBlock/getBlockTime.getTransaction— which returns the instruction data every Solana SpokePool event (FundsDeposited,FilledRelay,RequestedSlowFill,ExecutedRelayerRefundRoot) is decoded from — resolved at quorum 1, so an operator runningnodeQuorumThreshold > 1still trusted a single provider for event payloads. A compromised provider in that set could return a synthetic payload (fabricating a fill → fraudulent relayer-refund leaf) with no cross-check. This mirrors theeth_getLogsquorum treatment inRetryProvider._getQuorum.Making that safe needed three supporting fixes, all raised in review:
requiredFactoriesis aslice, sonodeQuorumThreshold: 2with one RPC URL silently degraded to one provider, and the all-agree early return is vacuously true on a single value — reporting quorum while providing none.RetryProvideralready had this guard; the SVM factory never copied it.compareSvmRpcResultsignored itsmethodargument and did a rawisEqual, unlike the EVMcompareRpcResults/IGNORED_FIELDSlayer. Mixed-version provider sets legitimately differ oncomputeUnitsConsumed,stackHeight,logMessagesandrewards, which would have thrown spurious quorum errors. Each is verified unread by the SDK.blockTimeis deliberately not normalised —eventsClientconsumes it asdepositTimestamp.nullgetTransactionmeans the queried provider lacks it, not that it does not exist, andprocessEventFromTxdecodesnullas "no events" — so pruned or lagging nodes could silently erase a real deposit or fill. Contested absence now fails loudly; genuine unanimous absence still returnsnull, so backfills past every provider's archive horizon do not become hard failures.Scope:
getSignaturesForAddressis deliberately not quorumedAn earlier revision of this PR quorumed it too. That was wrong and would have stalled SVM event ingestion.
queryAllEventsfetches the newest page atlimit: 1000and filters by slot only afterwards, so the page tracks the confirmed tip — any signature landing between two providers' responses shifts the window and deep equality fails.RetryProviderexcludeslatest/pendingfrometh_getBlockByNumberquorum for the same reason;eth_getLogsis quorum-safe only because its range is bounded.This leaves an omission vector open: a malicious provider serving the signature page can withhold signatures, and quorum on
getTransactioncannot detect what it was never asked about. Closing it needs a range-reconciling comparator — naive truncation to the common leading edge is gameable in exactly the hide direction — so it is left to a follow-up.Note this multiplies
getTransactionRPC volume by the quorum threshold during event scans, which only applies whennodeQuorumThreshold > 1.Tests cover each case.
eslint,prettier,tsc --project tsconfig.build.jsonand the SVM suites pass. The lint run also caught that the previous commit brokeno-fallthroughby putting a comment betweencaselabels; fixed here.