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.

Sign up for GitHub

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

Jump to bottom

fix: mocha block sync on v2.x #3843

Merged
merged 11 commits into from
Sep 16, 2024
Merged

fix: mocha block sync on v2.x #3843

merged 11 commits into from
Sep 16, 2024

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Sep 5, 2024 •
edited
Loading

Closes #3840 by implementing option 2 because it seems like a bad practice to modify genesis.json after the chain has launched.

Blocked on celestiaorg/celestia-core#1469

Testing

I can state sync and block sync on Arabica and Mocha

./scripts/arabica.sh
./scripts/arabica-block-sync.sh
./scripts/mocha.sh
./scripts/mocha-block-sync.sh

@rootulp rootulp self-assigned this Sep 5, 2024
@rootulp rootulp mentioned this pull request Sep 5, 2024
Merged
@rootulp rootulp changed the title fix: block sync for mocha fix: mocha block sync Sep 5, 2024
@cmwaters
Copy link
Contributor

cmwaters commented Sep 5, 2024

What about just defaulting to 1 if it is empty?

@rootulp
Copy link
Collaborator Author

rootulp commented Sep 5, 2024

What about just defaulting to 1 if it is empty?

I can do. The current approach only special cases mocha-4 b/c that's the only testnet we support that has an empty app version. IMO future chains (even testnets) should have an initial app version populated so this approach doesn't default to 1 for them, instead it panics.

@rootulp
Copy link
Collaborator Author

rootulp commented Sep 5, 2024

Discussed w/ Callum offline and decided to not special case mocha. Instead we'll default to app version 1.

@rootulp rootulp changed the title fix: mocha block sync fix: mocha block sync on v2.x Sep 5, 2024
@rootulp
Copy link
Collaborator Author

rootulp commented Sep 5, 2024

Need to forward port this to main

rootulp added a commit to celestiaorg/celestia-core that referenced this pull request Sep 5, 2024
@rootulp
fix: mocha block sync (#1469)
aef3227 
Unblocks celestiaorg/celestia-app#3843
@rootulp rootulp mentioned this pull request Sep 5, 2024
Merged
@rootulp rootulp marked this pull request as ready for review September 5, 2024 16:12
@rootulp rootulp requested a review from a team as a code owner September 5, 2024 16:12
@rootulp rootulp marked this pull request as draft September 11, 2024 10:16
@rootulp
Copy link
Collaborator Author

rootulp commented Sep 11, 2024 •
edited
Loading

Moving to draft. Discussion w/ @cmwaters :

  • Delete the Mocha genesis file from this PR
  • Add a unit test with a genesis file that is broken in the same way as the Mocha genesis file
  • Create an issue to track adding a end to end test that starts a testapp with the Mocha genesis file and verifies that the app can get to block 1. Created: Test with Mocha and Mainnet genesis files #3861

@rootulp rootulp marked this pull request as ready for review September 11, 2024 19:49
@rootulp rootulp mentioned this pull request Sep 11, 2024
Open
cmwaters
cmwaters approved these changes Sep 16, 2024
View reviewed changes
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

LGTM

@rootulp rootulp enabled auto-merge (squash) September 16, 2024 17:00
@rootulp rootulp merged commit d3f8b29 into celestiaorg:v2.x Sep 16, 2024
31 of 32 checks passed
@rootulp rootulp deleted the rp/init-chain-fix branch September 16, 2024 17:00
rootulp added a commit that referenced this pull request Sep 16, 2024
@rootulp
fix: block sync on mocha on main (#3846)
628fb99 
Closes #3840
Forward port of #3843


## Testing

I can state sync and block sync on Arabica and Mocha

```shell
./scripts/arabica.sh
./scripts/arabica-block-sync.sh
./scripts/mocha.sh
./scripts/mocha-block-sync.sh
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evan-forbes evan-forbes evan-forbes approved these changes

@cmwaters cmwaters cmwaters approved these changes

@ninabarbakadze ninabarbakadze Awaiting requested review from ninabarbakadze

@staheri14 staheri14 Awaiting requested review from staheri14

Assignees

@rootulp rootulp

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rootulp @cmwaters @evan-forbes @ninabarbakadze