Skip to content

fix(client): correlate batch responses by JSON-RPC id#67

Closed
Dollyerls wants to merge 2 commits into
tempoxyz:mainfrom
Dollyerls:fix/batch-response-id-correlation
Closed

fix(client): correlate batch responses by JSON-RPC id#67
Dollyerls wants to merge 2 commits into
tempoxyz:mainfrom
Dollyerls:fix/batch-response-id-correlation

Conversation

@Dollyerls

Copy link
Copy Markdown
Contributor

Batch responses must be correlated by id, not by position

Description

Client.SendBatch returns the raw decoded reply slice, and every caller (plus the doc
comment example) indexes it positionally — responses[0] is assumed to answer the first
Add. JSON-RPC 2.0 explicitly allows the server to return batch responses in any
order
, requiring the client to correlate them via the id member. We never did that.

Steps to reproduce

  1. Build a 2-request batch (eth_sendRawTransaction × 2).
  2. Point it at a node/proxy that returns the two responses in reversed order.
  3. Read responses[0].

Expected

responses[0] corresponds to the first request added.

Actual

responses[0] holds whatever the server happened to put first — for the reversed reply,
the wrong transaction's hash/nonce/receipt. Against well-behaved test servers it looks
correct; against a real reordering node it mismatches intermittently. For a parallel-nonce
SDK this means tracking or acting on the wrong transaction.

SendBatch returned responses in server order while callers index them
positionally. JSON-RPC 2.0 allows arbitrary batch response ordering, so
a reordering node could associate a response with the wrong request.
Realign responses to request order via the id field.
SendBatch returned responses in server order while callers index them
positionally. JSON-RPC 2.0 allows arbitrary batch response ordering, so
a reordering node could associate a response with the wrong request.
Realign responses to request order via the id field.
@brendanjryan

Copy link
Copy Markdown
Collaborator

Thanks for the PR! Rolled fwd with your commits in the linked PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants