Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ACP-77: Write subnet public key diffs to state #3487

Merged
merged 19 commits into from
Oct 25, 2024

Conversation

StephenButtolph
Copy link
Contributor

@StephenButtolph StephenButtolph commented Oct 21, 2024

Why this should be merged

Factored out of #3388.

After ACP-77, L1 validators specify their own BLS public keys. This means that we can no longer assume that the public keys are inherited from a primary network validator.

How this works

Previously public key diffs were only written for primary network validators.
Now when a subnet validator is added or removed, a subnet specific key diff is written.
Additionally, non-nil public keys were only held in the validator set for the primary network validators. The state now exposes the public keys on all subnets.

How this was tested

  • Existing unit tests
  • Expanded state diff application to include testing of the new index
  • Expanded state diff application to include testing of the prior subnet validator key indexing lookup

@StephenButtolph StephenButtolph added this to the v1.11.13 milestone Oct 21, 2024
@StephenButtolph StephenButtolph self-assigned this Oct 21, 2024
@@ -271,7 +272,7 @@ func (m *manager) makePrimaryNetworkValidatorSet(
validatorSet,
currentHeight,
lastDiffHeight,
constants.PlatformChainID,
Copy link
Contributor Author

@StephenButtolph StephenButtolph Oct 22, 2024

Choose a reason for hiding this comment

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

This was the wrong constant before - but they happen to have the same value... So not an issue

@StephenButtolph StephenButtolph marked this pull request as draft October 22, 2024 11:17
@StephenButtolph StephenButtolph marked this pull request as ready for review October 23, 2024 15:46
Copy link
Contributor

@marun marun left a comment

Choose a reason for hiding this comment

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

Not the fault of this PR, but I found the testing changes quite painful to review. Ideally there would be comprehensive tests of low-level behavior and then higher-level testing could be less rigorous (only validating behavior not covered by the lower-level tests), but both of the tests that were modified seem a bit unfocused to me.

vms/platformvm/state/state.go Outdated Show resolved Hide resolved
vms/platformvm/state/state.go Show resolved Hide resolved
vms/platformvm/state/state_test.go Outdated Show resolved Hide resolved
vms/platformvm/state/state_test.go Outdated Show resolved Hide resolved
vms/platformvm/state/state_test.go Outdated Show resolved Hide resolved
vms/platformvm/state/state_test.go Outdated Show resolved Hide resolved
@StephenButtolph StephenButtolph changed the base branch from master to update-state-staker-tests October 24, 2024 14:40
@StephenButtolph
Copy link
Contributor Author

StephenButtolph commented Oct 24, 2024

@marun I refactored "unrelated" testing changes into a separate PR: #3487

Base automatically changed from update-state-staker-tests to master October 24, 2024 18:41
@StephenButtolph StephenButtolph added this pull request to the merge queue Oct 25, 2024
Merged via the queue into master with commit f212c3e Oct 25, 2024
23 checks passed
@StephenButtolph StephenButtolph deleted the populate-subnet-public-key-diffs branch October 25, 2024 15:50
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants