Skip to content

Conversation

@fcecin
Copy link

@fcecin fcecin commented Apr 15, 2025

Description

This PR is part of #24114

Summary:

Description:

This upgrade has been mostly guided by SDK v52 changes done to port v52 to CometBFT v1.0.1, with the assumption that if it worked for SDK v52, it should still work now. So in a sense this is a backport of the SDK v52's Comet v1 port of the root module. Only the changes that were necessary to get the SDK working with Comet v1 were ported.

Tests:

Testing that involve modules that were not yet upgraded to Comet v1 will not work. Unit tests for the root module pass. e.g. go test -mod=readonly -tags="cgo ledger test_ledger_mock norace" -count=1 ./.... When all tests are enabled, we can act on their failures and fix whatever issues remain.

Other CI failures:

There are other CI failures also pointing to modules that have not yet ported to Comet v1 in this PR (such as x/evidence). Like the tests, these CI jobs won't pass until the offending modules are ported.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

- fix go.mod with comet v1 upgraded dependencies: api, core, x/tx
- fix go.mod: add temporary replace to ./store (comet v1 upgraded)
- fix tendermint -> cometbft.v1 imports & proto field names
- regenerated gogo proto code (pb.go files) tendermint -> cometbft
- doc fixes for cometbft v1
- misc Go API fixes (incomplete, WIP)
@ironbird-prod
Copy link

ironbird-prod bot commented Apr 15, 2025

Ironbird - launch a network To use Ironbird, you can use the following commands:
  • /ironbird loadtests - List of load test modes that ironbird can run against testnet
  • /ironbird start OR /ironbird start --load-test-config= - Launch a testnet with the specified chain and load test configuration.
  • /ironbird chains - List of chain images that ironbird can use to spin-up testnet
Custom Load Test Configuration You can provide a custom load test configuration using the `--load-test-config=` flag:
/ironbird start cosmos --load-test-config={
  "block_gas_limit_target": 0.75,
  "num_of_blocks": 50,
  "msgs": [
    {"weight": 0.3, "type": "MsgSend"},
    {"weight": 0.3, "type": "MsgMultiSend"},
	{"weight": 0.4, "type": "MsgArr", "ContainedType": "MsgSend", "NumMsgs": 3300}
  ]
}

Use /ironbird loadtests to see more examples.

fcecin added 2 commits April 15, 2025 19:07
- remove store module replace & fix existing require instead;
go mod tidy now works
- more comet v1 code fixes (enums, fields, imports) (WIP)
- fix more ABCI enum values & types to comet 1 format
- fix several callsites to satisfy the new Comet 1 API,
following mostly what SDK v52 did to satisfy Comet 1
- revert addition of internal/conv/comet.go (unnecessary)
- fix x/consensus/keeper/keeper_test.go and
ToProtoConsensusParams() to handle new ABCI consensus
params API
- regen golden x/genutil/types/testdata/app_genesis.json
which adds new ABCI consensus params for Comet v1
- fix TestABCI_CheckTx assuming CHECK_TX_TYPE_CHECK is 0

tmPk, err := encoding.PubKeyFromProto(tmProtoPk)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods Warning

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt

tmPk, err := encoding.PubKeyFromProto(tmProtoPk)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods Warning

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
fcecin added 5 commits April 24, 2025 16:11
- fix baseapp ABCI now handles FeatureParams
- fix baseapp ABCI tests now handles FeatureParams
- fix lint/fmt problem with import statement ordering
@fcecin fcecin marked this pull request as ready for review April 25, 2025 00:23
@github-actions
Copy link
Contributor

@fcecin your pull request is missing a changelog!

return errors.Wrap(err, "Failed to get consensus key algo")
}

nodeID, valPubKey, err := genutil.InitializeNodeValidatorFiles(config, consensusKey)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did this api change?

Copy link
Author

@fcecin fcecin Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InitializeNodeValidatorFiles calls InitializeNodeValidatorFilesFromMnemonic, which calls privval.LoadOrGenFilePV from Comet here, but this API has changed in Comet V1. So I just copied the fixed InitializeNodeValidatorFilesFromMnemonic from SDK v52, which takes a key type string as argument, and pulled in everything that needs. If we don't want this, we can do something else.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(x2) I have reverted all these "ed25519 validator keys but BLS keys maybe" changes. CometBFT can take a nil for a key generator function and it will just use the ed25519 as default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good as long as we have a comment explianing that default behavior

@fcecin fcecin merged commit 7acf61e into feature/comet_v1_upgrade Apr 28, 2025
38 of 48 checks passed
@fcecin fcecin deleted the fcecin/comet_v1_root branch April 28, 2025 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants