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.

Sign up for GitHub

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

Jump to bottom

compact block: add epoch index to compact block #4100

Merged
merged 1 commit into from
Mar 26, 2024
Merged

compact block: add epoch index to compact block #4100

merged 1 commit into from
Mar 26, 2024

Conversation

aubrika
Copy link
Contributor

@aubrika aubrika commented Mar 25, 2024

closes #2689

@aubrika aubrika force-pushed the 2689-epoch-index-in-compact-block branch from a582c22 to fda91ee Compare March 25, 2024 22:36
@conorsch conorsch added the consensus-breaking breaking change to execution of on-chain data label Mar 25, 2024
conorsch
conorsch reviewed Mar 25, 2024
View reviewed changes
proto/penumbra/penumbra/core/component/compact_block/v1/compact_block.proto
@@ -28,6 +28,8 @@ message CompactBlock {
bool app_parameters_updated = 9;
// Updated gas prices, if they have changed.
fee.v1.GasPrices gas_prices = 10;
// The epoch index
uint64 epoch_index = 11;
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be grand to have a more descriptive docstring here, incorporating some of the context from #2689, that speaks to what this value represents, as well as why and how it might be used. Unfortunately, I'm a bit at a loss to suggest specific phrasing.

Copy link
Contributor Author

@aubrika aubrika Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was also at a loss for what to say on this one ... maybe @hdevalence or perhaps @plaidfinch could shed some light on the "epoch index" for future readers of our documentation strings?

Copy link
Member

Choose a reason for hiding this comment

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

This is the counter that identifies/index each epoch similar to the height of a block

Copy link
Member

Choose a reason for hiding this comment

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

but for epochs

@aubrika aubrika added this to the Sprint 3 milestone Mar 26, 2024
@erwanor
Copy link
Member

erwanor commented Mar 26, 2024

@conorsch I am not sure that this is consensus breaking; I could be wrong, but CompactBlocks are stored in nonverifiable storage and, in terms of interfaces, the change looks forward compatible because it is just adding a field to the proto definitions

@aubrika aubrika merged commit 8f55a18 into main Mar 26, 2024
7 checks passed
@aubrika aubrika deleted the 2689-epoch-index-in-compact-block branch March 26, 2024 00:39
@erwanor erwanor added the protobuf-changes Makes changes to the protobuf definitions. label Mar 26, 2024
@cratelyn cratelyn assigned cratelyn and aubrika and unassigned cratelyn Mar 26, 2024
@erwanor erwanor removed the consensus-breaking breaking change to execution of on-chain data label Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@conorsch conorsch conorsch left review comments

@erwanor erwanor erwanor approved these changes

Assignees

@aubrika aubrika

Labels
protobuf-changes Makes changes to the protobuf definitions.
Projects
Penumbra
Status: Done
Milestone
Sprint 3
Development

Successfully merging this pull request may close these issues.

Include the epoch index in the CompactBlock as well as the block height.
4 participants
@aubrika @erwanor @conorsch @cratelyn