Validate RPC log/callTrace response #1152
Conversation
|
@kyscott18 Added a check after filtering transactions, was wondering if this would be sufficient for realtime-sync |
| throw new Error( | ||
| `Received callTrace/log in the block referencing transaction that is not in current head block '${block.hash}'`, | ||
| ); |
There was a problem hiding this comment.
Not blocking, but I'd advocate for a more specific error message which includes the suspicious transaction hashes. That would make it much easier for us to submit bug reports to chains / RPC providers.
Also, what if the lengths are different because we have an extra required transaction? That scenario would suggest a Ponder bug rather than a faulty RPC.
There was a problem hiding this comment.
- With regards to error message, do you think it would be better to include the list of suspicious tx hashes in the message?
- We would expect requiredTransactions size to be larger than transactions length only in the case if some logs or callTraces that is matched with filter are pointing to more transactions, than there are transactions filtered by those logs/traces.
const transactions = block.transactions.filter(({ hash }) => requiredTransactions.has(hash), );
There was a problem hiding this comment.
- yes
- I think the transaction validation should be more precise. Something like:
const blockTransactionsHashes = new Set(
block.transactions.map((t) => t.hash),
);
for (const hash of Array.from(requiredTransactions)) {
if (blockTransactionsHashes.has(hash) === false) {
// throw error, include `hash`
}
}I also think it would be nice to have something that compares blockHash in logs and callTraces to block.hash
|
@kyscott18 @0xOlias I have added the validation checks in sync-realtime for logs response:
Similarly, for sync-historical I have added the following checks:
|
| ); | ||
|
|
||
| // Validate that traces point to the valid transaction hash in the block and collect them | ||
| const transactionHashes = new Set( |
There was a problem hiding this comment.
Would be nice to move this outside of the Set constructor, so it's not doing two things at once
| @@ -393,6 +393,15 @@ export const createRealtimeSync = ( | |||
| `Detected invalid '${args.network.name}' eth_getLogs response.`, | |||
| ); | |||
| } | |||
|
|
|||
| // Check that logs refer to the correct block | |||
| for (const log of logs) { | |||
| (t) => t.hash === transactionHash, | ||
| ); | ||
| if (!isTransactionValid) { | ||
| throw new Error( |
There was a problem hiding this comment.
We should add something to catch this error and make sure it is getting logged properly
| @@ -485,9 +485,18 @@ export const createRealtimeSync = ( | |||
| // Protect against RPCs returning empty logs. Known to happen near chain tip. | |||
| if (block.logsBloom !== zeroLogsBloom && logs.length === 0) { | |||
| throw new Error( | |||
| `Detected invalid '${args.network.name}' eth_getLogs response.`, | |||
| "Detected invalid eth_getLogs response. `block.logsBloom` is not empty but zero logs were returned.", | |||
Added a check to validate whether filtered logs and calltraces point to valid transactions in the block for realtime-sync engine. Relates to #1150 issue.