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

EIP-37 implementation and activation #1845

Merged
merged 25 commits into from
Sep 27, 2022
Merged

EIP-37 implementation and activation #1845

merged 25 commits into from
Sep 27, 2022

Conversation

kushti
Copy link
Member

@kushti kushti commented Sep 26, 2022

This PR contains implementation and activation of EIP-37 described in ergoplatform/eips#79.

Activation: it is possible to activate EIP-37 after block #843,776 and before block #851,969 . For activation, 232 or more votes for activation required in last 256 blocks, with voting checked every 128 blocks (for blocks which height % 128 == 1), and immediate activation once threshold is met.

Also, in this PR:

  • offlineGeneration is now true by default for the mainnet (which is what is needed by the pools all the time)

Copy link
Member

@greenhat greenhat left a comment

Choose a reason for hiding this comment

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

Looking good! Please check my comments.
Also, I did not find any tests for the new code.

kushti and others added 3 commits September 27, 2022 15:19
Co-authored-by: Alexander Slesarenko <aslesarenko@users.noreply.github.com>
…justment.scala

Co-authored-by: Alexander Slesarenko <aslesarenko@users.noreply.github.com>
@kushti
Copy link
Member Author

kushti commented Sep 27, 2022

Looking good! Please check my comments. Also, I did not find any tests for the new code.

See above on testing. Unfortunately, automated tests would be tricky to do with our testing framework, will take a look how to do them.

Copy link
Member

@greenhat greenhat left a comment

Choose a reason for hiding this comment

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

One question for diff lower bound.
Regarding the tests for new code, maybe just test eip37Calculate?

val limitedPredictiveDiff = if (predictiveDiff > lastDiff) {
predictiveDiff.min(lastDiff * 3 / 2)
} else {
predictiveDiff.max(lastDiff * 2)
Copy link
Member

Choose a reason for hiding this comment

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

Should it be lastDiff / 2? The same goes for uncompressedDiff.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, my bad, fixed.

For tests, thinking to add some test vectors. Anything better we can do?

Copy link
Member

Choose a reason for hiding this comment

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

PBT might be a good fit here.

@kushti kushti merged commit 3e5b5c0 into master Sep 27, 2022
@kushti kushti deleted the eip37 branch September 27, 2022 18:39
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