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

feat(sequencer): Add new validator update action #1679

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

ethanoroshiba
Copy link
Contributor

@ethanoroshiba ethanoroshiba commented Oct 17, 2024

Summary

Changed action ValidatorUpdate to include a name and moved it to our proprietary action types.

Background

Previously, validator names were not stored app-side, and names could only be set in CometBFT genesis.

Changes

  • Added name to ValidatorUpdate and moved it from astria_vendored to astria.
  • Restricted validator name to 32 characters when converting from on-wire format.
  • Added ValidatorNames struct, which contains address <> name key-value pairs.
  • Added storage keys, value impls, snapshots, and state read/write methods for ValidatorNames.
  • Added new ABCI query for validator name, accepting address as a parameter.
  • Edited ValidatorSet to use insert instead of push_update so that it doesn't imply any ordering.

Testing

  • Snapshot tests added for new VALIDATOR_NAMES storage key
  • Unit tests for new state read/write methods
  • Unit tests that new ABCI query works as expected
  • Unit test for ABCI query router to validator_name_request.
  • App execution test for ValidatorUpdate.

Related Issues

closes #1590

@github-actions github-actions bot added proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate labels Oct 17, 2024
@ethanoroshiba ethanoroshiba marked this pull request as ready for review October 25, 2024 14:27
crates/astria-sequencer/src/authority/query.rs Outdated Show resolved Hide resolved
crates/astria-sequencer/src/authority/mod.rs Outdated Show resolved Hide resolved
crates/astria-sequencer/src/authority/query.rs Outdated Show resolved Hide resolved
crates/astria-sequencer/src/authority/query.rs Outdated Show resolved Hide resolved
crates/astria-sequencer/src/authority/query.rs Outdated Show resolved Hide resolved
crates/astria-sequencer/src/authority/query.rs Outdated Show resolved Hide resolved
crates/astria-sequencer/src/authority/query.rs Outdated Show resolved Hide resolved
power,
name,
} = value;
if name.len() > MAX_VALIDATOR_NAME_LENGTH {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we explicitly check against chars(), or is 32 bytes fine?

},
Protobuf,
};

pub mod group;

const MAX_VALIDATOR_NAME_LENGTH: usize = 32;
Copy link
Member

Choose a reason for hiding this comment

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

Just a note that I thought this might be a bit short, but did some research and I don't think there is any validator in top 100 for celestia with a longer name, so probably fine.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking a bit outside the box on breaking changes here:

Technically speaking the ValidatorUpdateV2 action is not a wire breaking change. I think I said we should introduce this as a whole new action, and I still believe we should take ownership over it but fields 1 and 2 are the same. Should we instead just replace the type (will make buf mad I believe because I think I enforced higher level than this for the module than just wire.

But if we do that and don't add a whole new action, we could have this update be built into the oracle upgrade as well and just not do any database state on the name until the fork is triggered? This requires the upgrade logic to be added.

@ethanoroshiba
Copy link
Contributor Author

Noting that I'm holding off on further work on this until upgrade logic and DB changes are landed.

@SuperFluffy SuperFluffy added the blocked blocked on another PR, dependency, or missing feature label Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
allow-breaking-proto blocked blocked on another PR, dependency, or missing feature proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(sequencer): updates of validator names
5 participants