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

QGB state machine remove invalid validator set check #646

Merged
merged 4 commits into from
Aug 23, 2022
Merged

QGB state machine remove invalid validator set check #646

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
59ad719
remove state machine confirm valset check
rach-id Aug 22, 2022
83d736d
remove invalid state machine confirm validator check
rach-id Aug 22, 2022
b1cdd52
Merge branch 'qgb-integration' into remove_invalid_validator_check_fr…
rach-id Aug 22, 2022
0fbb702
cosmetics
rach-id Aug 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 0 additions & 22 deletions x/qgb/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,23 +45,11 @@ func (k msgServer) ValsetConfirm(
return nil, sdkerrors.Wrap(types.ErrInvalid, "acc address invalid")
}

// Verify if validator exists
Copy link
Collaborator

Choose a reason for hiding this comment

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

[question][non-blocking] do any checks need to be removed from https://github.com/celestiaorg/celestia-app/blob/qgb-integration/docs/architecture/ADR-002-QGB-ValSet.md?plain=1#L158 ? I don't see these checks in that ADR section so maybe not but the contents of the ADR don't match 1:1 with the ValsetConfirm implementation here so wanted to confirm.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the ADRs are pretty oooold now. We will need to update them once we have a stable QGB 1.0. The reason we stopped updating them, is we were trying a lot of approaches, and we changed the design multiple times. Thus, it was too much work to keep updating them always.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense. If the ADR is no longer accurate then perhaps we can include a disclaimer at the top stating that it's unmaintained and create a GH issue to update it once we have a stable QGB 1.0? Defer to you on how best to track / update it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. We have few issues concerning this:
https://github.com/celestiaorg/celestia-app/issues/217
#501
#270

validator, found := k.StakingKeeper.GetValidatorByOrchestrator(ctx, orchaddr)
if !found {
return nil, sdkerrors.Wrap(types.ErrUnknown, "validator")
}
if err := sdk.VerifyAddressFormat(validator.GetOperator()); err != nil {
return nil, sdkerrors.Wrapf(err, "discovered invalid validator address for orchestrator %v", orchaddr)
}

// Verify ethereum address match
if !common.IsHexAddress(msg.EthAddress) {
return nil, sdkerrors.Wrap(stakingtypes.ErrEthAddressNotHex, "ethereum address")
}
submittedEthAddress := common.HexToAddress(msg.EthAddress)
if validator.EthAddress != submittedEthAddress.Hex() {
return nil, sdkerrors.Wrap(types.ErrInvalid, "signing eth address does not match delegate eth address")
}

// Verify if signature is correct
bytesSignature, err := hex.DecodeString(msg.Signature)
Expand Down Expand Up @@ -160,22 +148,12 @@ func (k msgServer) DataCommitmentConfirm(
if err != nil {
return nil, sdkerrors.Wrap(types.ErrInvalid, "validator address invalid")
}
validator, found := k.StakingKeeper.GetValidatorByOrchestrator(ctx, validatorAddress)
if !found {
return nil, sdkerrors.Wrap(types.ErrUnknown, "validator")
}
if err := sdk.VerifyAddressFormat(validator.GetOperator()); err != nil {
return nil, sdkerrors.Wrapf(err, "discovered invalid validator address for validator %v", validatorAddress)
}

// Verify ethereum address
if !common.IsHexAddress(msg.EthAddress) {
return nil, sdkerrors.Wrap(stakingtypes.ErrEthAddressNotHex, "ethereum address")
}
ethAddress := common.HexToAddress(msg.EthAddress)
if validator.EthAddress != ethAddress.Hex() {
return nil, sdkerrors.Wrap(types.ErrInvalid, "submitted eth address does not match delegate eth address")
}

// Verify signature
commitment, err := hex.DecodeString(msg.Commitment)
Expand Down