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

Add a --create-object-mappings node CLI option #3125

Merged
merged 2 commits into from
Oct 14, 2024
Merged

Add a --create-object-mappings node CLI option #3125

merged 2 commits into from
Oct 14, 2024

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Oct 13, 2024

This PR adds a --create-object-mappings CLI option to the node, which enables mapping generation for new and historic blocks. This option is implied by dev mode.

It also fixes the implementation of this feature to:

  • correctly skip mappings in the first (partial) block passed to the archiver when it is initialised
  • skip getting the runtime mappings, rather than skipping sending the notifications of those mappings

Code contributor checklist:

@teor2345 teor2345 added enhancement New feature or request node Node (service library/node app) labels Oct 13, 2024
@teor2345 teor2345 self-assigned this Oct 13, 2024
Copy link
Contributor Author

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I think this is what we want, but maybe the code is too strict on skipping some mappings?

crates/sc-consensus-subspace/src/archiver.rs Outdated Show resolved Hide resolved
crates/sc-consensus-subspace/src/archiver.rs Outdated Show resolved Hide resolved
Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Make sense overall, but I'd call it --create-object-mappings instead of --force-mapping. And accordingly skip creation altogether if it is false (the default).

Those who need them would have to specify a CLI option, while everyone else would not have to pay the cost of runtime API calls and save some CPU cycles.

crates/sc-consensus-subspace/src/archiver.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 changed the title Add a --force-mapping node CLI option Add a --create-object-mappings node CLI option Oct 14, 2024
@teor2345 teor2345 added this pull request to the merge queue Oct 14, 2024
Merged via the queue into main with commit 1f65ca4 Oct 14, 2024
10 checks passed
@teor2345 teor2345 deleted the full-map-cli branch October 14, 2024 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request node Node (service library/node app)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants