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

Slashing related e2e test improvements #461

Merged
merged 5 commits into from
Nov 15, 2022
Merged

Slashing related e2e test improvements #461

merged 5 commits into from
Nov 15, 2022

Conversation

shaspitz
Copy link
Contributor

@shaspitz shaspitz commented Nov 9, 2022 •
edited
Loading

Improves a couple slashing related e2e tests by:

  1. Removing duplicate test/setup code in favor of test cases and if-statements (TestSendSlashPacketDowntime and TestSendSlashPacketDoubleSign were 90% the same, now they are one test method)
  2. Removing a hack which previously reconstructed a VSC packet from local variables, and manually sent/received that packet using IBC testing functions
  3. Using providerSlashingKeeper.SlashFractionDowntime instead of hardcoding an expected slash fraction

The second point here is needed to properly tie #462 into existing e2e tests, the hack was faultily making e2e tests fail.

shaspitz
shaspitz commented Nov 9, 2022
View reviewed changes
tests/e2e/slashing.go
oldBlockTime := s.consumerCtx().BlockTime()
packetData := types.NewSlashPacketData(validator, valsetUpdateId, stakingtypes.DoubleSign)
// Relay the VSC packet to the consumer
relayAllCommittedPackets(s, s.providerChain, s.path, ccv.ProviderPortID, s.path.EndpointB.ChannelID, 1)
Copy link
Contributor Author

@shaspitz shaspitz Nov 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is the new way that we relay the VSC packet from provider to consumer. Note that this method is commonly used elsewhere, and is a more effective test compared to manual packet construction

Copy link
Contributor

Choose a reason for hiding this comment

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

Why relayAllCommittedPackets is needed for VSC packets and not the Slash packets here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question, after looking through the slashing file a bit, this method is specific to the relaying and provider handling for slash packets.

There are methods further in the file that test the actual logic around sending downtime and double signing slash packets from a consumer. If we implemented that logic into this test, we'd be able to use relayAllCommittedPackets. Instead this test just manually sends/relays slash packets.

We could consolidate all those tests into a single tests, but prob out of the scope for this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've also updated the comment for TestRelayAndApplySlashPacket to make this more clear

shaspitz
shaspitz commented Nov 9, 2022
View reviewed changes
tests/e2e/slashing.go
err = s.path.EndpointA.UpdateClient()
s.Require().NoError(err)

// reconstruct VSC packet
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the old way we sent the VSC packet from provider to consumer

shaspitz
shaspitz commented Nov 9, 2022
View reviewed changes
tests/e2e/slashing.go
}

oldBlockTime := s.consumerCtx().BlockTime()
slashFraction := int64(100)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was removed in favor of the slashing keeper's value used below

@shaspitz shaspitz requested a review from MSalopek November 9, 2022 22:54
danwt
danwt approved these changes Nov 11, 2022
View reviewed changes
Copy link
Contributor

@danwt danwt left a comment

Choose a reason for hiding this comment

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

Really nice work, the new test is very clear 👍

x/ccv/provider/keeper/keeper.go
@@ -830,6 +830,24 @@ func (k Keeper) SetVscSendTimestamp(
store.Set(types.VscSendingTimestampKey(chainID, vscID), timeBz)
}

// GetVscSendTimestamp returns a VSC send timestamp by chainID and vscID
//
// Note: This method is used only for testing.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

sainoe
sainoe approved these changes Nov 14, 2022
View reviewed changes
Copy link
Contributor

@sainoe sainoe left a comment

Choose a reason for hiding this comment

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

LGTM! Just a small comment.

tests/e2e/slashing.go
oldBlockTime := s.consumerCtx().BlockTime()
packetData := types.NewSlashPacketData(validator, valsetUpdateId, stakingtypes.DoubleSign)
// Relay the VSC packet to the consumer
relayAllCommittedPackets(s, s.providerChain, s.path, ccv.ProviderPortID, s.path.EndpointB.ChannelID, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why relayAllCommittedPackets is needed for VSC packets and not the Slash packets here ?

@shaspitz shaspitz merged commit df53566 into main Nov 15, 2022
@shaspitz shaspitz deleted the slash-e2e-improvements branch November 15, 2022 00:11
@shaspitz shaspitz restored the slash-e2e-improvements branch November 15, 2022 05:05
danwt pushed a commit that referenced this pull request Nov 16, 2022
Squashed commit of the following:
28b313a 
commit fb83bb7
Author: MSalopek <[email protected]>
Date:   Tue Nov 15 22:54:06 2022 +0100

    add multiple consumer chains in integration tests (#407)

commit 33137b3
Author: Shawn Marshall-Spitzbart <[email protected]>
Date:   Tue Nov 15 12:05:45 2022 -0800

    testutils refactors and cleanup (#472)

    * changes

    * mas

    * Update README.md

    * Update README.md

    * sorry for the friday night emails

    * Update instance_test.go

    * util naming

commit 0096317
Author: Daniel T <[email protected]>
Date:   Tue Nov 15 19:24:40 2022 +0000

    Adds Cryptographic Identity utility for working with various keys ect (#470)

    * Adds crypto.go Cryptographic Identity util

    * Changes mock to ibcmock

    Co-authored-by: Daniel <[email protected]>

commit b10e132
Author: Shawn Marshall-Spitzbart <[email protected]>
Date:   Mon Nov 14 16:32:31 2022 -0800

    circuit breaker params  (#444)

    * changes

    * Update params.go

commit df53566
Author: Shawn Marshall-Spitzbart <[email protected]>
Date:   Mon Nov 14 16:11:22 2022 -0800

    Slashing related e2e test improvements (#461)
danwt added a commit that referenced this pull request Nov 16, 2022
@danwt
Sync to main fb83bb7 - Squashed commit of the following: (#484)
f1cc4ee 
Squashed commit of the following:

commit fb83bb7
Author: MSalopek <[email protected]>
Date:   Tue Nov 15 22:54:06 2022 +0100

    add multiple consumer chains in integration tests (#407)

commit 33137b3
Author: Shawn Marshall-Spitzbart <[email protected]>
Date:   Tue Nov 15 12:05:45 2022 -0800

    testutils refactors and cleanup (#472)

    * changes

    * mas

    * Update README.md

    * Update README.md

    * sorry for the friday night emails

    * Update instance_test.go

    * util naming

commit 0096317
Author: Daniel T <[email protected]>
Date:   Tue Nov 15 19:24:40 2022 +0000

    Adds Cryptographic Identity utility for working with various keys ect (#470)

    * Adds crypto.go Cryptographic Identity util

    * Changes mock to ibcmock

    Co-authored-by: Daniel <[email protected]>

commit b10e132
Author: Shawn Marshall-Spitzbart <[email protected]>
Date:   Mon Nov 14 16:32:31 2022 -0800

    circuit breaker params  (#444)

    * changes

    * Update params.go

commit df53566
Author: Shawn Marshall-Spitzbart <[email protected]>
Date:   Mon Nov 14 16:11:22 2022 -0800

    Slashing related e2e test improvements (#461)

Co-authored-by: Daniel <[email protected]>
@danwt danwt deleted the slash-e2e-improvements branch November 21, 2022 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danwt danwt danwt approved these changes

@sainoe sainoe sainoe approved these changes

@jtremback jtremback Awaiting requested review from jtremback

@mpoke mpoke Awaiting requested review from mpoke

@MSalopek MSalopek Awaiting requested review from MSalopek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shaspitz @sainoe @danwt