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

feat: new command for 3D dymint store migration #1302

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

srene
Copy link
Contributor

@srene srene commented Jan 3, 2025

PR Standards

Opening a pull request should be able to meet the following requirements

--

PR naming convention: https://hackmd.io/@nZpxHZ0CT7O5ngTp0TP9mg/HJP_jrm7A


Close #1301

<-- Briefly describe the content of this pull request -->

For Author:

  • Targeted PR against correct branch
  • included the correct type prefix in the PR title
  • Linked to Github issue with discussion and accepted design
  • Targets only one github issue
  • Wrote unit and integration tests
  • All CI checks have passed
  • Added relevant godoc comments

For Reviewer:

  • confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • confirmed all author checklist items have been addressed

After reviewer approval:

  • In case targets main branch, PR should be squashed and merged.
  • In case PR targets a release branch, PR should be rebased.

@srene srene requested a review from a team as a code owner January 3, 2025 15:49
@srene srene self-assigned this Jan 3, 2025
@srene srene marked this pull request as draft January 3, 2025 15:49
@srene srene changed the title feat: add command for 3D dymint store migration feat: new command for 3D dymint store migration Jan 3, 2025
Copy link

codecov bot commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 4.03226% with 119 lines in your changes missing coverage. Please review.

Project coverage is 17.29%. Comparing base (ed72d9b) to head (15e0ff2).
Report is 31 commits behind head on main.

Files with missing lines Patch % Lines
store/store.go 0.00% 61 Missing ⚠️
...github.com/dymensionxyz/dymint/store/mock_Store.go 0.00% 25 Missing ⚠️
cmd/dymint/commands/migration.go 0.00% 20 Missing ⚠️
...ub.com/dymensionxyz/dymint/block/mock_ExecutorI.go 0.00% 4 Missing ⚠️
...b.com/tendermint/tendermint/proxy/mock_AppConns.go 0.00% 2 Missing ⚠️
block/submit.go 50.00% 0 Missing and 1 partial ⚠️
cmd/dymint/commands/init.go 0.00% 1 Missing ⚠️
cmd/dymint/main.go 0.00% 1 Missing ⚠️
...com/dymensionxyz/dymint/block/mock_FraudHandler.go 0.00% 1 Missing ⚠️
...nxyz/dymint/da/mock_DataAvailabilityLayerClient.go 0.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1302      +/-   ##
==========================================
- Coverage   17.72%   17.29%   -0.44%     
==========================================
  Files         187      195       +8     
  Lines       55555    58975    +3420     
==========================================
+ Hits         9849    10201     +352     
- Misses      44182    47183    +3001     
- Partials     1524     1591      +67     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@srene srene marked this pull request as ready for review January 3, 2025 16:37
block/submit.go Show resolved Hide resolved
@@ -111,4 +111,6 @@ type Store interface {
LoadLastBlockSequencerSet() (types.Sequencers, error)

Close() error

Run3DMigration(da string) error
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure it needs to be in the interface.
migration code can cast to DefaultStore and call migration code

store/store.go Outdated
@@ -423,6 +424,79 @@ func (s *DefaultStore) LoadLastBlockSequencerSet() (types.Sequencers, error) {
return sequencers, nil
}

func (s *DefaultStore) Run3DMigration(da string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why it's Store's method
probably should be some migration code in block/
migration cmd has both store and db (store.NewPrefixKV(baseKV, mainPrefix))

Copy link
Contributor

Choose a reason for hiding this comment

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

I've moved it to a standalone func

Comment on lines 237 to 240
// skip validation for DRS version 0 (2D rollapps)
if version == 0 {
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

gonna merge for now but need some reasoning why this is OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think we need to skip drs validation for previous to migration blocks, because in case blocks were received via p2p previous to the migration, when is trying to validate them it will fail to find drs info on store. i replaced this check by only logging error when drs version info is not found (cannot be considered fraud, so i think is fine).

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah makes sense!

@danwt danwt requested review from mtsitrin and danwt January 6, 2025 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2d rollapps upgrade
3 participants