search

celestiaorg / celestia-app

PoS application for the consensus portion of the Celestia network. Built using celestia-core (fork of CometBFT) and the cosmos-sdk
https://celestia.org
Apache License 2.0
328 stars 270 forks source link

Pass app version to CheckTx #2214

Open rootulp opened 11 months ago

rootulp commented 11 months ago

Context

While working on Option A of https://github.com/celestiaorg/celestia-app/issues/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

https://github.com/celestiaorg/celestia-app/blob/70b06242669760008428a2b2d265ebfea36990fb/app/check_tx.go#L12-L15

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 https://github.com/celestiaorg/celestia-app/issues/1166

evan-forbes commented 11 months ago

alternatively we could also just use the app.AppVersion method

evan-forbes commented 10 months ago

can we close this? or what could close this?

rootulp commented 10 months ago

We're no longer doing Option A of https://github.com/celestiaorg/celestia-app/issues/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 commented 10 months ago

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:)