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

Make the encoding require the app version #3134

Closed
evan-forbes opened this issue Feb 27, 2024 · 7 comments
Closed

Make the encoding require the app version #3134

evan-forbes opened this issue Feb 27, 2024 · 7 comments
Assignees
@cmwaters
Labels
proposal item is not yet actionable and is suggesting a change that must first be agreed upon

Comments

@evan-forbes
Copy link
Member

The encoding could differ between app versions. This could result in non-determinism. The safest thing to do imo would be to require the app version when creating an encoding config, and we create a new encoding config each height. This way we ensure that there encoding is done deterministically across heights and nodes. There might be other mechanisms for forcing this.
@evan-forbes evan-forbes added proposal item is not yet actionable and is suggesting a change that must first be agreed upon needs:triage needs:discussion item needs to be discussed as a group in the next sync. if marking an item, pls be prepped to talk labels Feb 27, 2024
@cmwaters
Copy link
Contributor

Another option is to have the encoding config simply support all versions

@evan-forbes
Copy link
Member Author

Another option is to have the encoding config simply support all versions

does the user need to specify which version of the encoding is needed at a given point?

@cmwaters
Copy link
Contributor

I guess what we are trying to prevent is someone submitting a transaction that is not yet or no longer supported right?

Or is there any other use cases you had in mind?

@evan-forbes
Copy link
Member Author

evan-forbes commented Feb 28, 2024 •
edited
Loading

the main thing is trying to avoid non-determinism when upgrading since being able to decode or not decode a tx could result in that if we didn't get everything perfect. I was worried about differences in the antehandlers during process and even finalize block. Like, if someone submits a new tx type, will that get passed to ante handlers on nodes that have been upgraded where nodes that haven't upgraded won't?

by requiring the app version, we ensure that this doesn't occur. We could also ensure that all old tx types are treated as unsupported, instead of quasi-supported

@cmwaters
Copy link
Contributor

For the antehandler, I was planning on trying to decode it and if it was successful then checking that the msg type was supported by the current version

@evan-forbes evan-forbes added the WS: V2 ✌️ lemongrass hardfork related label Feb 28, 2024
@evan-forbes evan-forbes added this to the v2 milestone Feb 28, 2024
@cmwaters cmwaters self-assigned this Feb 28, 2024
@evan-forbes
Copy link
Member Author

I could see that working as well, can we document that somewhere if its not already? this is marked as needs-discussion with the intention to disucss this during the next v2 sync so we can discuss further there.

while requiring the app version is annoying, imo I think its annoying in a good way similar to a strict type system. Especially if it avoids a bug that requires manually digging through protobuf bytes

@cmwaters cmwaters mentioned this issue Mar 6, 2024
Merged
@evan-forbes evan-forbes removed needs:triage needs:discussion item needs to be discussed as a group in the next sync. if marking an item, pls be prepped to talk WS: V2 ✌️ lemongrass hardfork related labels Mar 11, 2024
@evan-forbes evan-forbes removed this from the v2 milestone Mar 11, 2024
@evan-forbes
Copy link
Member Author

we don't need this because of #3162

cmwaters added a commit that referenced this issue Mar 14, 2024
@cmwaters @rootulp
feat!: prototype for gatekeeping messages based on their version (#3162)
91603c4 
Ref: #3134

This PR solves the problem of ensuring the messages belonging to modules
not part of the current app version aren't executed.

It does this in two ways:
- Introducing an antehandler decorator to be predominantly used in
CheckTx to immediately reject transactions giving users a useful error
message (instead of just failing to execute)
- Introduces a `CircuitBreaker` implementation to the `MsgServiceRouter`
which prevents the execution of messages not belonging to the current
app version. We need this because another module may call a message that
is not current supported (think a governance proposal)

I had several complications with the wiring of this given the structure
of the SDK and tried a few different variants - this one I think being
the better.

It uses the configurator which is reponsible for registering services to
read all the methods a modules grpc Server supports and extracting out
the message names and mapping them to one or more versions that they are
supported for.

---------

Co-authored-by: Rootul P <[email protected]>
ninabarbakadze pushed a commit to ninabarbakadze/celestia-app that referenced this issue Apr 2, 2024
@cmwaters @rootulp @ninabarbakadze
feat!: prototype for gatekeeping messages based on their version (cel…
920f2a2 
…estiaorg#3162)

Ref: celestiaorg#3134

This PR solves the problem of ensuring the messages belonging to modules
not part of the current app version aren't executed.

It does this in two ways:
- Introducing an antehandler decorator to be predominantly used in
CheckTx to immediately reject transactions giving users a useful error
message (instead of just failing to execute)
- Introduces a `CircuitBreaker` implementation to the `MsgServiceRouter`
which prevents the execution of messages not belonging to the current
app version. We need this because another module may call a message that
is not current supported (think a governance proposal)

I had several complications with the wiring of this given the structure
of the SDK and tried a few different variants - this one I think being
the better.

It uses the configurator which is reponsible for registering services to
read all the methods a modules grpc Server supports and extracting out
the message names and mapping them to one or more versions that they are
supported for.

---------

Co-authored-by: Rootul P <[email protected]>
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this issue Aug 1, 2024
@cmwaters @rootulp
feat!: prototype for gatekeeping messages based on their version (#3162)
7c9d60d 
Ref: celestiaorg/celestia-app#3134

This PR solves the problem of ensuring the messages belonging to modules
not part of the current app version aren't executed.

It does this in two ways:
- Introducing an antehandler decorator to be predominantly used in
CheckTx to immediately reject transactions giving users a useful error
message (instead of just failing to execute)
- Introduces a `CircuitBreaker` implementation to the `MsgServiceRouter`
which prevents the execution of messages not belonging to the current
app version. We need this because another module may call a message that
is not current supported (think a governance proposal)

I had several complications with the wiring of this given the structure
of the SDK and tried a few different variants - this one I think being
the better.

It uses the configurator which is reponsible for registering services to
read all the methods a modules grpc Server supports and extracting out
the message names and mapping them to one or more versions that they are
supported for.

---------

Co-authored-by: Rootul P <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cmwaters cmwaters

Labels
proposal item is not yet actionable and is suggesting a change that must first be agreed upon
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@cmwaters @evan-forbes