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

fix: include genesis validators in gateway parent validators tracker #1166

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sanderpick
Copy link
Contributor

This PR is the result of trying to track down the cause of the following behavior:

  • Create a new subnet with min validators = 2
  • Create 3 validators and join
  • The first 2 validators will be considered "genesis validators", and the third one will be added to the power table (membership in the subnet gateway contract) via a top down staking change
  • The subnet receives and stores the validator change, then checkpointing applies those changes. The next power table excludes the first two validators here. We now have only one validator with power.
  • Since the genesis validators are not included in the subnet gateway's parent validator tracker, the new membership set by the top down finalizer excludes the genesis validators, ie, listActiveValidators() will be zero when applying this first change here.
  • Bottom up checkpoints fail because they are only signed by this one validator with power, but the subnet contract on the parent still thinks (correctly) that the total collateral is from three validators, ie, the quorum threshold is higher than what's included in the checkpoint. Checkpoint fails with WeightsSumLessThanThreshold
  • You can see this reflected in the power as known by cometbft by looking at the nodes' status endpoint (http://localhost:<comet_rpc_port>/status). Power is zero for the first two nodes.

I'm not sure if this fix is ideal, but it works. It just adds the genesis validators to the parent validator tracker before setting the initial membership in the gateway constructor.

@sanderpick sanderpick requested a review from a team as a code owner October 8, 2024 17:02
@sanderpick sanderpick changed the title gateway: include genesis validators in gateway parent validators tracker fix: include genesis validators in gateway parent validators tracker Oct 8, 2024
@@ -203,6 +203,11 @@ library LibValidatorSet {
validators.validators[validator].metadata = metadata;
}

/// @notice Set validator data using metadata from memory bytes
function setMetadataFromConstructor(ValidatorSet storage validators, address validator, bytes memory metadata) internal {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugly... we can only have bytes memory in the constructor.

@sanderpick
Copy link
Contributor Author

Hunch: This issue is also the reason that in federated mode, you have to always include all validators, instead of being able to add them one-by-one.

Copy link
Contributor

@cryptoAtwill cryptoAtwill left a comment

Choose a reason for hiding this comment

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

@sanderpick The fix is correct for staking, but need to call record federated for permission mode. But adding permission mode into the constructor is actually quite a big change propagating to fendermint too. Check out #1150 as well, some ideas are documented there too.

uint256 amount = params.genesisValidators[i].weight;
bytes memory metadata = params.genesisValidators[i].metadata;
LibValidatorSet.setMetadataFromConstructor(s.validatorsTracker.validators, addr, metadata);
LibValidatorSet.recordDeposit(s.validatorsTracker.validators, addr, amount);
Copy link
Contributor

Choose a reason for hiding this comment

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

this technically works for Staking backed power, but will not work for federated. Ideally it would call record federated based on the permission mode.

@raulk raulk linked an issue Oct 9, 2024 that may be closed by this pull request
@sanderpick
Copy link
Contributor Author

Ah, makes sense. Somehow I missed #1150. I'm happy to take a stab at passing the mode in the constructor here, but sounds like from slack convo you're picking this up. If so, do you want access to this branch or will you just recreate it? Nbd either way.

@cryptoAtwill
Copy link
Contributor

@sanderpick yeah, it's quite tedious to fix the constructor, quite some code needs to be updated in fendermint. @raulk mentioned yesterday that he will check this out to see if there are easier fixes. But I think for collateral based subnets, this is the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

Genesis validator set is not updated completely
2 participants