Deprecate IAVL and fix all dependency and test#3146
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
* main: Fix edge case for non atomic commit (#3128)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3146 +/- ##
==========================================
+ Coverage 58.56% 58.86% +0.30%
==========================================
Files 2099 2096 -3
Lines 173961 173528 -433
==========================================
+ Hits 101881 102155 +274
+ Misses 62899 62236 -663
+ Partials 9181 9137 -44
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| with: | ||
| version: 1.58.0 No newline at end of file | ||
| version: 1.58.0 | ||
| breaking: false No newline at end of file |
There was a problem hiding this comment.
We do not want this skipped on main though.
There was a problem hiding this comment.
On main, buf.yaml has 9 modules (includes sei-iavl/proto). On this branch, we removed sei-iavl/proto, leaving 8 modules. buf breaking compares the PR branch against main and requires the same number of modules — it fails with "input contained 8 images, whereas against contained 9 images."
I think this is a one-time problem that only exists while main still has sei-iavl/proto. Once this PR merges into main, both sides will have 8 modules and buf breaking will work again and we can revert back this change
There was a problem hiding this comment.
What exact breaking failure do we get?
I think there might be a less aggressive way to skip that specific error for specific protos, compared to disabling it for everything.
Worst comes to worst, we can always force-merge this PR knowing that the breaking changes in buf are expected.
The edge-case I worry about is one where a breaking proto change would find its way to main between this PR landing, and the setting getting reverted.
There was a problem hiding this comment.
We would get this failure:
input contained 8 images, whereas against contained 9 images
There was a problem hiding this comment.
I'm fine to force merge it as well, reverted the change
* main: fix(flatkv): harden error handling for readonly store and crash cleanup (#3127) Remove redundant makefiles, unused code and adjust docs (#3149) Fix minor logging issues (#3147) PLT-225 Use in place orbytes for bloom filter (#3144) Eliminate per-iteration allocation in SetEvmOnlyBlockBloom (#3133)
| exit 0 | ||
| fi | ||
| echo "packages=./..." >> "$GITHUB_OUTPUT" | ||
|
|
|
|
||
| // app.StoreConsensusParams(ctx, &tmproto.ConsensusParams{Block: &tmproto.BlockParams{MaxGas: -5000000}}) | ||
| // require.Panics(t, func() { app.getMaximumBlockGas(ctx) }) | ||
| // } |
There was a problem hiding this comment.
Do you know by any chance why these were commented out? Can we just remove these?
There was a problem hiding this comment.
Not really sure, but I will look into it and see if we can remove it.
There was a problem hiding this comment.
The test calls app.InitChain followed by app.StoreConsensusParams, which is a pattern that was broken when Sei forked and restructured the ABCI interface (e.g., merging BeginBlock/DeliverTx/EndBlock into FinalizeBlock, and detaching the Tendermint header from the Cosmos header). The InitChain + StoreConsensusParams flow likely doesn't work the same way as before any more.
So it's mostly dead code that never worked in the Sei fork and safe to delete.
* main: Make cryptosim state store backend configurable + No Op Wrapper + Read Disable Config (#3145) Add warning message for IAVL deprecation (#3159) Change default min valid per window to zero (#3157) support for starting autobahn from non-zero global block (#3136) Fix upgrade list comparison to respect semver (#3153)
|
Added a note to PR description regarding the intentional breaking proto changes. Will be force merging this as a one off. Cc @yzang2019 and thanks for driving this work; super glad to see sei-iavl go 🙌 |
Describe your changes and provide context
This PR fully removes the sei-iavl dependency from sei-chain, completing the migration to sei-db (memiavl) as the sole state commitment backend. After this change, nodes no longer open or depend on application.db, and all state commitment is handled exclusively through storev2/rootmulti backed by memiavl.
Note: This PR has an intentionally breaking Proto change, since it removes the sei-iavl protos from root.