Skip to content
New issue

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

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

Already on GitHub? # to your account

Bugfix: trimmed metadata hash comparison fails in is_codegen_valid_for #1301

Closed

Conversation

tadeohepperle
Copy link
Contributor

Fixes #1286

What is the bug:

Metadata can be trimmed with Metadata::retain(..). This is done by the CLI tool, e.g. subxt metadata --url wss://rpc.polkadot.io --pallets "ElectionProviderMultiPhase,System" retains only the specified pallets.
When comparing the hash of trimmed metadata (metadata that contains only a limited number of pallets/runtime apis) with the hash of the original metadata using the hashers only_these_pallets / only_these_runtime_apis, the hashes were different. This comparison was made in the generated is_codegen_valid_for function.

Reason for the bug

in Metadata::retain(..) 3 things happen:

  • the unneeded pallets/apis are removed at the top level
  • all types that only these pallets/apis used are removed as well
  • the top level enums (call, event and error) are Variant types. The variants relating to unneeded pallets/apis are removed. This happens in retain_pallets_in_runtime_outer_types().

The last point is what caused the bug:
When specifying only_these_pallets for a MetadataHasher, the variant types of the top level enums (call, event and error) were modified to discard unneded pallets before the hash is calculated. HOWEVER the same types can be included in other places of the metadata. In @niklasad1 case for the staking-miner, the "Events" storage entry of the "System" pallet, had a type of Vec<Event>, where Event is the event top level type. But we did not go through all types of the metadata checking for each one if they are a top level variant and modifying it accordingly before computing its hash.
==> This caused the bug.

Solution

To solve the bug, the hash computation of the top level enums has been moved to the top of the MetadataHasher::hash(..) function. Also we explicitly insert the hash of the modified enums into a cache that is used throughout the rest of the hashing process. So whenever we encounter a type again, that is a top level enum, it is already present in the cache with the correct value.

Alternative solution (not implemented)

We could also just clone and trim the metadata when calling MetadataHasher::hash() and specific pallets/apis are set.
This is probably less performant, but maybe in the end easier and more consistent.

Testing

Added a test to confirm this works and manually tested it for the example Niklas provided.

@tadeohepperle tadeohepperle changed the title Tadeohepperle/bugfix trimmed metadata hash Bugfix: trimmed metadata hash comparison fails in is_codegen_valid_for Dec 1, 2023
@tadeohepperle
Copy link
Contributor Author

Closing This PR, opening a new PR with a different solution to the same problem.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

codegen: is_codegen_valid_for doesn't work as expected for "trimmed metadata"
1 participant