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 2 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
65 changes: 2 additions & 63 deletions x/qgb/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,11 @@ import (
"fmt"
"math/big"

"github.com/ethereum/go-ethereum/common"

stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"

"github.com/celestiaorg/celestia-app/x/qgb/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
"github.com/ethereum/go-ethereum/common"
)

type msgServer struct {
Expand Down Expand Up @@ -47,48 +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 if validator is part of the previous valset
var previousValset *types.Valset
// TODO add test for case nonce == 1.
if msg.Nonce == 1 {
// if the msg.Nonce == 1, the current valset should sign the first valset.
// Because, it's the first attestation, and there is no prior validator set defined
// that should sign this change.
// In fact, the first nonce should never be signed. Because, the first attestation, in the case
// where the `earliest` flag is specified when deploying the contract, will be relayed as part of
// the deployment of the QGB contract.
// It will be signed temporarily for now.
previousValset = valset
} else {
previousValset, err = k.GetLastValsetBeforeNonce(ctx, msg.Nonce)
if err != nil {
return nil, err
}
}
if !ValidatorPartOfValset(previousValset.Members, validator.EthAddress) {
return nil, sdkerrors.Wrap(
types.ErrValidatorNotInValset,
fmt.Sprintf("validator %s not part of valset %d", validator.Orchestrator, previousValset.Nonce),
)
}

// 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 @@ -187,34 +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 if validator is part of the previous valset
previousValset, err := k.GetLastValsetBeforeNonce(ctx, msg.Nonce)
if err != nil {
return nil, err
}
if !ValidatorPartOfValset(previousValset.Members, validator.EthAddress) {
return nil, sdkerrors.Wrap(
types.ErrValidatorNotInValset,
fmt.Sprintf("validator %s not part of valset %d", validator.Orchestrator, previousValset.Nonce),
)
}

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