feat!: introduce epochs #1660

insumity · 2024-02-23T14:20:09Z

Description

This PR introduces epochs. The high-level idea behind the implementation is that we keep a slice of validators ([]ConsumerValidator) that are currently validating in an epoch. Each ConsumerValidator contains the power the validator had at the beginning of the epoch on the consumer chain, as well as the consumer public key the validator had set at the beginning of the epoch. At the boundaries of an epoch, we compute the ValidatorUpdates we need to send to a consumer chain using the DiffValidators function. DiffValidators can compute the updates by checking if the power or/and the consumer public key have been modified since the last epoch.

In this implementation, an epoch consists of a number of blocks and is not utilizing the time elapsed between blocks (i.e., based on BFT time). The benefit of this approach is potential simplicity because we do not have to consider 2 different thresholds (e.g., see here on a potential issue of expiring evidence when using both blocks and time). A potential drawback of our approach is that in cases the chain is down for multiple hours or the blocks need more time to be decided upon, then the epochs would extend in time. As a result VSCMaturedPackets might be delayed that could lead to unbonding pausing.

This PR slightly modified the majority of integration and E2E tests because those tests usually where advancing the provider chain by one block to verify that VSCPackets were being sent. Because now we have epochs, one block advancements are not enough, and we have to advance by an epoch to verify that VSCPackets are being sent. Additionally note that we increased the signed_blocks_window parameter of consumer clients from 15 to 20, while min_signed_per_window remains 0.5, and correspondingly we increased invokeDowntimeSlash to wait 11 (> 20 * 0.5) blocks instead of 10 to guarantee that a slash action would be triggered.

For a reviewer, the main focus should be the code in validator_set_update.go file and specifically the DiffValidators function and the ComputeNextEpochValidators method and how those are used in relay.go. Also, another important component is on how the consumer genesis is created in proposal.go.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! the type prefix if the change is state-machine breaking
confirmed this PR does not introduce changes requiring state migrations, OR confirmed migration code has been added to consumer and/or provider modules
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage

…port #1570) (#1577) fix!: Validation of SlashAcks fails due to marshaling to Bech32 (#1570) * add different Bech32Prefix for consumer and provider * separate app encoding and params * remove ConsumerValPubKey from ValidatorConfig * update addresses in tests * make SlashAcks consistent across chains * add comments for clarity * Regenerate traces * Fix argument order * set bech32prefix for provider to cosmos * add changelog entries * add consumer-double-downtime e2e test * update nightly-e2e workflow * fix typo * add consumer-double-downtime to testConfigs * remove changes on provider * skip invalid SlashAcks * seal the config * clear the outstanding downtime flag for new vals * add info on upgrading to v4.0.0 * fix upgrade handler * fix changeover e2e test * Update tests/e2e/config.go Co-authored-by: Philip Offtermatt <[email protected]> * Update tests/e2e/config.go Co-authored-by: Philip Offtermatt <[email protected]> * add AccountPrefix to ChainConfig * fix docstrings * update AccountAddressPrefix in app.go * fix consumer-misb e2e test --------- Co-authored-by: Philip Offtermatt <[email protected]> Co-authored-by: Simon Noetzlin <[email protected]> Co-authored-by: Philip Offtermatt <[email protected]> (cherry picked from commit 8604692) Co-authored-by: Marius Poke <[email protected]>

update changelog

* unclog build * update release notes * update release date

mpoke · 2024-02-28T13:34:05Z

x/ccv/provider/keeper/validator_set_update.go

+		if !found {
+			k.Logger(ctx).Error("could not retrieve public key for validator (%+v)", val)
+			// if no consumer key assigned then use the validator's key itself
+			nextConsumerPublicKey, err = val.TmConsPublicKey()


Are you sure that this assignment replaces the value of nextConsumerPublicKey outside this if block?

Yes. This is actually tested in TestComputeNextEpochValidators and if we were to remove this assignment, the test would fail. This happens because the test also checks against a validator that hasn't set a consumer key.

x/ccv/provider/keeper/validator_set_update.go

insumity · 2024-02-26T15:08:39Z

tests/e2e/actions.go

@@ -1307,6 +1307,8 @@ func (tr TestConfig) relayPacketsGorelayer(
 	target ExecutionTarget,
 	verbose bool,
 ) {
+	tr.waitBlocks(action.ChainA, 3, 90*time.Second)


Later calls to waitBlocks for 1 block have a timeout of 30 seconds, so for 3 blocks I'm adding a 90 seconds time out here.

Why do we have to wait for 3 blocks when the epoch param is set to 2 blocks?

Good catch. Changed blocks_per_epoch to 3.

x/ccv/provider/keeper/validator_set_update.go

+		iterator.Value()
+		var validator types.ConsumerValidator
+		if err := validator.Unmarshal(iterator.Value()); err != nil {
+			panic(fmt.Errorf("failed to unmarshal ConsumerValidator: %w", err))


tests/integration/common.go

x/ccv/provider/keeper/validator_set_update_test.go

This reverts commit 85a52b7.

jtremback · 2024-03-05T15:47:05Z

x/ccv/provider/types/params.go

+
+	// MaxBlocksPerEpoch defines the maximum blocks that constitute an epoch. Assuming we need 6 seconds per block,
+	// the maximum epoch corresponds to 2 hours (6 * 1200 = 7200 seconds).
+	MaxBlocksPerEpoch = 1200


Don't do this. Governance can decide whatever it wants

Agreed. Why force someone to update this if e.g. in the future block times are decreased to 0.1s? Seems unnecessary to have

jtremback

Overall architecture LGTM!

x/ccv/provider/keeper/validator_set_update.go

p-offtermatt · 2024-03-06T09:36:23Z

x/ccv/provider/types/params.go

+
+	// MaxBlocksPerEpoch defines the maximum blocks that constitute an epoch. Assuming we need 6 seconds per block,
+	// the maximum epoch corresponds to 2 hours (6 * 1200 = 7200 seconds).
+	MaxBlocksPerEpoch = 1200


Agreed. Why force someone to update this if e.g. in the future block times are decreased to 0.1s? Seems unnecessary to have

p-offtermatt

LGTM. a few comments. The only important thing is getting rid of maxBlocksPerEpoch, imo

* initial commit * removed KeyAssignmentReplacementsKey * refactor: simplify key-assignment logic (#1684) * fixed typo: depreciated to deprecated --------- Co-authored-by: Marius Poke <[email protected]>

* cleanup ./changelog entries * rebase * fix!: Validation of SlashAcks fails due to marshaling to Bech32 (backport #1570) (#1577) fix!: Validation of SlashAcks fails due to marshaling to Bech32 (#1570) * add different Bech32Prefix for consumer and provider * separate app encoding and params * remove ConsumerValPubKey from ValidatorConfig * update addresses in tests * make SlashAcks consistent across chains * add comments for clarity * Regenerate traces * Fix argument order * set bech32prefix for provider to cosmos * add changelog entries * add consumer-double-downtime e2e test * update nightly-e2e workflow * fix typo * add consumer-double-downtime to testConfigs * remove changes on provider * skip invalid SlashAcks * seal the config * clear the outstanding downtime flag for new vals * add info on upgrading to v4.0.0 * fix upgrade handler * fix changeover e2e test * Update tests/e2e/config.go Co-authored-by: Philip Offtermatt <[email protected]> * Update tests/e2e/config.go Co-authored-by: Philip Offtermatt <[email protected]> * add AccountPrefix to ChainConfig * fix docstrings * update AccountAddressPrefix in app.go * fix consumer-misb e2e test --------- Co-authored-by: Philip Offtermatt <[email protected]> Co-authored-by: Simon Noetzlin <[email protected]> Co-authored-by: Philip Offtermatt <[email protected]> (cherry picked from commit 8604692) Co-authored-by: Marius Poke <[email protected]> * docs: update changelog for v4.0.0 (#1578) update changelog * docs: prepare for v4.0.0 (#1581) * unclog build * update release notes * update release date * added proto declaration * temp commit * temp commit * more changes * first commit * add param and fix tests * reduce epoch size for e2e * clean up * mbt fix * fix diff bug * cleaning up * cleaning up * cleaning up * cleaning up * cleaning up * cleaning up * added more tests * more fixes * nit fixes * cleaning up * increase downtime by one block * fix logs * took into account Marius' comments * tiny fixes * Update x/ccv/provider/keeper/params.go Co-authored-by: Simon Noetzlin <[email protected]> * use Bech32 addresses as keys for maps * refactor nextBlocks(epoch) to nextEpoch * fixed comment * Remove new block creation during consumer chain setup * Revert "Remove new block creation during consumer chain setup" This reverts commit 85a52b7. * added simple param test * added upper bound and addressed a comment * Add another edge case for diffing * used smarted solution (based on Philip's comment) for diffing validators * refactor!: remove key-assignment replacements (#1672) * initial commit * removed KeyAssignmentReplacementsKey * refactor: simplify key-assignment logic (#1684) * fixed typo: depreciated to deprecated --------- Co-authored-by: Marius Poke <[email protected]> * add the epoch param in the docs --------- Co-authored-by: mpoke <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Simon Noetzlin <[email protected]> Co-authored-by: Philip Offtermatt <[email protected]>

* docs: modify epochs ADR to capture latest design (#1668) * modified ADR to capture the epoch design * feat!: introduce epochs (#1660) * cleanup ./changelog entries * rebase * fix!: Validation of SlashAcks fails due to marshaling to Bech32 (backport #1570) (#1577) fix!: Validation of SlashAcks fails due to marshaling to Bech32 (#1570) * add different Bech32Prefix for consumer and provider * separate app encoding and params * remove ConsumerValPubKey from ValidatorConfig * update addresses in tests * make SlashAcks consistent across chains * add comments for clarity * Regenerate traces * Fix argument order * set bech32prefix for provider to cosmos * add changelog entries * add consumer-double-downtime e2e test * update nightly-e2e workflow * fix typo * add consumer-double-downtime to testConfigs * remove changes on provider * skip invalid SlashAcks * seal the config * clear the outstanding downtime flag for new vals * add info on upgrading to v4.0.0 * fix upgrade handler * fix changeover e2e test * Update tests/e2e/config.go Co-authored-by: Philip Offtermatt <[email protected]> * Update tests/e2e/config.go Co-authored-by: Philip Offtermatt <[email protected]> * add AccountPrefix to ChainConfig * fix docstrings * update AccountAddressPrefix in app.go * fix consumer-misb e2e test --------- Co-authored-by: Philip Offtermatt <[email protected]> Co-authored-by: Simon Noetzlin <[email protected]> Co-authored-by: Philip Offtermatt <[email protected]> (cherry picked from commit 8604692) Co-authored-by: Marius Poke <[email protected]> * docs: update changelog for v4.0.0 (#1578) update changelog * docs: prepare for v4.0.0 (#1581) * unclog build * update release notes * update release date * added proto declaration * temp commit * temp commit * more changes * first commit * add param and fix tests * reduce epoch size for e2e * clean up * mbt fix * fix diff bug * cleaning up * cleaning up * cleaning up * cleaning up * cleaning up * cleaning up * added more tests * more fixes * nit fixes * cleaning up * increase downtime by one block * fix logs * took into account Marius' comments * tiny fixes * Update x/ccv/provider/keeper/params.go Co-authored-by: Simon Noetzlin <[email protected]> * use Bech32 addresses as keys for maps * refactor nextBlocks(epoch) to nextEpoch * fixed comment * Remove new block creation during consumer chain setup * Revert "Remove new block creation during consumer chain setup" This reverts commit 85a52b7. * added simple param test * added upper bound and addressed a comment * Add another edge case for diffing * used smarted solution (based on Philip's comment) for diffing validators * refactor!: remove key-assignment replacements (#1672) * initial commit * removed KeyAssignmentReplacementsKey * refactor: simplify key-assignment logic (#1684) * fixed typo: depreciated to deprecated --------- Co-authored-by: Marius Poke <[email protected]> * add the epoch param in the docs --------- Co-authored-by: mpoke <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Simon Noetzlin <[email protected]> Co-authored-by: Philip Offtermatt <[email protected]> * test: Add epochs to MBT (#1676) * cleanup ./changelog entries * rebase * fix!: Validation of SlashAcks fails due to marshaling to Bech32 (backport #1570) (#1577) fix!: Validation of SlashAcks fails due to marshaling to Bech32 (#1570) * add different Bech32Prefix for consumer and provider * separate app encoding and params * remove ConsumerValPubKey from ValidatorConfig * update addresses in tests * make SlashAcks consistent across chains * add comments for clarity * Regenerate traces * Fix argument order * set bech32prefix for provider to cosmos * add changelog entries * add consumer-double-downtime e2e test * update nightly-e2e workflow * fix typo * add consumer-double-downtime to testConfigs * remove changes on provider * skip invalid SlashAcks * seal the config * clear the outstanding downtime flag for new vals * add info on upgrading to v4.0.0 * fix upgrade handler * fix changeover e2e test * Update tests/e2e/config.go Co-authored-by: Philip Offtermatt <[email protected]> * Update tests/e2e/config.go Co-authored-by: Philip Offtermatt <[email protected]> * add AccountPrefix to ChainConfig * fix docstrings * update AccountAddressPrefix in app.go * fix consumer-misb e2e test --------- Co-authored-by: Philip Offtermatt <[email protected]> Co-authored-by: Simon Noetzlin <[email protected]> Co-authored-by: Philip Offtermatt <[email protected]> (cherry picked from commit 8604692) Co-authored-by: Marius Poke <[email protected]> * docs: update changelog for v4.0.0 (#1578) update changelog * docs: prepare for v4.0.0 (#1581) * unclog build * update release notes * update release date * added proto declaration * temp commit * temp commit * more changes * first commit * add param and fix tests * reduce epoch size for e2e * clean up * mbt fix * fix diff bug * cleaning up * cleaning up * cleaning up * cleaning up * cleaning up * cleaning up * added more tests * more fixes * nit fixes * cleaning up * increase downtime by one block * fix logs * took into account Marius' comments * tiny fixes * Update x/ccv/provider/keeper/params.go Co-authored-by: Simon Noetzlin <[email protected]> * use Bech32 addresses as keys for maps * refactor nextBlocks(epoch) to nextEpoch * Start adding epochs * Adjust tests for epochs * Use invariant script instead of handwriting Makefile * Fix key assignment valset invariant * Add better run_invariants script * Start adding epochs from trace into driver * Remove new block creation during consumer chain setup * Adjust model for epochs * Take into account comments * Revert changes to actions.go * Revert changes to x/ * Remove unused listMul * Advance time by epochLength instead of 1 second * Indent condition and clarify EndProviderEpoch --------- Co-authored-by: mpoke <[email protected]> Co-authored-by: insumity <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Simon Noetzlin <[email protected]> * added changelogs * rebase and fix compatibility test * Update docs/docs/adrs/adr-014-epochs.md Co-authored-by: Philip Offtermatt <[email protected]> * Update docs/docs/adrs/adr-014-epochs.md Co-authored-by: Philip Offtermatt <[email protected]> * nit change in test * removed blocks per epoch upper limit --------- Co-authored-by: mpoke <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Simon Noetzlin <[email protected]> Co-authored-by: Philip Offtermatt <[email protected]> Co-authored-by: Philip Offtermatt <[email protected]>

mpoke and others added 13 commits February 23, 2024 15:11

cleanup ./changelog entries

01daa3c

rebase

1ab7c20

docs: update changelog for v4.0.0 (#1578)

a5c1d4e

update changelog

docs: prepare for v4.0.0 (#1581)

c03587c

* unclog build * update release notes * update release date

added proto declaration

6c39a2f

temp commit

ddae681

temp commit

c9b1b3e

more changes

e0861e2

first commit

d6cd207

add param and fix tests

d144c39

reduce epoch size for e2e

44d13cd

clean up

076ec8b

github-actions bot added C:Testing Assigned automatically by the PR labeler C:x/provider Assigned automatically by the PR labeler C:x/types Assigned automatically by the PR labeler labels Feb 23, 2024

github-advanced-security bot found potential problems Feb 23, 2024

View reviewed changes

insumity added 2 commits February 23, 2024 15:34

mbt fix

a56b6e0

fix diff bug

36afb40

github-advanced-security bot found potential problems Feb 26, 2024

View reviewed changes

x/ccv/provider/keeper/validator_set_update.go Fixed Show fixed Hide fixed

insumity changed the title ~~Insumity/1650~~ feat!: introduce epochs Feb 26, 2024

insumity added 4 commits February 26, 2024 15:57

cleaning up

481fd88

cleaning up

35385dd

cleaning up

27290f6

cleaning up

c707c54

insumity commented Feb 26, 2024

View reviewed changes

cleaning up

cf8bdb8

github-advanced-security bot found potential problems Feb 27, 2024

View reviewed changes

x/ccv/provider/keeper/validator_set_update.go Fixed Show fixed Hide fixed

x/ccv/provider/keeper/validator_set_update.go Fixed Show fixed Hide fixed

x/ccv/provider/keeper/validator_set_update.go Fixed Show fixed Hide fixed

insumity added 2 commits February 27, 2024 13:29

cleaning up

ead0873

added more tests

88081cd

github-advanced-security bot found potential problems Feb 29, 2024

View reviewed changes

insumity self-assigned this Feb 29, 2024

mpoke linked an issue Feb 29, 2024 that may be closed by this pull request

Epochs: Introduce the last-epoch validator set #1650

Closed

sainoe reviewed Mar 1, 2024

View reviewed changes

tests/integration/common.go Outdated Show resolved Hide resolved

refactor nextBlocks(epoch) to nextEpoch

914840c

sainoe reviewed Mar 1, 2024

View reviewed changes

x/ccv/provider/keeper/validator_set_update_test.go Outdated Show resolved Hide resolved

sainoe reviewed Mar 1, 2024

View reviewed changes

x/ccv/provider/keeper/validator_set_update_test.go Outdated Show resolved Hide resolved

insumity and others added 5 commits March 1, 2024 16:30

fixed comment

791e582

Remove new block creation during consumer chain setup

85a52b7

Revert "Remove new block creation during consumer chain setup"

f909e1a

This reverts commit 85a52b7.

added simple param test

d683b5a

added upper bound and addressed a comment

44b9eb1

jtremback reviewed Mar 5, 2024

View reviewed changes

jtremback approved these changes Mar 5, 2024

View reviewed changes

Add another edge case for diffing

538eee3

p-offtermatt approved these changes Mar 6, 2024

View reviewed changes

insumity and others added 2 commits March 6, 2024 15:15

used smarted solution (based on Philip's comment) for diffing validators

294aacb

refactor!: remove key-assignment replacements (#1672)

31b7416

* initial commit * removed KeyAssignmentReplacementsKey * refactor: simplify key-assignment logic (#1684) * fixed typo: depreciated to deprecated --------- Co-authored-by: Marius Poke <[email protected]>

github-actions bot added C:x/consumer Assigned automatically by the PR labeler C:Docs Assigned automatically by the PR labeler C:ADR Assigned automatically by the PR labeler labels Mar 7, 2024

add the epoch param in the docs

ba2ed34

insumity merged commit ff578da into feat/epochs Mar 7, 2024
19 checks passed

insumity deleted the insumity/1650 branch March 7, 2024 15:19

insumity mentioned this pull request Mar 11, 2024

feat!: introduce epochs #1691

Merged

21 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: introduce epochs #1660

feat!: introduce epochs #1660

insumity commented Feb 23, 2024 •

edited

Loading

mpoke Feb 28, 2024

insumity Feb 28, 2024

insumity Feb 26, 2024

sainoe Mar 1, 2024

insumity Mar 4, 2024

jtremback Mar 5, 2024 •

edited

Loading

p-offtermatt Mar 6, 2024

jtremback left a comment

p-offtermatt Mar 6, 2024

p-offtermatt left a comment

feat!: introduce epochs #1660

feat!: introduce epochs #1660

Conversation

insumity commented Feb 23, 2024 • edited Loading

Description

Author Checklist

Reviewers Checklist

mpoke Feb 28, 2024

Choose a reason for hiding this comment

insumity Feb 28, 2024

Choose a reason for hiding this comment

insumity Feb 26, 2024

Choose a reason for hiding this comment

sainoe Mar 1, 2024

Choose a reason for hiding this comment

insumity Mar 4, 2024

Choose a reason for hiding this comment

jtremback Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

p-offtermatt Mar 6, 2024

Choose a reason for hiding this comment

jtremback left a comment

Choose a reason for hiding this comment

p-offtermatt Mar 6, 2024

Choose a reason for hiding this comment

p-offtermatt left a comment

Choose a reason for hiding this comment

insumity commented Feb 23, 2024 •

edited

Loading

jtremback Mar 5, 2024 •

edited

Loading