-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[pallet-transaction-storage] Improved check_proof check + tests + docs
#10153
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
Conversation
|
/cmd fmt |
|
/cmd prdoc --audience runtime_dev --bump patch |
…time_dev --bump patch'
| if total_chunks.is_zero() { | ||
| const MSG: &str = "No chunks to build proof for."; | ||
| return Err(Error::Application(Box::from(MSG))); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this now an error? This will make the block production fail. Before it was just not providing any proof, which is the sensible thing to do here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this now an error? This will make the block production fail. Before it was just not providing any proof, which is the sensible thing to do here.
oh, good point, thank you, you're right, my bad, please check here: 04cb7d7
I also tried to remove that .unwrap( and change return type to Result<Option, but I am not sure about the end (looks ok-ish and should not happen):
debug_assert!(false, "No chunk matched the target_chunk_index; logic error");
Ok(None)
@bkchr What would you suggest here?
I also talked to @arkpar and added an early return when the chunk is found, since we don’t need to process the subsequent transactions.
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
|
/cmd fmt |
Co-authored-by: Bastian Köcher <[email protected]>
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
1 similar comment
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
|
/cmd bench --pallet pallet_transaction_storage --runtime dev |
|
Command "bench --pallet pallet_transaction_storage --runtime dev" has started 🚀 See logs here |
|
Command "bench --pallet pallet_transaction_storage --runtime dev" has failed ❌! See logs here |
This PR:
check_proofand itsbinary_search_by_keychunkensure_chunk_proof_workstest, which covers all possible chunk index build/verify proof roundtrips (to catch all corner cases)pallet-transaction-storage