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

go/upgrade: Adjust MaxTxSize and MaxBlockSize in consensus240 handler #5588

Merged
merged 2 commits into from
Mar 8, 2024

Conversation

kostko
Copy link
Member

@kostko kostko commented Mar 7, 2024

This is needed as DCAP quotes are larger and nodes running multiple confidential runtimes may otherwise exceed the max transaction size.

Copy link

netlify bot commented Mar 7, 2024

Deploy Preview for oasisprotocol-oasis-core canceled.

Name Link
🔨 Latest commit 4944990
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-oasis-core/deploys/65eab961e2ec7b0008d193df

@kostko kostko force-pushed the kostko/feature/upgrade-max-tx-size branch from 1c85327 to d19b668 Compare March 7, 2024 08:28
@kostko kostko marked this pull request as ready for review March 7, 2024 08:29
Copy link
Member

@ptrus ptrus left a comment

Choose a reason for hiding this comment

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

Probably makes sense to also update defaults in

initGenesisFlags.String(cfgConsensusMaxTxSizeBytes, "32kb", "cometbft maximum transaction size (in bytes)")
initGenesisFlags.String(cfgConsensusMaxBlockSizeBytes, "21mb", "cometbft maximum block size (in bytes)")

go/upgrade/migrations/consensus_240.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 7, 2024

Codecov Report

Attention: Patch coverage is 44.44444% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 65.94%. Comparing base (ea412e2) to head (90a6df2).
Report is 5 commits behind head on master.

❗ Current head 90a6df2 differs from pull request most recent head 4944990. Consider uploading reports for the commit 4944990 to get more accurate results

Files Patch % Lines
go/upgrade/migrations/consensus_240.go 44.44% 2 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5588      +/-   ##
==========================================
+ Coverage   65.89%   65.94%   +0.04%     
==========================================
  Files         577      577              
  Lines       58879    58887       +8     
==========================================
+ Hits        38799    38833      +34     
+ Misses      15567    15533      -34     
- Partials     4513     4521       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kostko
Copy link
Member Author

kostko commented Mar 7, 2024

Probably makes sense to also update defaults in

But then it won't update anything in tests.

This is needed as DCAP quotes are larger and nodes running multiple
confidential runtimes may otherwise exceed the max transaction size.
@kostko kostko force-pushed the kostko/feature/upgrade-max-tx-size branch from 90a6df2 to 4944990 Compare March 8, 2024 07:08
@peternose
Copy link
Contributor

But then it won't update anything in tests.

in this case, I like to prepare a draft PR which we can merge as soon as we do the upgrade.

@kostko
Copy link
Member Author

kostko commented Mar 8, 2024

in this case, I like to prepare a draft PR which we can merge as soon as we do the upgrade.

Good idea, I'll do that.

@kostko kostko merged commit a903c45 into master Mar 8, 2024
5 checks passed
@kostko kostko deleted the kostko/feature/upgrade-max-tx-size branch March 8, 2024 07:34
@kostko
Copy link
Member Author

kostko commented Mar 8, 2024

#5591

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants