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

Pass app version to CheckTx #2214

Open
rootulp opened this issue Aug 4, 2023 · 7 comments
Open

Pass app version to CheckTx #2214

rootulp opened this issue Aug 4, 2023 · 7 comments
Labels
needs:investigation more knowledge is required before item can be groomed

Comments

@rootulp
Copy link
Collaborator

rootulp commented Aug 4, 2023

Context

While working on Option A of #2156 @evan-forbes and I realized that CheckTx doesn't have access to the current app version. We want access to the app version so that CheckTx can fetch the upper bound for max square size (a versioned constant) to reject blob txs that are too large.

Problem

// CheckTx implements the ABCI interface and executes a tx in CheckTx mode. This
// method wraps the default Baseapp's method so that it can parse and check
// transactions that contain blobs.
func (app *App) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {

doesn't pass app version. If we want to modify CheckTx in the future in a way that is compatible with single binary syncs, we need a way to branch control flow in CheckTx based on the current block header's app version.

Proposal

Consider passing app version. Note this likely involves changes to ABCI / Cosmos SDK so this proposal needs further investigation.

@evan-forbes
Copy link
Member

evan-forbes commented Aug 4, 2023

You're correct that we have to make sure that we have access to the app version, fortunately we should be able to get the app version from the checktx state, which is initialized each block during Commit here. note that the latest header is used to initialize the checktxstate each block.

The general important thing that is missing is we have to pass the app version to all validate basic methods, and I think in most cases we should be able to do that by getting the latest value from the sdk.Contexts that are initialized from their relavant states (checktx, deliver, proposal)

@rootulp
Copy link
Collaborator Author

rootulp commented Aug 4, 2023

Thanks for sharing! The baseApp's checkState appears private. Do you know which getter we can use to fetch it? I couldn't find one.

@evan-forbes
Copy link
Member

evan-forbes commented Aug 4, 2023

ahh sorry, by "get from the checktx state", I mean from that context. Which is passed via antehandlers and the rest of the state machine. this might mean that we have to first finish #1166

@evan-forbes
Copy link
Member

alternatively we could also just use the app.AppVersion method

@evan-forbes
Copy link
Member

can we close this? or what could close this?

@rootulp
Copy link
Collaborator Author

rootulp commented Aug 29, 2023

We're no longer doing Option A of #2156 so maybe we close this issue for now and open a new issue when we need to plumb app version into various ValidateBasic()s.

@evan-forbes
Copy link
Member

for posterity and to hopefully save a few future braincycles:

the alternative to plumbing app version to validate basics which would be to keep all validate basic method stateless, and then refactor any important checks that would have been performed there to be antehandlers, along with defining a new type so we can define a new validate basic (:grimacing:)

@evan-forbes evan-forbes added investigation item tracks efforts related to an investigation. does not always require a PR to close. and removed T:investigate labels May 13, 2024
@evan-forbes evan-forbes added discussion inherently unactionable issue for the sole purpose of discussion and removed discussion inherently unactionable issue for the sole purpose of discussion labels Jun 5, 2024
@evan-forbes evan-forbes changed the title CheckTx doesn't pass app version Pass app version to CheckTx Jun 5, 2024
@evan-forbes evan-forbes added needs:investigation more knowledge is required before item can be groomed and removed investigation item tracks efforts related to an investigation. does not always require a PR to close. labels Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:investigation more knowledge is required before item can be groomed
Projects
None yet
Development

No branches or pull requests

2 participants