Remove public key deduplication migration and diff key program #8316
Conversation
The migration completed successfully, so we can remove the migration and the related program that validated the migration.
📝 WalkthroughWalkthroughThis PR removes the account public-key deduplication feature and the diff-keys CLI, shifts public-key validation and key-metadata handling to environment/accountkeymetadata, and exports an environment constant for stored digests. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Files:
{module,engine,cmd}/**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)cmd/util/cmd/check-storage/account_key_validation.go (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (37)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/util/cmd/check-storage/account_key_validation.go (1)
577-577: Update stale comment to reference current function name.The comment references
ValidateAccountPublicKeyV4(), which has been renamed tovalidateAccountPublicKey. Please update the comment for clarity.📝 Proposed fix
- // NOTE: don't need to check unreachable keys because it is checked in ValidateAccountPublicKeyV4(). + // NOTE: don't need to check unreachable keys because it is checked in validateAccountPublicKey().
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cmd/util/cmd/check-storage/account_key_validation.gocmd/util/cmd/check-storage/cmd.gocmd/util/cmd/diffkeys/cmd.gocmd/util/cmd/execution-state-extract/cmd.gocmd/util/cmd/root.gocmd/util/ledger/migrations/account_key_deduplication_encoder.gocmd/util/ledger/migrations/account_key_deduplication_encoder_test.gocmd/util/ledger/migrations/account_key_deduplication_migration.gocmd/util/ledger/migrations/account_key_deduplication_migration_results.gocmd/util/ledger/migrations/account_key_deduplication_migration_test.gocmd/util/ledger/migrations/account_key_deduplication_migration_utils.gocmd/util/ledger/migrations/account_key_diff.gocmd/util/ledger/migrations/account_key_diff_test.gofvm/environment/accounts_status.go
💤 Files with no reviewable changes (11)
- cmd/util/cmd/execution-state-extract/cmd.go
- cmd/util/ledger/migrations/account_key_deduplication_migration_test.go
- cmd/util/cmd/root.go
- cmd/util/ledger/migrations/account_key_deduplication_encoder_test.go
- cmd/util/ledger/migrations/account_key_deduplication_migration_results.go
- cmd/util/ledger/migrations/account_key_deduplication_migration.go
- cmd/util/ledger/migrations/account_key_deduplication_encoder.go
- cmd/util/ledger/migrations/account_key_diff.go
- cmd/util/ledger/migrations/account_key_deduplication_migration_utils.go
- cmd/util/ledger/migrations/account_key_diff_test.go
- cmd/util/cmd/diffkeys/cmd.go
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in @docs/agents/CodingConventions.md
Follow Go coding standards and conventions as documented in @docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
fvm/environment/accounts_status.gocmd/util/cmd/check-storage/cmd.gocmd/util/cmd/check-storage/account_key_validation.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/environment/accounts_status.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
fvm/environment/accounts_status.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/environment/accounts_status.go
{module,engine,cmd}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
All major processing components must implement the
Componentinterface from/module/component/component.goto ensure consistent lifecycle management and graceful shutdown patterns
Files:
cmd/util/cmd/check-storage/cmd.gocmd/util/cmd/check-storage/account_key_validation.go
🧠 Learnings (1)
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Applied to files:
cmd/util/cmd/check-storage/cmd.go
🧬 Code graph analysis (1)
fvm/environment/accounts_status.go (1)
fvm/environment/account-key-metadata/metadata.go (2)
NewKeyMetadataAppender(123-129)NewKeyMetadataAppenderFromBytes(133-155)
🔇 Additional comments (14)
fvm/environment/accounts_status.go (3)
60-62: LGTM! Constant exported for broader reuse.The constant is appropriately exported to support usage across packages after migration code consolidation. The value and purpose remain unchanged.
380-380: LGTM! Correct usage of exported constant.The call to
NewKeyMetadataAppendercorrectly uses the newly exportedMaxStoredDigestsconstant.
383-383: LGTM! Correct usage of exported constant.The call to
NewKeyMetadataAppenderFromBytescorrectly uses the newly exportedMaxStoredDigestsconstant.cmd/util/cmd/check-storage/account_key_validation.go (10)
13-13: LGTM! Import added to support consolidated key metadata handling.The import of
accountkeymetadatapackage is necessary for usingDecodeKeyMetadataand related types, aligning with the consolidation of key metadata logic.
17-19: LGTM! Clear constant for legacy register detection.The constant
legacyAccountPublicKeyRegisterKeyPrefixis well-named and appropriately scoped for checking legacy account public key registers.
21-24: LGTM! Function visibility change aligns with migration completion.The function is now unexported (
validateAccountPublicKey) as the public API is no longer needed after the migration completed. The removal of "V4" from the name suggests V4 is now the standard format.
31-31: LGTM! Call site correctly updated.The function call is correctly updated to match the renamed function
validateAccountStatusRegister.
93-101: LGTM! Function rename reflects V4 as standard format.The function is renamed from
validateAccountStatusV4RegistertovalidateAccountStatusRegister, removing the version designation as V4 is now the standard format.
139-142: LGTM! Logic consolidated into accountkeymetadata package.The use of
accountkeymetadata.DecodeKeyMetadataconsolidates key metadata parsing logic into the appropriate package, improving maintainability.
222-225: LGTM! Batch public key decoding consolidated.The use of
environment.DecodeBatchPublicKeysconsolidates decoding logic into the environment package, improving code organization.
253-255: LGTM! Constant correctly sourced from environment package.The use of
environment.MaxPublicKeyCountInBatchconsolidates the constant definition, ensuring consistency across the codebase.
268-268: LGTM! Type correctly sourced from accountkeymetadata package.The type
accountkeymetadata.WeightAndRevokedStatusis correctly used, consolidating type definitions in the appropriate package.
278-280: LGTM! Constant correctly sourced from environment package.The use of
environment.MaxStoredDigests(exported inaccounts_status.go) consolidates the constant definition for broader reuse.cmd/util/cmd/check-storage/cmd.go (1)
422-422: LGTM! Call site correctly updated to use local function.The call is correctly updated to use
validateAccountPublicKeyfrom the localcheck_storagepackage instead of the removedmigrations.ValidateAccountPublicKeyV4. This consolidates validation logic appropriately.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/util/cmd/check-storage/account_key_validation.go (1)
195-257: Address error handling violations in accordance with Flow's error handling guidelines.The code violates Flow's high-assurance error handling conventions. All errors returned by
validateBatchPublicKeyRegistersindicate data corruption or invalid storage state and should be treated as exceptions per the coding guidelines, not benign errors:
Use
irrecoverable.NewExceptionfinstead offmt.Errorf- Validation failures (missing keys, decode failures, count mismatches, data corruption) represent unexpected failures indicating potential state corruption. Per the CodingConventions, these must be wrapped as exceptions.Document error handling strategy - If you intend to use
fmt.Errorfas an exception, clearly document for each public function that all returned errors are exceptions and why. The optional simplification forfmt.Errorfonly applies to functions that solely return benign (expected) errors during normal operation. Validation failures are not benign.Apply context-dependent error classification - The guidelines emphasize that error classification depends on the caller's context. Storage validation failures are never expected during normal operation; they always indicate a critical failure.
Reference:
CodingConventions.mdsections on error classification and high-assurance principles, anddocs/agents/CodingConventions.mdon context-dependent error handling.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/util/cmd/check-storage/account_key_validation.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in @docs/agents/CodingConventions.md
Follow Go coding standards and conventions as documented in @docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
cmd/util/cmd/check-storage/account_key_validation.go
{module,engine,cmd}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
All major processing components must implement the
Componentinterface from/module/component/component.goto ensure consistent lifecycle management and graceful shutdown patterns
Files:
cmd/util/cmd/check-storage/account_key_validation.go
🧬 Code graph analysis (1)
cmd/util/cmd/check-storage/account_key_validation.go (5)
model/flow/ledger.go (1)
AccountPublicKey0RegisterKey(24-24)fvm/environment/account-key-metadata/metadata.go (1)
DecodeKeyMetadata(371-407)fvm/environment/account_public_key_util.go (2)
DecodeBatchPublicKeys(360-398)MaxPublicKeyCountInBatch(14-14)fvm/environment/account-key-metadata/weight_and_revoked_group.go (1)
WeightAndRevokedStatus(326-329)fvm/environment/accounts_status.go (1)
MaxStoredDigests(61-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (37)
- GitHub Check: Lint (./)
- GitHub Check: Lint (./integration/)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (4)
cmd/util/cmd/check-storage/account_key_validation.go (4)
1-15: LGTM!The new import for
accountkeymetadatais properly aliased and aligns with the refactoring to use centralized packages for key metadata handling.
17-85: LGTM!The function has been appropriately renamed and unexported, consistent with its utility-only usage. The use of
flow.AccountPublicKey0RegisterKeyconstant instead of an inline string improves maintainability. The removal of the legacy register check aligns with the PR objective of removing migration-related validation code.
87-163: LGTM!The function refactoring to use
accountkeymetadata.DecodeKeyMetadatais correct. The call signature matches the function definition in the relevant code snippets, and error handling remains consistent.
259-299: LGTM!The type reference
accountkeymetadata.WeightAndRevokedStatusand constantenvironment.MaxStoredDigestsare correctly used. Per the relevant snippets,MaxStoredDigests = 2is defined infvm/environment/accounts_status.go, which aligns with the validation logic here.
fxamacker
deleted the
fxamacker/remove-public-key-deduplication-migration
branch
Updates #8314 #7573
Account public key deduplication migration completed successfully, so we can remove that migration code and the diff key program used for validating the migration.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.