CIP-21 Governable "lookbackWindow" parameter #1136
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mcortesi
previously approved these changes
Oct 21, 2020
13 tasks
mcortesi
changed the title
mcortesi
force-pushed
the
andres-dg/loopbackwindow-refactor
branch
from
30237db
to
3b341ce
Compare
mcortesi
force-pushed
the
andres-dg/loopbackwindow-refactor
branch
3 times, most recently
from
8ea43f1
to
657191b
Compare
mcortesi
force-pushed
the
andres-dg/loopbackwindow-refactor
branch
from
657191b
to
adfa925
Compare
mcortesi
requested review from
asaj,
timmoreton and
trianglesphere
as code owners
oneeman
approved these changes
Jan 15, 2021
prestwich
approved these changes
Jan 17, 2021
mcortesi
force-pushed
the
andres-dg/loopbackwindow-refactor
branch
from
3fcaef2
to
cc1f94b
Compare
oneeman
reviewed
Jan 19, 2021
consensus/istanbul/uptime/config.go
Outdated
| value, err := getLookbackWindow() | ||
| if err != nil { | ||
| value = MinSafeLookbackWindow | ||
| if !isDonut { |
There was a problem hiding this comment.
I usually prefer to flip the then/else to have if isDonut { instead, except when there is a reason not to. Having the shorter block first is nice, but not necessarily worth it.
oneeman
reviewed
Jan 19, 2021
consensus/istanbul/uptime/config.go
Outdated
| if !isDonut { | ||
| return defaultLookbackWindow | ||
| } | ||
| var returnValue uint64 |
There was a problem hiding this comment.
Why returnValue rather than value? Also, we could put in the signature (value uint64)
prestwich
reviewed
Jan 19, 2021
| } | ||
| return 0, err | ||
| } | ||
| return lookbackWindow.Uint64(), nil |
There was a problem hiding this comment.
this will have the side effect of truncating any 256-bit value to a go uint64?
There was a problem hiding this comment.
basically I'm double-checking that an invalid contract state won't cause a panic or something here
These flags are actually ignored lated in the code; and generate confussion. Hence, this commit deprecrated them and clearle states that they are ignored.
Governable lookback window
* Move logic to uptime model, for cohesiveness * Move istanbul config processing to istanbul module * Verify lookbackWindow is within safe boundaries * Verify EpochSize is at least MinEpochSize Nits & clean ups
# Conflicts: # consensus/istanbul/backend/engine.go
This commits also changes the behaviour pre-hardfork, but it it considered safe.
mcortesi
force-pushed
the
andres-dg/loopbackwindow-refactor
branch
from
b1e37d4
to
375f565
Compare
mcortesi
added a commit
to celo-org/celo-monorepo
that referenced
this pull request
Jan 20, 2021
Implementation of CIP-21 smart contract changes. celo-blockchain related PR: celo-org/celo-blockchain#1136 Adds the uptimeLookbackWindow to the BlockchainParameters contract. This is only ever used on the celo-blockchain client to determine the size of the uptime lookback window which is used to compute the validator uptimeScore An important invariant, is that UptimeLookbackWindow must remain constant during an epoch. Thus, changes to the parameter only take effect on the next epoch. There's an exception to this rule during migration/deployment of this change; there uptimeLookbackWindow should be initialized to chain's current value for the parameter which is part of the ChainConfig on the genesis block. To initialize the value: On a new testnet: As part of the migrations, BlockchainParamets.initialize() will do the initialization On an existing testnet: After upgrading the contract, first call to setLookbackWindow() will the value for the current epoch. Since BlockchainParameters won't be reinitialized when this change is deployed, a patch was made to the getUptimteLookbackWindow() contract to return 12 (mainnet, baklava & alfajores value for lookbackWindow) in the case of the value not being initalized. This patch can be removed in the future, since it's only necessary until setLoobackWindow() is called and first epoch since deployment has passed Co-authored-by: Mariano Cortesi <mariano@clabs.co>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Modifies celo-blockchain to obtain
lookbackWindowparameter from theBlockchainParameterssmart contract. It assumes the smart contract will return the same value during an epoch.lookbackWindowis used on 3 places:Blockchainwe obtain theloobackWindowasses the "upness" of each validator on that blockOther changes
Tested
Added unit tests.
Related issues
Backwards compatibility
Hardfork Change. Part of
DonutHardfork