-
Notifications
You must be signed in to change notification settings - Fork 80
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
feat(sequencer)!: update to ABCI v0.38 #831
Conversation
…ria into noot/sequencer-0.38
…ria into noot/sequencer-0.38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No blockers, but a few questions/suggestions/nitpicks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just comments re issues for tracking the next penumbra release && populating AppHash
with already available data.
penumbra-ibc = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.69.1", default-features = false } | ||
penumbra-proto = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.69.1" } | ||
penumbra-tower-trace = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.69.1" } | ||
# update once https://github.com/penumbra-zone/penumbra/commit/8b06546af43bf073fd99f3f9d82b8afb51872489 makes it into a release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's have a tracking issue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tower-actor = "0.1.0" | ||
cnidarium = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.69.1" } | ||
cnidarium-component = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.69.1" } | ||
cnidarium = { git = "https://github.com/penumbra-zone/penumbra.git", rev = "8b06546af43bf073fd99f3f9d82b8afb51872489" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as for the penumbra deps, right? Let's add this to a tracking issue
evidence_hash: Some(Hash::default()), // unused | ||
height: finalize_block.height, | ||
last_block_id: None, // unused | ||
last_commit_hash: Some(Hash::default()), // unused |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out, this is just: Some(Hash::default()) -> Some(Hash::None)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
infra approval for charts
Summary
updates the sequencer app to use ABCI v0.38, which replaces
begin_block
/deliver_tx
/end_block
with one call,finalize_block
.relevant penumbra PRs:
commit
(done in cnidarium: implement deferred commits viaStagedWriteBatch
penumbra-zone/penumbra#4122)Background
this was inevitable, also it cleans up the block execution flow inside the sequencer application. (removed
processed_txs
andcurrent_sequencer_block_builder
from the app)Changes
finalize_block
; for the most part, I left thebegin_block
/deliver_tx
/end_block
as they are but called them insidefinalize_block
SequencerBlock
at the end offinalize_block
without needing to track things between callsTesting
unit tests
ran it with cometbft release v0.38.6
Breaking Changelist
Related Issues
closes #679