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

chore: Add integration for equivocation with CometMock #1192

Merged
merged 1 commit into from
Aug 10, 2023
Merged

chore: Add integration for equivocation with CometMock #1192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ test-e2e-short:
# run only happy path E2E tests with cometmock
# this set of traces does not test equivocation but it does check downtime
test-e2e-short-cometmock:
go run ./tests/e2e/... --short-happy-path --use-cometmock --use-gorelayer
go run ./tests/e2e/... --cometmock-happy-path --use-cometmock --use-gorelayer
Copy link
Contributor

Choose a reason for hiding this comment

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

I think all of this is great and very exciting.

However a small suggestion, maybe we refactor the main.go flag parsing so that --cometmock-happy-path uses go relayer by default so the flag does not need to be specified.

The flag could just be -cometmock-happy-path --use-cometmock.

This can also be done as part of a larger refactor, I'm just asking for opinions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason why it is not like this is that the gorelayer can be used without cometmock, too.
(just not CometMock without the Gorelayer)
I suggest leaving it as it, in the anticipation that one day CometMock can be used with hermes, too (and it does not seem too bad until then to have it as-is)

Lmk if this sounds convincing to you

Copy link
Contributor

Choose a reason for hiding this comment

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

To me it seems implicit that if you're specifying --cometmock-happy-path, both --use-cometmock and --use-gorelayer would default to true

Copy link
Contributor

@bermuell bermuell Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

pls note that PR #1191 is changing the way tests can be selected.
Keeping selection of tests and infra seperated regarding parameters is better from a usablity perspective (full flexibility with reduced set of option).
I'd be in favor in not renaming the 'test case' and rather not having 'cometmock' in the name as it leads to assumptions that infra/test setup is using cometmock which is not the case. Running --short-happy-path --use-cometmock (with or without implicit usage of --use-gorelayer) is clearer IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

assumptions that infra/test setup is using cometmock which is not the case. Running --short-happy-path --use-cometmock (with or without implicit usage of --use-gorelayer) is clearer IMO.

I think this is a good approach to take.


# run full E2E tests in sequence (including multiconsumer)
test-e2e-multi-consumer:
Expand Down
58 changes: 38 additions & 20 deletions tests/e2e/actions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1604,18 +1604,7 @@ func (tr TestRun) setValidatorDowntime(chain chainID, validator validatorID, dow

if tr.useCometmock {
// send set_signing_status either to down or up for validator
var validatorAddress string
if chain == chainID("provi") {
validatorAddress = tr.getValidatorKeyAddressFromString(tr.validatorConfigs[validator].privValidatorKey)
} else {
var valAddressString string
if tr.validatorConfigs[validator].useConsumerKey {
valAddressString = tr.validatorConfigs[validator].consumerPrivValidatorKey
} else {
valAddressString = tr.validatorConfigs[validator].privValidatorKey
}
validatorAddress = tr.getValidatorKeyAddressFromString(valAddressString)
}
validatorAddress := tr.GetValidatorAddress(chain, validator)

method := "set_signing_status"
params := fmt.Sprintf(`{"private_key_address":"%s","status":"%s"}`, validatorAddress, lastArg)
Expand Down Expand Up @@ -1648,6 +1637,22 @@ func (tr TestRun) setValidatorDowntime(chain chainID, validator validatorID, dow
}
}

func (tr TestRun) GetValidatorAddress(chain chainID, validator validatorID) string {
var validatorAddress string
if chain == chainID("provi") {
validatorAddress = tr.getValidatorKeyAddressFromString(tr.validatorConfigs[validator].privValidatorKey)
} else {
var valAddressString string
if tr.validatorConfigs[validator].useConsumerKey {
valAddressString = tr.validatorConfigs[validator].consumerPrivValidatorKey
} else {
valAddressString = tr.validatorConfigs[validator].privValidatorKey
}
validatorAddress = tr.getValidatorKeyAddressFromString(valAddressString)
}
return validatorAddress
}

type unjailValidatorAction struct {
provider chainID
validator validatorID
Expand Down Expand Up @@ -1795,15 +1800,28 @@ func (tr TestRun) invokeDoublesignSlash(
action doublesignSlashAction,
verbose bool,
) {
chainConfig := tr.chainConfigs[action.chain]
//#nosec G204 -- Bypass linter warning for spawning subprocess with cmd arguments.
bz, err := exec.Command("docker", "exec", tr.containerConfig.instanceName, "/bin/bash",
"/testnet-scripts/cause-doublesign.sh", chainConfig.binaryName, string(action.validator),
string(chainConfig.chainId), chainConfig.ipPrefix).CombinedOutput()
if err != nil {
log.Fatal(err, "\n", string(bz))
if !tr.useCometmock {
chainConfig := tr.chainConfigs[action.chain]
//#nosec G204 -- Bypass linter warning for spawning subprocess with cmd arguments.
bz, err := exec.Command("docker", "exec", tr.containerConfig.instanceName, "/bin/bash",
"/testnet-scripts/cause-doublesign.sh", chainConfig.binaryName, string(action.validator),
string(chainConfig.chainId), chainConfig.ipPrefix).CombinedOutput()
if err != nil {
log.Fatal(err, "\n", string(bz))
}
tr.waitBlocks("provi", 10, 2*time.Minute)
} else { // tr.useCometMock
validatorAddress := tr.GetValidatorAddress(action.chain, action.validator)

method := "cause_double_sign"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet!

Copy link
Contributor

Choose a reason for hiding this comment

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

Curious where the script lives that actually handles "cause_double_sign"?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's a cometmock RPC method, it does not live here. It's a feature of cometmock

params := fmt.Sprintf(`{"private_key_address":"%s"}`, validatorAddress)

address := tr.getQueryNodeRPCAddress(action.chain)

tr.curlJsonRPCRequest(method, params, address)
tr.waitBlocks(action.chain, 1, 10*time.Second)
return
}
tr.waitBlocks("provi", 10, 2*time.Minute)
}

type assignConsumerPubKeyAction struct {
Expand Down
11 changes: 5 additions & 6 deletions tests/e2e/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,11 @@ import (
)

var (
verbose = flag.Bool("verbose", false, "turn verbose logging on/off")
happyPathOnly = flag.Bool("happy-path-only", false, "run happy path tests only")
shortHappyPathOnly = flag.Bool("short-happy-path", false, `run abridged happy path tests only.
verbose = flag.Bool("verbose", false, "turn verbose logging on/off")
happyPathOnly = flag.Bool("happy-path-only", false, "run happy path tests only")
cometmockCompatibleHappyPath = flag.Bool("cometmock-happy-path", false, `run cometmock compatible happy path tests only.
This is like the happy path, but skips steps
that involve starting or stopping nodes for the same chain outside of the chain setup or teardown.
In particular, this skips steps related to downtime and double signing.
This is suited for CometMock+Gorelayer testing`)
includeMultiConsumer = flag.Bool("include-multi-consumer", false, "include multiconsumer tests in run")
parallel = flag.Bool("parallel", false, "run all tests in parallel")
Expand All @@ -42,10 +41,10 @@ var (
func main() {
flag.Parse()

if shortHappyPathOnly != nil && *shortHappyPathOnly {
if cometmockCompatibleHappyPath != nil && *cometmockCompatibleHappyPath {
fmt.Println("=============== running short happy path only ===============")
tr := DefaultTestRun()
tr.Run(shortHappyPathSteps, *localSdkPath, *useGaia, *gaiaTag)
tr.Run(cometmockCompatibleHappyPathSteps, *localSdkPath, *useGaia, *gaiaTag)
return
}

Expand Down
4 changes: 3 additions & 1 deletion tests/e2e/steps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,14 @@ var happyPathSteps = concatSteps(
stepsStopChain("consu", 4), // stop chain
)

var shortHappyPathSteps = concatSteps(
var cometmockCompatibleHappyPathSteps = concatSteps(
stepsStartChains([]string{"consu"}, false),
stepsDelegate("consu"),
stepsUnbond("consu"),
stepsRedelegateShort("consu"),
stepsDowntime("consu"),
stepsRejectEquivocationProposal("consu", 2), // prop to tombstone bob is rejected
stepsDoubleSignOnProviderAndConsumer("consu"), // carol double signs on provider, bob double signs on consumer
stepsStartRelayer(),
stepsConsumerRemovalPropNotPassing("consu", 2), // submit removal prop but vote no on it - chain should stay
stepsStopChain("consu", 3), // stop chain
Expand Down