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.

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

feat: check class & transaction hashes #2349

Merged
merged 2 commits into from
Nov 6, 2024
Merged

feat: check class & transaction hashes #2349

merged 2 commits into from
Nov 6, 2024

Conversation

vbar
Copy link
Contributor

@vbar vbar commented Nov 4, 2024

for equality with locally-computed hashes (and fail on mismatch).

Fixes #2346.

@vbar vbar requested a review from a team as a code owner November 4, 2024 13:08
Copy link
Contributor

@kkovaacs kkovaacs left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!


#[derive(Debug)]
pub struct ClassDefinitionWithHash<'a> {
pub cls_def: ClassDefinition<'a>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a small nit: can we maybe call this definition instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upon reflection, I don't like the whole ClassDefinitionWithHash - now that I've finally fit the carpet, :-) it isn't really used that much...

})
if hash != layout.hash {
tracing::debug!(input_hash=%layout.hash, actual_hash=%hash, "Class hash mismatch");
Err(SyncError2::ClassHashComputationError.into())
Copy link
Member

Choose a reason for hiding this comment

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

nit: I wonder if we should differentiate between a mismatch (ie. proper layout, bad values) and hash computation error (which means bad layout).
So for the former: ClassHashMismatch (maybe we need to add this variant)
and for the latter: ClassHashComputationError (which really is an InvalidDto but at a higher level)

Copy link
Contributor Author

@vbar vbar Nov 5, 2024

Choose a reason for hiding this comment

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

Well, I wasn't keen on extending SyncError / SyncError2, b/c

  1. I don't really understand why there's 2 of them, and
  2. the type isn't used here anyway,
    but yes, it can be done...

Copy link
Member

Choose a reason for hiding this comment

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

Re 1. Don't worry, there will be one error type soon.

Copy link
Member

@CHr15F0x CHr15F0x left a comment

Choose a reason for hiding this comment

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

LGTM 🙇

A small nit - maybe we should use 2 variants there to differentiate between utter garbage (invalid dto, resulting in class computation error) and a mismatch (valid dto, the values dont match).

@vbar vbar force-pushed the vbar/use_cls_tx_hash branch from 711d68f to 3705dec Compare November 6, 2024 08:50
@vbar vbar merged commit 951c790 into main Nov 6, 2024
7 checks passed
@vbar vbar deleted the vbar/use_cls_tx_hash branch November 6, 2024 09:07
@vbar vbar mentioned this pull request Nov 6, 2024
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.

P2P sync: use the proto's class_hash & transaction_hash fields to bail early
3 participants