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.

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

fix: decrease minimums for genesis parameters #13106

Merged

Conversation

shaspitz
Copy link

@shaspitz shaspitz commented Aug 31, 2022

For use in cosmos/interchain-security#313 to speed up integration tests

@mergify
Copy link
Contributor

mergify bot commented Aug 31, 2022

⚠️ The sha of the head commit of this PR conflicts with #12830. Mergify cannot evaluate rules on this PR. ⚠️

@shaspitz shaspitz changed the base branch from main to interchain-security-rebase August 31, 2022 00:19
@shaspitz shaspitz changed the title Decrease genesis parameters Decrease minimums for genesis parameters Aug 31, 2022
@tac0turtle
Copy link
Member

You should be able to do this without modifying code. Using sed or wrapping the module would suffice

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Mhhh I don't want to change genesis validation just to make tests pass. I think there are other ways to tweak this in tests.

@shaspitz shaspitz closed this Aug 31, 2022
@shaspitz shaspitz deleted the decrease-downtime branch August 31, 2022 14:30
@shaspitz
Copy link
Author

This isn't a matter of tests passing or failing, it'd just speed up ICS integration tests. There are definitely alternatives to changing the hardcoded values within this PR, I'm just struggling to see the intuition behind the exact values that were chosen.

That being said, I'm happy to implement one of the alternatives mentioned. It seems there is importance to these constraint values

@alexanderbez
Copy link
Contributor

We can't say what strong motivation initially existed, but to change I'd say we need sufficient reason to do so, apart from making tests faster 👍

@jtremback
Copy link
Contributor

@jackzampolin wrote this code originally here 4b4a2b8

Jack, do you remember what the 1 minute minimum was? Can it be reduced?

@jtremback
Copy link
Contributor

jtremback commented Aug 31, 2022

In the commit above, a bunch of validations were added at once, and they look to be "sensible defaults". I would be surprised if there was a specific protocol reason for any of them to checked (beyond making sure that SignedBlocksWindow is positive).

	maxEvidence := data.Params.MaxEvidenceAge
	if maxEvidence < 1*time.Minute {
		return fmt.Errorf("Max evidence age must be at least 1 minute, is %s", maxEvidence.String())
	}

	dblSignUnbond := data.Params.DoubleSignUnbondDuration
	if dblSignUnbond < 1*time.Minute {
		return fmt.Errorf("Double sign unblond duration must be at least 1 minute, is %s", dblSignUnbond.String())
	}

	downtimeUnbond := data.Params.DowntimeUnbondDuration
	if downtimeUnbond < 1*time.Minute {
		return fmt.Errorf("Downtime unblond duration must be at least 1 minute, is %s", downtimeUnbond.String())
	}

	signedWindow := data.Params.SignedBlocksWindow
	if signedWindow < 10 {
		return fmt.Errorf("Signed blocks window must be at least 10, is %d", signedWindow)
	}

@alexanderbez
Copy link
Contributor

Mhhh I give in. This PR can be re-opened then -- sorry

@shaspitz shaspitz restored the decrease-downtime branch September 1, 2022 14:15
@shaspitz
Copy link
Author

shaspitz commented Sep 1, 2022

No worries, sounds good 🙏

@shaspitz shaspitz reopened this Sep 1, 2022
@shaspitz shaspitz changed the title Decrease minimums for genesis parameters fix: decrease minimums for genesis parameters Sep 1, 2022
@shaspitz shaspitz marked this pull request as ready for review September 1, 2022 14:18
@alexanderbez alexanderbez merged commit 06d4a64 into cosmos:interchain-security-rebase Sep 1, 2022
@shaspitz shaspitz deleted the decrease-downtime branch September 1, 2022 18:17
@jtremback
Copy link
Contributor

thanks @alexanderbez ! I also confirmed with Jack that it should be ok to change

jtremback added a commit that referenced this pull request Nov 1, 2022
* feat: store ABCI validator updates in transient store

* fix test build

* change transient key name

* add UnbondingDelegationEntryCreated hook

* add id to UnbondingDelegationEntry

* changes to add simple version of staking ccv hooks

* ubde id to string

* rough draft of more efficient technique

* change hook api and do some clean ups

* use ByEntry index and keep stopped entries in Entries array

* correct error convention

* add comment

* some cleanups

* comment cleanup

* finish hooking up hooks

* get the tests to pass

* proof of concept with embedded mock hooks

* first unit test of CCV hooks

* fix forgotten pointer bug

* move hook mocks into own file

* added test for completing stopped unbonding early

* added staking hooks template

* correct file and module names

* clean up and fix import error

* move staking hooks template to types

* fix hooks after merge

* fix silly proto bug

* feat: Add AfterValidatorDowntime hook in slashing module (#10938)

Create a slashing hook that allows external modules to register an execution when a validator has downtime.

Implementation details:

* Call hook in HandleValidatorSignature (x/slashing/keeper/infractions.go) which updates validator SignInfo data

* Defer hook execution in case it also wants to update validator SignInfo data

* Add methods to update SignInfo to slashing keeper interface(/x/slashing/types/expected_keepers.go)

* update: Remove slashing module hooks (#11425)

* update: Remove slashing module hooks

Hooks are not required anymore to implement the slashing for downtime in CCV. The logic is now using the staking keeper interface definition from the slashing module.

The SDK changes are the following:
- /x/slashing/keeper/infractions.go - remove hook calls and don't update validators missed blocks when jailed

- /x/slashing/types/expected_keepers.go - remove `AfterValidatorDowntime` hook interface and add `IsJailed()` method to staking interface definition

- /x/staking/keeper/validator.go - implement `IsJailed()` method

* fix last details

* Finish staking hooks merge (#11677)

* allow stopping and completing of redelegations

* refactor to remove BeforeUnbondingDelegationEntryComplete hook and notes for validator unbonding

* WIP rough draft of validator unbonding hooks

* add many of marius's suggested changes

* More review changes

* unbonding delegation tests pass

* WIP adding redelegation tests

* WIP redelegation tests work

* unbondingDelegation and redelegation tests pass and cleanup

* WIP validator unbonding tests almost pass

* tests for all new functionality pass

* fix index deleting logic

* clean up TODOs

* fix small logic bug

* fix slashing tests

* Rename statements containing 'UnbondingOp' to 'Unbond' in code, docs and proto files

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

* feat: enable double-signing evidence in Interchain-Security (#11921)

* Add a `InfractionType` enum to Slash function arguments

* Remove pubkey condition in HandleEquivocation

* Update docs/core/proto-docs.md

Co-authored-by: billy rennekamp <[email protected]>

* Update proto/cosmos/staking/v1beta1/staking.proto

Co-authored-by: billy rennekamp <[email protected]>

* add a possible solution to the evidence module issue

Co-authored-by: billy rennekamp <[email protected]>

* chore: remove direct reliance on staking from slashing (backport #12177) (#12181)

* fix: make ModuleAccountInvariants pass for IS SDK changes (#12554)

* fix bug breaking ModuleAccountInvariants

* set UnbondingOnHold to false explicitly

* Fixes staking hooks safety issues (#12578)

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

* Revert "fix: make ModuleAccountInvariants pass for IS SDK changes (#12554)" (#12782)

This reverts commit 67c8163.

* fix: make ModuleAccountInvariants pass for IS SDK changes (#12783)

* fix bug breaking ModuleAccountInvariants

* set UnbondingOnHold to false explicitly

* fix: [Interchain Security] `validatorUnbondingCanComplete` must take into account (re)bonded validators (#12796)

* replace val.UnbondingOnHold w/ UnbondingOnHoldRefCount

* add UnbondingOnHoldRefCount for undel and red (for consistency)

* improve comments

* improve TestValidatorUnbondingOnHold test

* ret error if UnbondingOnHoldRefCount is negative

* adding extra validator unbonding test

* change OnHold() def

* fix: [Interchain Security] Fix leak in unbonding hooks (#12849)

* remove leak for UBDEs and REDEs

* remove leak for val unbondings

* docs: [Interchain Security] update spec (#12848)

* updating staking spec

* clarify code

* fix typo

* store ValidatorUpdates in normal store (#12845)

* Update x/slashing/keeper/signing_info.go

Co-authored-by: Simon Noetzlin <[email protected]>

* Update x/staking/keeper/val_state_change.go

* Update x/staking/keeper/val_state_change.go

* Update x/slashing/keeper/infractions.go

Co-authored-by: Simon Noetzlin <[email protected]>

* Update x/staking/keeper/val_state_change.go

* Update x/staking/keeper/val_state_change.go

* fix compile errors

* remove stakingtypes.TStoreKey

* fix: decrease minimums for genesis parameters (#13106)

* Update genesis.go

* Update genesis.go

Co-authored-by: Federico Kunze <[email protected]>
Co-authored-by: Aditya Sripal <[email protected]>
Co-authored-by: Simon Noetzlin <[email protected]>
Co-authored-by: billy rennekamp <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Marius Poke <[email protected]>
Co-authored-by: Daniel T <[email protected]>
Co-authored-by: Shawn Marshall-Spitzbart <[email protected]>
sainoe pushed a commit that referenced this pull request Nov 2, 2022
* Update genesis.go

* Update genesis.go
sainoe pushed a commit that referenced this pull request Nov 3, 2022
* Update genesis.go

* Update genesis.go
sainoe pushed a commit that referenced this pull request Nov 16, 2022
* Update genesis.go

* Update genesis.go
sainoe pushed a commit that referenced this pull request Jan 26, 2023
* Update genesis.go

* Update genesis.go
sainoe pushed a commit that referenced this pull request Feb 9, 2023
* Update genesis.go

* Update genesis.go
julienrbrt pushed a commit that referenced this pull request Mar 23, 2023
* Update genesis.go

* Update genesis.go
@faddat faddat mentioned this pull request Mar 23, 2023
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants