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

Voting system #540

Merged
merged 281 commits into from
Jan 15, 2019
Merged

Voting system #540

merged 281 commits into from
Jan 15, 2019

Conversation

kushti
Copy link
Member

@kushti kushti commented Nov 6, 2018

Many parameters can be changed on-the-fly via miners voting, namely instruction costs, computational cost limit per block,
block size limit, storage fee factor, block version, and so on. Voting for the block version~(so for a soft-fork)
lasts for 32 epochs~(see epoch length below), and requires 90 percent of the miners to vote for the change.
For less critical changes~(such as block size limit), simple majority is enough. We will further refer to the changes
of the first kind as to foundational changes, we call the changes of the second kind as to everyday changes.
Per block, a miner can vote for two everyday changes and also one foundational change.

@kushti kushti added the S-wip Status: Work in progress label Nov 6, 2018
@kushti kushti removed the S-wip Status: Work in progress label Nov 26, 2018
If minimum value for parameter is not defined, it equals to zero. If maximum value is not defined, it equals to
1,073,741,823.

All the monetary values in the table (storage fee factor, minimum box value) are in nanoErgs. All the sizes (block etc)
Copy link
Member

Choose a reason for hiding this comment

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

It is better to include Units in the table, when table will be bigger, it will be hard to read.
Also looks like that minimum box value and storage fee factor are in nanoErg/byte, not in nanoErg.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

\begin{tabular}{*{6}{l}}
Id & Description & Default & Step & Min & Max \\
\hline
1 & Storage fee factor (per byte per storage period) & 1250000 & 25000 & 0 & 5000000 \\
Copy link
Member

Choose a reason for hiding this comment

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

Why 5000000 instead of previously discussed 2500000?

Copy link
Member Author

Choose a reason for hiding this comment

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

idk but fixed

Id & Description & Default & Step & Min & Max \\
\hline
1 & Storage fee factor (per byte per storage period) & 1250000 & 25000 & 0 & 5000000 \\
2 & Minimum monetary value of a box & 360 & 10 & 0 & 10000000 \\
Copy link
Member

Choose a reason for hiding this comment

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

With step 10 maximum value will be achieved in 3896 years (if it will be increased every epoch).
I propose to remove limits, or set some realistic limit that can be achieved in several years, 10000000 does not make sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reduced by the factor of 1,000.

1 & Storage fee factor (per byte per storage period) & 1250000 & 25000 & 0 & 5000000 \\
2 & Minimum monetary value of a box & 360 & 10 & 0 & 10000000 \\
3 & Maximum block size & 524288 & - & - & - \\
4 & Maximum cumulative computational cost of a block & 1000000 & - & - & - \\
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to add a minimal value here (and to block size), that will allow to include at least transaction that collects emission.

Copy link
Member Author

Choose a reason for hiding this comment

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

added, both = 16K

epochLength = 256

# Length of an epoch in difficulty recalculation. 1 means difficulty recalculation every block
Copy link
Member

Choose a reason for hiding this comment

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

it is not Length of an epoch in difficulty recalculation

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@@ -105,6 +114,10 @@ ergo {
# So we check from a queue only one box per "scanningInterval".
scanningInterval = 1s
}

voting {
1 = 2000000
Copy link
Member

Choose a reason for hiding this comment

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

Let's specify all parameters (with comments, describing what does they mean) here and set them to default values first,

Copy link
Member Author

Choose a reason for hiding this comment

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

Then a node will vote automatically to always adjust back to default values, which is not appropriate behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

See new comments there.

case class InfoRoute(statsCollector: ActorRef,
settings: RESTApiSettings,
timeProvider: NetworkTimeProvider)
(implicit val context: ActorRefFactory) extends ErgoBaseApiRoute {
override val route: Route = withCors {
info
info ~ params
Copy link
Member

Choose a reason for hiding this comment

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

Our previous API routes had the same prefix for all paths. Adding params here looks inconsistent, we may add common prefix for info and params or move params route to a separate class

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed somehow: now parameters are under /info resulting object.

@@ -28,6 +28,7 @@ import scala.util.Try
* bytes value size.
*/
case class Extension(headerId: ModifierId,
height: Int,
Copy link
Member

Choose a reason for hiding this comment

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

We already have height in Header, why to duplicate it here? I think we should only one height field, Header looks better option for it.
Also this change should be included in class comments and in openapi.yaml (but I propose to remove height from here).

Copy link
Member Author

Choose a reason for hiding this comment

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

height removed

@kushti kushti merged commit fce0ab1 into v2.0 Jan 15, 2019
@catena2w catena2w deleted the voting branch January 17, 2019 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-ready-for-merge Status: This PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants