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: expose config.IsSealed #414

Merged

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Aug 16, 2024

Expose config.IsSealed so that we can check it before invoking seal on it from the v2 and v3 state machine. We can remove this as soon as Cosmos SDK removes the global singleton config.

@rootulp rootulp self-assigned this Aug 16, 2024
@rootulp rootulp requested a review from a team as a code owner August 16, 2024 16:17
@rootulp rootulp requested review from staheri14, rach-id, cmwaters and evan-forbes and removed request for a team, staheri14 and rach-id August 16, 2024 16:17
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.

Is there a way to only call this once in the multiplexer and then never call it in the application?

@rootulp
Copy link
Collaborator Author

rootulp commented Aug 16, 2024

Is there a way to only call this once in the multiplexer and then never call it in the application?

No because the v2 state machine needs to work independently of the multiplexer. There are at least these two combinations:

  • User runs v2.0.0 binary which should invoke config.Seal()
  • User runs v3.0.0 binary which has multiplexer. The multiplexer imports v2.0.0 state machine which invokes config.Seal(). The v3.0.0 state machine can not run config.Seal() but that means it isn't usable unless run via the multiplexer.

@evan-forbes
Copy link
Member

the v2 state machine needs to work independently of the multiplexer

if the multiplexer is the only thing that calls seal, and it gets ran no matter which state machine is picked, then is this true?

its not really worth spending a lot of time on, but if there are common initializing things, then I think refactoring them into the multiplexer could be beneficial. Like, afaiu, we're not stuck calling app.New, we can modify app.New to be whatever we want now because we're calling Multiplexer.New, and that function is initializing the application upon each celestia-appd start

@rootulp
Copy link
Collaborator Author

rootulp commented Aug 17, 2024

if the multiplexer is the only thing that calls seal, and it gets ran no matter which state machine is picked, then is this true?

We can do this for the v3.0.0 binary but the v2.x.x binaries need to be able to run without the multiplexer so they have to invoke config.Seal() themselves.

We could make a 2.x.x release and inform users that they can't run it independently and it must be run via a multiplexer but that would be a breaking change and pretty confusing for users.

We could also make a 2.x.x release that exposes a variant of app.New() that is only meant to be called by the multiplexer which bypasses the config.Seal(). That seems more complicated than conditionally invoking config.Seal() from v3.

@evan-forbes
Copy link
Member

thanks for explaining 🙂, makes sense

@rootulp rootulp merged commit 3562f46 into celestiaorg:release/v0.46.x-celestia Sep 9, 2024
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants