Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Validate RPC log/callTrace response #1152

Merged
merged 10 commits into from
Oct 11, 2024
Merged

Validate RPC log/callTrace response #1152

Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
91fe0cd
sync-realtime fix: checks if filtered logs/callTraces point to tx in …
khaidarkairbek Oct 9, 2024
5cb87f5
minor fix
khaidarkairbek Oct 9, 2024
e20f6bd
added invalid tx hashes into error for debugging
khaidarkairbek Oct 9, 2024
9093a34
checks logs' blockhash, more precise tx hash validation
khaidarkairbek Oct 9, 2024
2449608
added validation for trace and log tx hashes in sync-historical
khaidarkairbek Oct 10, 2024
e44b7e5
added validation for log and trace blockhash in sync-historical
khaidarkairbek Oct 10, 2024
35688df
nits
kyscott18 Oct 11, 2024
e5e295d
return from historical sync error
kyscott18 Oct 11, 2024
09aea59
Merge branch 'main' into khaid/validate
kyscott18 Oct 11, 2024
51b8db2
chore: changeset
kyscott18 Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/core/src/sync-realtime/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,13 @@ export const createRealtimeSync = (
requiredTransactions.has(hash),
);

// Validate that filtered logs/callTraces point to valid transaction in the block
if (transactions.length !== requiredTransactions.size) {
throw new Error(
`Received callTrace/log in the block referencing transaction that is not in current head block '${block.hash}'`,
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. With regards to error message, do you think it would be better to include the list of suspicious tx hashes in the message?
  2. 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), );

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. yes
  2. 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

}

////////
// Transaction Receipts
////////
Expand Down
Loading