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: state sync on v2.x #3808

Merged
merged 8 commits into from
Aug 22, 2024
Merged

fix: state sync on v2.x #3808

merged 8 commits into from
Aug 22, 2024

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Aug 22, 2024 •
edited
Loading

Closes #3804 with a hacky approach that infers the app version based on the snapshot height and mounts KV stores accordingly.

Kinda related to #3804 but we didn't hit this because we don't have tests that try to state sync so I created #3807

Testing

Using both the arabica.sh and mocha.sh scripts in this PR, I can start state syncing. I let the Arabica node fully state sync to the tip of the chain and it worked.

@rootulp rootulp self-assigned this Aug 22, 2024
rootulp
rootulp commented Aug 22, 2024
View reviewed changes
app/app.go Outdated Show resolved Hide resolved
@rootulp rootulp marked this pull request as ready for review August 22, 2024 03:12
@rootulp rootulp requested a review from a team as a code owner August 22, 2024 03:12
@rootulp rootulp requested review from cmwaters, staheri14 and evan-forbes and removed request for staheri14 August 22, 2024 03:12
evan-forbes
evan-forbes previously approved these changes Aug 22, 2024
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

might want to mention somewhere in the scripts or in their names that they do statesync, but not blocking

thanks for testing state sync!! great catch. ideally we test this with knuu

@rootulp rootulp marked this pull request as draft August 22, 2024 13:50
@cmwaters
Copy link
Contributor

I am going to test this out manually. If it passes we can merge this for now and then work on adding it to the e2e tests and a more long term fix

rootulp
rootulp commented Aug 22, 2024
View reviewed changes
app/app.go Show resolved Hide resolved
@cmwaters cmwaters marked this pull request as ready for review August 22, 2024 14:13
cmwaters
cmwaters approved these changes Aug 22, 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.

So long as everything is green this should be good to merge

@rootulp rootulp changed the title fix: state sync fix: state sync on v2.x Aug 22, 2024
@rootulp rootulp mentioned this pull request Aug 22, 2024
Merged
@rootulp rootulp merged commit fb79af5 into celestiaorg:v2.x Aug 22, 2024
33 checks passed
@rootulp rootulp deleted the rp/fix-state-sync branch August 22, 2024 16:36
rootulp added a commit that referenced this pull request Aug 22, 2024
@rootulp
fix: state sync on main (#3811)
ad250dd 
Forward port of #3808
@evan-forbes evan-forbes mentioned this pull request Aug 25, 2024
Closed
@rootulp rootulp mentioned this pull request Aug 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ninabarbakadze ninabarbakadze ninabarbakadze approved these changes

@cmwaters cmwaters cmwaters approved these changes

@evan-forbes evan-forbes Awaiting requested review from evan-forbes

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