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

refactor: end block ordering - staking after gov and module sorting #2937

Merged
merged 5 commits into from
Oct 3, 2022

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Oct 3, 2022

Closes: #XXX

What is the purpose of the change

ModuleNames() function returns modules in non-deterministic order due to a map iteration. Our DAG took those modules to construct total order from a partial order. Although we tried to sort the module names to prevent non-determinism, we ended up erroneously providing the unsorted copy.

Additionally, there was no ordering constraint for the staking EndBlock to go after the gov EndBlock. The constraint is added here.

It seems that the DAG ordering problems haven't been causing issues since they were added. The ordering between stake and gov modules only matters in some edge cases. In our case, the following proposal has led to the dependency being fatal: https://www.mintscan.io/osmosis/proposals/337

Lastly, @alexanderbez created a test to make sure that the ordering is deterministic.

Testing and Verifying

This change added tests.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? no
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
  • How is the feature or change documented? not applicable

@github-actions github-actions bot added the C:app-wiring Changes to the app folder label Oct 3, 2022
@p0mvn p0mvn changed the title refactor: add staking after gov and module sorting refactor: end block ordering - staking after gov and module sorting; Bez's test Oct 3, 2022
@p0mvn p0mvn added V:state/breaking State machine breaking PR A:backport/v12.x backport patches to v12.x branch labels Oct 3, 2022
@p0mvn p0mvn changed the title refactor: end block ordering - staking after gov and module sorting; Bez's test refactor: end block ordering - staking after gov and module sorting Oct 3, 2022
@p0mvn p0mvn removed the A:backport/v12.x backport patches to v12.x branch label Oct 3, 2022
@p0mvn p0mvn marked this pull request as ready for review October 3, 2022 19:00
@p0mvn p0mvn requested review from a team, alexanderbez, mattverse and nicolaslara October 3, 2022 19:00
@ValarDragon ValarDragon added the A:backport/v12.x backport patches to v12.x branch label Oct 3, 2022
@p0mvn p0mvn added A:backport/v13.x and removed A:backport/v12.x backport patches to v12.x branch labels Oct 3, 2022
@p0mvn p0mvn merged commit f057e15 into main Oct 3, 2022
@p0mvn p0mvn deleted the roman/ordering branch October 3, 2022 19:16
mergify bot pushed a commit that referenced this pull request Oct 3, 2022
…2937)

* refactor: add staking after gov and module sorting

* gov first staking last

* add Bez's test

* fix old test ordering

* changelog

(cherry picked from commit f057e15)
pysel pushed a commit to pysel/osmosis that referenced this pull request Oct 4, 2022
…smosis-labs#2937)

* refactor: add staking after gov and module sorting

* gov first staking last

* add Bez's test

* fix old test ordering

* changelog
@github-actions github-actions bot mentioned this pull request Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:app-wiring Changes to the app folder V:state/breaking State machine breaking PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants