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

stateful-storage: check hash in signed extension #2137

Merged
merged 7 commits into from
Aug 30, 2024
Merged

stateful-storage: check hash in signed extension #2137

merged 7 commits into from
Aug 30, 2024

Conversation

aramikm
Copy link
Collaborator

@aramikm aramikm commented Aug 21, 2024 •
edited
Loading

Goal

The goal of this PR is to check the hash for stateful storage transactions and fail early.

Closes #1860

Checklist

  • e2e Tests added?
  • Benchmarks added?
  • Spec version incremented?

@aramikm aramikm requested a review from wilwade as a code owner August 21, 2024 23:22
@aramikm aramikm mentioned this pull request Aug 21, 2024
Closed
4 tasks
@codecov Codecov
Copy link

codecov bot commented Aug 21, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
pallets/stateful-storage/src/lib.rs 79.85% <100.00%> (ø)

@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Aug 21, 2024
shannonwells
shannonwells previously approved these changes Aug 22, 2024
View reviewed changes
Copy link
Collaborator

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

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

Only a couple of little things for comments, not worth doing a CI round trip unless you are already going to change those files.

pallets/stateful-storage/src/signed_extension.rs Outdated Show resolved Hide resolved
saraswatpuneet
saraswatpuneet previously approved these changes Aug 22, 2024
View reviewed changes
Copy link
Collaborator

@saraswatpuneet saraswatpuneet left a comment

Choose a reason for hiding this comment

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

Read through, looks good

Do we need to update any weights (overhead) ?

enddynayn
enddynayn reviewed Aug 23, 2024
View reviewed changes
pallets/stateful-storage/src/signed_extension.rs Outdated
.unwrap_or_default()
.try_into()
.unwrap_or_default(),
_ => 255u8,
Copy link
Collaborator

@enddynayn enddynayn Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It seems we could end up with an error code that’s not entirely accurate, particularly in the case where zero is returned. Using unwrap_or_default() might not be the best approach here, as falling back to the default value could potentially introduce a bug. That said, I might be overlooking something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, just a note: the "Custom Error" code that we get as part of the RPC "1010" error is entirely arbitrary, and does not indicate the pallet. So a code of 9 (StalePageHash) is not meaningful to the end user, because they don't know what pallet enum that comes from. I do think somewhere there is an enum of custom RPC error codes for Frequency; perhaps we should map to that instead? Sadly Polkadot/Substrate does not give us a way to propagate anything more than an integer value for custom RPC errors.

Copy link
Collaborator Author

@aramikm aramikm Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@enddynayn The first unwrap_or_default() tries to convert encoded error value (coming from the position of error in the Error enum in the pallet) to u32 which means it can not fail. The second unwrap_or_default() tries to convert u32 to u8 but that would only fail if we have more than 256 errors in the same pallet which is highly unlikely.

@JoeCap08055 Since this is coming from validate it means that after submission you would immediately get that error back. And for any individual extrinsic since the client knows what type of extrinsic they are submitting it is obvious what that error would mean. I have to look into the batch usecase since that would be trickier.

In any case since we only have 256 unique values for different errors we might be able to create a universal custom error enum which would be different between the errors in different pallets but that does not seem to scale based on the total number of pallets and errors. I'm open to suggestions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting approach but works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably a wider issue than this PR, but the universal enum idea might not be so bad; we wouldn't need to map every error from every pallet; only the ones that can happen in a SignedExtension check. There shouldn't be that many.

Anyway, this approach works for now; maybe it's worth a backlog issue to revisit how we do this across all of our signed extensions, or maybe not.

enddynayn
enddynayn previously approved these changes Aug 23, 2024
View reviewed changes
Copy link
Collaborator

@enddynayn enddynayn left a comment

Choose a reason for hiding this comment

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

👍 great!

@github-actions github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Aug 23, 2024
@aramikm
Copy link
Collaborator Author

aramikm commented Aug 23, 2024

Read through, looks good

Do we need to update any weights (overhead) ?

I calculated the overhead benchmark and it didn't change. My guess is that since we don't have anything inside pre_dispatch it didn't add to overhead.

@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Aug 23, 2024
JoeCap08055
JoeCap08055 previously approved these changes Aug 26, 2024
View reviewed changes
@github-actions github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Aug 27, 2024
@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Aug 27, 2024
@aramikm aramikm dismissed stale reviews from JoeCap08055, enddynayn, saraswatpuneet, and shannonwells August 29, 2024 16:34

Code is refactored

enddynayn
enddynayn reviewed Aug 30, 2024
View reviewed changes
runtime/frequency/src/lib.rs
}) => Self::extract_hash_data(call),
RuntimeCall::FrequencyTxPayment(
FrequencyPaymentCall::pay_with_capacity_batch_all { calls, .. },
) => calls.iter().flat_map(|c| Self::extract_hash_data(c)).collect(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! I like this refactor.

enddynayn
enddynayn approved these changes Aug 30, 2024
View reviewed changes
Copy link
Collaborator

@enddynayn enddynayn left a comment

Choose a reason for hiding this comment

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

Great! 👍 I also suggest adding some unit tests to the signed extension.

JoeCap08055
JoeCap08055 approved these changes Aug 30, 2024
View reviewed changes
Copy link
Collaborator

@JoeCap08055 JoeCap08055 left a comment

Choose a reason for hiding this comment

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

Nice rework.

  • Pulled & built branch
  • Ran unit tests
  • Ran e2e tests

Looks good!

shannonwells
shannonwells approved these changes Aug 30, 2024
View reviewed changes
Copy link
Collaborator

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

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

Looks good to me. I think it's good to go and if it can be done the same way as CapacityEligibleCalls I think it can be revisited later.

@aramikm aramikm merged commit 978d6af into main Aug 30, 2024
27 checks passed
@aramikm aramikm deleted the hash_check_extrinsic branch August 30, 2024 21:44
@aramikm
Copy link
Collaborator Author

aramikm commented Aug 30, 2024

Refactor issue is created #2145

@aramikm aramikm mentioned this pull request Oct 11, 2024
Merged
3 tasks
aramikm added a commit that referenced this pull request Oct 11, 2024
@aramikm
revert: stalehash extension (#2184)
3931f20 
# Goal
The goal of this PR is <!-- insert goal here -->

Closes #2183 

Reverting #2137 


# Checklist
- [x] e2e Tests updated
- [X] Benchmarks updated
- [X] Spec version incremented?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoeCap08055 JoeCap08055 JoeCap08055 approved these changes

@enddynayn enddynayn enddynayn approved these changes

@shannonwells shannonwells shannonwells approved these changes

@wilwade wilwade Awaiting requested review from wilwade wilwade is a code owner

@mattheworris mattheworris Awaiting requested review from mattheworris

@saraswatpuneet saraswatpuneet Awaiting requested review from saraswatpuneet

Assignees
No one assigned
Labels
metadata-changed Metadata has changed since the latest full release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

target_hash RPC Level validation
5 participants
@aramikm @shannonwells @enddynayn @saraswatpuneet @JoeCap08055