-
Notifications
You must be signed in to change notification settings - Fork 415
refactor(scripts): combine block sync scripts #4833
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
refactor(scripts): combine block sync scripts #4833
Conversation
Signed-off-by: Arya Nair <[email protected]>
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.
Yes this is on the right track for the block sync script. We can do one PR for the block sync script and a follow-up PR for the state sync script.
- Can you please add mainnet to this PR?
- Can you include in the PR description how this script was tested? For example, locally you should invoke
./scripts/block-sync.sh
and include the important console output for each network you tested on.
Command -
|
Command -
|
Command -
|
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.
Pull Request Overview
This PR consolidates multiple block sync scripts into one unified script for Celestia networks and removes the separate Mocha and Arabica sync scripts.
- Consolidates sync logic into scripts/block-sync.sh
- Removes redundant scripts for Mocha and Arabica networks
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
scripts/mocha-block-sync.sh | Removed – functionality merged into unified script |
scripts/block-sync.sh | New unified script supporting arabica, mocha, and mainnet |
scripts/arabica-block-sync.sh | Removed – functionality merged into unified script |
@rootulp can you check the PR? |
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.
LGTM. Thanks for the hard work on this.
Based on your test output, it looks like the Mocha seeds may no longer be correct because there are a lot of addressbook logs before your Mocha node was able to process block heights up to 3. As a follow-up we can explore finding more up to date seeds / peers for Mocha.
@rootulp fixed the network names |
Let's hold off on merging this until we get v4.x.x-mocha released and deployed. Also I'd like to manually test locally before merging. |
Manually tested and LGTM. |
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.
thank you!
Closes #4820