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: max-rate bellow the era min-rate #4552

Merged
merged 7 commits into from
Nov 29, 2023
Merged

fix: max-rate bellow the era min-rate #4552

merged 7 commits into from
Nov 29, 2023

Conversation

diego1q2w
Copy link
Contributor

Issue

Following the discovery in issue #4549 on the Harmony repository, there is an operational inconsistency that occurs before a block is finalized. During the process where the validator data stored in memory, are written to disk as defined in statedb.go lines 896-898, a sanity check is performed. This check executes a series of rules to ensure the integrity of the data before it's committed to disk. One particular rule, which can be found in validator.go lines 315-320, verifies that the rate is not exceeding the maximum allowed rate. If the rate is greater than the maximum, the write operation fails.

This issue became prominent after the HIP30 hard fork, which increased the minimum rate to 7%. Subsequently, validators with a 5% maximum rate were unable to adjust their fees accordingly due to the aforementioned validation rule.

In this PR, we address the issue by adjusting the max-rate to be at least equal to the sum of the minRate and the max-increase-rate. This change requires a hard fork, as it necessitates consensus across the majority of the nodes to ensure that all participants agree on the same value.

@sophoah
Copy link
Contributor

sophoah commented Nov 3, 2023

can you test it on localnet with external validator created with a max rat of 5% ?

@diego1q2w
Copy link
Contributor Author

diego1q2w commented Nov 3, 2023

can you test it on localnet with external validator created with a max rat of 5% ?

yup! will do

@@ -267,3 +267,27 @@ func UpdateMinimumCommissionFee(
}
return false, nil
}

// UpdateMaxCommissionFee makes sure the max-rate is at least higher than the rate + max-rate-change.
func UpdateMaxCommissionFee(state *state.DB, addr common.Address, minRate numeric.Dec) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

bool value returns from function, but never used. Maybe use it in tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah good catch! i wanted to keep the same function signature as the UpdateMinCommissionFee but as you said if not used then we can remove it

@diego1q2w
Copy link
Contributor Author

can you test it on localnet with external validator created with a max rat of 5% ?

yup! will do

And it works! tested it two validators as you can see in the PR test script, one had the max-rate bellow the min and the other had the max-rate above, the latter got updated as soon as hip30 hit, while the former didn't (as expected) however when the MaxRate epoch hit the former got the update as well.

@ONECasey ONECasey merged commit f993468 into dev Nov 29, 2023
2 checks passed
Frozen added a commit that referenced this pull request Dec 1, 2023
ONECasey pushed a commit that referenced this pull request Dec 2, 2023
@diego1q2w diego1q2w deleted the fix/max-rate branch December 4, 2023 08:19
diego1q2w added a commit that referenced this pull request Dec 4, 2023
@diego1q2w diego1q2w restored the fix/max-rate branch December 4, 2023 08:21
Copy link

mergify bot commented Dec 4, 2023

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

Frozen pushed a commit that referenced this pull request Dec 4, 2023
* fix: max-rate bellow the era min-rate

* fix comments

* add localnet epoch config

* update config

* update config

* update config

* update config
Frozen added a commit that referenced this pull request Dec 4, 2023
Frozen added a commit that referenced this pull request Dec 5, 2023
* Removed outdated check.

* Fallback for old sync for BeaconBlockChannel.

* Additional logs.

* fix: max-rate bellow the era min-rate (#4552)

* fix: max-rate bellow the era min-rate

* fix comments

* add localnet epoch config

* update config

* update config

* update config

* update config

* Revert "fix: max-rate bellow the era min-rate (#4552)" (#4578)

This reverts commit f993468.

---------

Co-authored-by: Diego Nava <8563843+diego1q2w@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants