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

migration: update cometbft tx size for t78 migration #4614

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

conorsch
Copy link
Contributor

@conorsch conorsch commented Jun 14, 2024

Describe your changes

We've already updated the CometBFT config template, but running nodes on the current testnet will still be using the prior value, generated at time of join. Let's update the value as part of pd migrate, logging a warning if we fail to do so.

Issue ticket number and link

Refs #4594, #4582.

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    relevant to consensus-breaking logic and how it's handled via migrations

@conorsch
Copy link
Contributor Author

Encountered some errors while testing this. In short, the TendermintConfig struct is stricter about type validation than CometBFT is. For example, seed_mode = "false" is accepted by CometBFT, but the struct demands seed_mode = false. Worse, an indexer node will fail, due to the psql indexer not being supported by tendermint-rs (#3188). Let's log warnings in those instances, rather than allowing the error to percolate up to bailing out on the top-level pd migrate action. If that still proves problematic, we can fall back to asking node operators to make the change manually.

Comment on lines 527 to 544
/// Edit the node's CometBFT config file to set mempool.max_tx_bytes to the required value.
/// This value will affect consensus, but the config setting is specified for CometBFT
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Edit the node's CometBFT config file to set mempool.max_tx_bytes to the required value.
/// This value will affect consensus, but the config setting is specified for CometBFT
/// Edit the node's CometBFT config file to set mempool.max_txs_bytes to the required value.
/// This value won't affect consensus, but the config setting is specified for CometBFT

@conorsch conorsch force-pushed the migration-for-cometbft-txs-bytes branch from 1099505 to dbc7c33 Compare June 14, 2024 21:59
@conorsch conorsch added the consensus-breaking breaking change to execution of on-chain data label Jun 14, 2024
@conorsch conorsch force-pushed the migration-for-cometbft-txs-bytes branch from dbc7c33 to 869dee8 Compare June 17, 2024 15:10
@conorsch conorsch requested a review from erwanor June 17, 2024 16:15
@conorsch conorsch marked this pull request as ready for review June 17, 2024 16:15
@conorsch
Copy link
Contributor Author

I'm happy with these changes: the cometbft munging will bail out before overwriting if it cannot interpret the on-disk cometbft file, logging a warning. That works well enough with the variety of munged configs I tested against.

// We expect the pre-migration value to be '1073741824', assuming it's unedited from `pd testnet join`.
// let pre_migration_value = cometbft_config.mempool.max_txs_bytes;
// The new value was updated in GH4594 and represents ~10MB or (2^20 * 10).
let post_migration_value = 10485760;
Copy link
Member

Choose a reason for hiding this comment

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

There are two max_tx_bytes parameters that we want to change:

  • the first one is a mempool config parameter that controls the maximum size of a transaction to be included in the mempool/gossiped to other nodes
  • the second is a consensus parameter that controls the maximum size of block data (including headers)

We want to change both values:

Copy link
Contributor

@dynst dynst Jun 17, 2024

Choose a reason for hiding this comment

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

There are two different mempool configuration parameters that are easily mixed up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The max_tx_bytes vs max_txs_bytes distinction is certainly ripe for confusion, but if we're changing two values in the CometBFT config for Testnet 78, then we should update both values as part of the migration flow. I'll make a change to this PR to accommodate.

We've already updated the CometBFT config template, but running nodes on
the current testnet will still be using the prior values, generated at
time of join. Let's update the values as part of `pd migrate`, logging a
warning if we fail to do so.

Refs #4594, #4582.
@conorsch conorsch force-pushed the migration-for-cometbft-txs-bytes branch from 8e312f4 to 4eecfcb Compare June 18, 2024 17:30
@conorsch conorsch marked this pull request as ready for review June 18, 2024 18:43
@conorsch
Copy link
Contributor Author

This is ready for review again. After migration, the cometbft config's mempool block should look like:

[mempool]
recheck = true
broadcast = true
wal_dir = ""
size = 5000
max_txs_bytes = 10485760
cache_size = 10000
keep-invalid-txs-in-cache = false
max_tx_bytes = 30720
max_batch_bytes = 0

I made sure to test with a customized config that set persistent_peers, and those changes were preserved post-migration.

@conorsch conorsch requested a review from erwanor June 18, 2024 18:43
Copy link
Member

@erwanor erwanor left a comment

Choose a reason for hiding this comment

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

Looks great

@conorsch conorsch merged commit 7698ce5 into main Jun 20, 2024
16 checks passed
@conorsch conorsch deleted the migration-for-cometbft-txs-bytes branch June 20, 2024 15:25
conorsch added a commit that referenced this pull request Jun 28, 2024
During team sync today we decided that this new value is superior.

Follow-up to related work in #4614, #4627, and #4632.
conorsch added a commit that referenced this pull request Jun 28, 2024
During team sync today we decided that this new value is superior.

Follow-up to related work in #4614, #4627, and #4632.
erwanor pushed a commit that referenced this pull request Jun 28, 2024
## Describe your changes
During team sync today we decided that this new value is superior.



## Issue ticket number and link
Follow-up to related work in #4614, #4627, and #4632.

## Checklist before requesting a review

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

> This change does affect consensus and so should be treated carefully.

Co-authored-by: Conor Schaefer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus-breaking breaking change to execution of on-chain data
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants