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

testground's app version is not supported by the versioned module manager #3242

Closed
staheri14 opened this issue Apr 2, 2024 · 8 comments
Closed
Labels
enhancement New feature or request WS: Big Blonks 🔭 Improving consensus critical gossiping protocols

Comments

@staheri14
Copy link
Contributor

staheri14 commented Apr 2, 2024

Problem

To facilitate unbounded block size experiments in e2e tests using knuu, we require to use the testground app version that extends the permissible square size to 512, thereby accommodating block sizes of up to ~126MB. However, the current module manager fails to properly render this app version, resulting in a validator panic when such a version is provided. Specifically, when running SimpleE2ETest with the testground's app version, the validator panics at the outset with the following message (thrown at this line):

panic: version 420420420 not supported

Where 420420420 is the testground app version

More context

The maximum block size is limited by the following factors:

  • The versioned constant SquareSizeUpperBound
  • The parameter GovMaxSquareSize in the blob's module
  • The max_bytes setting in the consensus block parameters

Although the last two parameters can be modified at the time of genesis creation, the first is dependent on the application version. Currently, versions v1 and v2 support a maximum square size of 128, which limits the block size to 8 MB. However, our goal is to increase the block size to 100 MB.

We have previously introduced a testground application version that raises the SquareSizeUpperBound to 512, allowing for block sizes greater than 100 MB. Nonetheless, with the implementation of the versioned module manager, it is now necessary to ensure that the testground application version is also relevant to this module manager. Specifically, there should be a mechanism in versioned module manager to use the latest app version available in the code when the testground app version is seen (that is a potential solution). This capability is currently lacking.

@staheri14 staheri14 added enhancement New feature or request WS: Big Blonks 🔭 Improving consensus critical gossiping protocols and removed needs:triage labels Apr 2, 2024
@cmwaters
Copy link
Contributor

cmwaters commented Apr 2, 2024

In the constructor of the application, each module states the versions it supports and thus the application determines all the versions that it can run over. Given that v2 is just v1 and v2, v420420420 is not supported and thus it panics when genesis provides it an app version that it does not support

@cmwaters
Copy link
Contributor

cmwaters commented Apr 2, 2024

If we want to have an unbounded test, we could use a different docker image that we build specifically for our tests. This would allow us to make other alterations that aren't available in our official docker image. I believe this was the approach that testground used to take

@staheri14
Copy link
Contributor Author

If we want to have an unbounded test, we could use a different docker image that we build specifically for our tests. This would allow us to make other alterations that aren't available in our official docker image. I believe this was the approach that testground used to take

So, this means we need to maintain a separate long-lived branch mirroring the main branch with additional modifications for testing purposes only? At this point, I cannot see a lot of such test-related modifications that would motivate having a separate branch except for amending the square size limit. Nevertheless, I am not opposing it, just brain storming.

@cmwaters
Copy link
Contributor

cmwaters commented Apr 2, 2024

It's a bit difficult to inject code for test purposes in the production binary yet prevent it from users from using it. If we want greater flexibility (for testing sake), perhaps some of these variables shouldn't be hardcoded

@staheri14
Copy link
Contributor Author

To outline the next steps required to implement this change, following actions seem necessary for a long-term sustainable solution (please let me know your thoughts):

As for my current effort to test unbounded block size, I can push an image manually (which I will do), but it is better to aim for a more sustainable long-term solution.

@evan-forbes
Copy link
Member

evan-forbes commented Apr 2, 2024

I believe this was the approach that testground used to take

yeah, correct unfortunatley

we had to have this hack

if !versionSupported {
modules = m.versionedModules[appconsts.LatestVersion]

other than that tho, testground is just using the testground specific app version in the genesis

tmNode, app, err := testnode.NewCometNode(baseDir, &ucfg)
if err != nil {
return err
}
cn.cmtNode = tmNode
cctx := testnode.NewContext(ctx, cn.kr, ucfg.TmConfig, cn.params.ChainID, ucfg.AppConfig.API.Address)

return genesis.Document(
l.ecfg,
TestgroundConsensusParams(params),
l.params.ChainID,
gentxs,
accounts,
params.GenesisModifiers...,
)

func TestgroundConsensusParams(params *Params) *tmproto.ConsensusParams {
cp := app.DefaultConsensusParams()
cp.Block.MaxBytes = int64(params.MaxBlockBytes)
cp.Version.AppVersion = testgroundconsts.Version
return cp
}

@cmwaters
Copy link
Contributor

cmwaters commented Apr 3, 2024

Create a separate branch dedicated to testing.

We don't necessarily have to have a separate branch. It could be possible to have two binaries within the same codebase (and two docker images for each binary)

@staheri14
Copy link
Contributor Author

The resolution for the current issue is captured in #3370. Therefore, we can close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request WS: Big Blonks 🔭 Improving consensus critical gossiping protocols
Projects
None yet
Development

No branches or pull requests

3 participants