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

Fix archive sync panic #1051

Merged
merged 11 commits into from
Dec 16, 2024
Merged

Fix archive sync panic #1051

merged 11 commits into from
Dec 16, 2024

Conversation

orriin
Copy link
Contributor

@orriin orriin commented Dec 3, 2024

  • Fixes the archive sync getting stuck at block 2585477 due to panic by adding a codeSubstitute that patches the buggy code with one that doesn't panic.
  • Updates node/src/chain_spec/finney.rs so Finney chainspec always builds with the codeSubstitues field.

@orriin orriin requested a review from unconst as a code owner December 3, 2024 03:30
camfairchild
camfairchild previously approved these changes Dec 3, 2024
Copy link
Contributor

@ales-otf ales-otf left a comment

Choose a reason for hiding this comment

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

It would be better to have the substitute in the ChainSpec builder or in the function producing the chain spec instead. It's more reliable and doesn't require patching. If it's possible.

scripts/build_all_chainspecs.sh Outdated Show resolved Hide resolved
@ales-otf
Copy link
Contributor

ales-otf commented Dec 3, 2024

It would be better to have the substitute in the ChainSpec builder or in the function producing the chain spec instead. It's more reliable and doesn't require patching. If it's possible.

Probably this could help: https://paritytech.github.io/polkadot-sdk/master/sc_chain_spec/fn.set_code_substitute_in_json_chain_spec.html

@orriin
Copy link
Contributor Author

orriin commented Dec 4, 2024

It would be better to have the substitute in the ChainSpec builder or in the function producing the chain spec instead. It's more reliable and doesn't require patching. If it's possible.

@ales-otf I tried this but couldn't figure it out. It seems the ChainSpec Builder doesn't support setting code_substitutes, also the fn you linked seems to operate on a JSON object, whereas we have a ChainSpec. Let me know if you have any ideas tho! Would be great to get it working that way.

@orriin orriin added the no-spec-version-bump PR does not contain changes that requires bumping the spec version label Dec 4, 2024
@ales-otf
Copy link
Contributor

ales-otf commented Dec 4, 2024

It would be better to have the substitute in the ChainSpec builder or in the function producing the chain spec instead. It's more reliable and doesn't require patching. If it's possible.

@ales-otf I tried this but couldn't figure it out. It seems the ChainSpec Builder doesn't support setting code_substitutes, also the fn you linked seems to operate on a JSON object, whereas we have a ChainSpec. Let me know if you have any ideas tho! Would be great to get it working that way.

I believe you can modify this JSON: https://github.com/opentensor/subtensor/blob/devnet-ready/node/src/chain_spec/finney.rs#L198

Oh, it's just genesis. Then yeah. Looks like you have to patch the json the way you do. As an option, you can get the json from the ChainSpec (https://paritytech.github.io/polkadot-sdk/master/sc_chain_spec/struct.GenericChainSpec.html#method.as_json) modify it using the function and build the ChainSpec from the modified JSON. It's ugly, but it's more safe doing it in rust, than in a shell script.

BTW, it looks like we can omit using the script and just hardcode everything, as there is with_code method.

@orriin
Copy link
Contributor Author

orriin commented Dec 4, 2024

@ales-otf that is the genesis JSON not the chain spec JSON

@ales-otf
Copy link
Contributor

ales-otf commented Dec 4, 2024

@ales-otf that is the genesis JSON not the chain spec JSON

yeah, I've updated my comment

@orriin
Copy link
Contributor Author

orriin commented Dec 4, 2024

As an option, you can get the json from the ChainSpec (https://paritytech.github.io/polkadot-sdk/master/sc_chain_spec/struct.GenericChainSpec.html#method.as_json) modify it using the function and build the ChainSpec from the modified JSON. It's ugly, but it's more safe doing it in rust, than in a shell script.

Good spot! I will have a go at this.

BTW, it looks like we can omit using the script and just hardcode everything, as there is with_code method.

I don't think with_code is what we want, I believe it sets the genesis code rather than a code substitute for a specific block number

@ales-otf
Copy link
Contributor

ales-otf commented Dec 4, 2024

I don't think with_code is what we want, I believe it sets the genesis code rather than a code substitute for a specific block number

Yeah, that's exactly what I meant. We now need to use this script to keep the genesis mostly because of the genesis code. The rest comes frome that generate_genesis function. So if we put the code value into ChainSpec builder and if we also add substitute there, we can not only omit the shell script, but we can even connect to the running mainnet using cargo run -- --chain finney or something like this - without requiring to have the chainspec file at all. Still, it would be good to track changes in the chainspecs. Some warning system at least. Also, it would be better to extract the code value into a separate file and include it into the source from that file. It would be easier to edit the file and the git diffs would be smaller.

Opened an issue for this.

@ales-otf
Copy link
Contributor

ales-otf commented Dec 4, 2024

I don't think with_code is what we want, I believe it sets the genesis code rather than a code substitute for a specific block number

Yeah, that's exactly what I meant. We now need to use this script to keep the genesis mostly because of the genesis code. The rest comes frome that generate_genesis function. So if we put the code value into ChainSpec builder and if we also add substitute there, we can not only omit the shell script, but we can even connect to the running mainnet using cargo run -- --chain finney or something like this - without requiring to have the chainspec file at all. Still, it would be good to track changes in the chainspecs. Some warning system at least. Also, it would be better to extract the code value into a separate file and include it into the source from that file. It would be easier to edit the file and the git diffs would be smaller.

This is definitely out of scope of this PR. I've just spotted the method and this idea came to mind.

@orriin
Copy link
Contributor Author

orriin commented Dec 5, 2024

A bit hacky but figured out a solution for setting codeSubstitutes at the node/src/chain_spec/finney.rs level, thanks @ales-otf !

Copy link
Contributor

@ales-otf ales-otf left a comment

Choose a reason for hiding this comment

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

Please, look the comment - this change is important.

And just an additional not much important styling thing, which would improve readability and debugging — to encapsulate this whole part related to setting the substitute into a separate function.

node/src/chain_spec/finney.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ales-otf ales-otf left a comment

Choose a reason for hiding this comment

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

Thanks!

@orriin orriin requested a review from camfairchild December 16, 2024 11:08
@sam0x17 sam0x17 added the skip-cargo-audit This PR fails cargo audit but needs to be merged anyway label Dec 16, 2024
@sam0x17 sam0x17 merged commit 05bea90 into devnet-ready Dec 16, 2024
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-spec-version-bump PR does not contain changes that requires bumping the spec version skip-cargo-audit This PR fails cargo audit but needs to be merged anyway
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants