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

[Merged by Bors] - vm, blocks: integrate rewards from updated economics #3559

Closed
wants to merge 4 commits into from

Conversation

dshulyak
Copy link
Contributor

@dshulyak dshulyak commented Sep 16, 2022

closes: spacemeshos/pm#151
closes: #3408
closes: #3251

this change enforces next rules for block syntactic validity:

  • every block must have at least one reward (empty layers are an obvious exception)
  • to make block more compact there should be at most one reward per coinbase (smeshers are not recorded in rewards) - this is already like that in the code that generates block, i am adding a validation for it
  • weight is recorded as fractional int using two uint64 (num/denom), if any of those is 0 - block is syntactically invalid

total rewards is a sum of fees and subsidy (subsidy is computed using github.com/spacemeshos/economics). total rewards are splited between coinbases recorded in the block. each coinbase share is a relative to the total weight recorded in the block. share is computed using big.Rat from stdlib on integer values.

absolute value is computed by multiplying total reward by big.Rat numerator and divides by denominator using big.Int from stdlib (no floating operations).

@@ -41,7 +41,7 @@ func (v *Vault) Available(lid core.LayerID) uint64 {
}
incremental := new(big.Int).SetUint64(v.TotalAmount - v.InitialUnlockAmount)
incremental.Mul(incremental, new(big.Int).SetUint64(uint64(lid.Difference(v.VestingStart))))
incremental.Div(incremental, new(big.Int).SetUint64(uint64(v.VestingEnd.Difference(v.VestingStart))))
incremental.Quo(incremental, new(big.Int).SetUint64(uint64(v.VestingEnd.Difference(v.VestingStart))))
Copy link
Contributor Author

@dshulyak dshulyak Sep 16, 2022

Choose a reason for hiding this comment

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

this is for consistency with rewards, Div and Quo works the same for uint64

@dshulyak dshulyak changed the title vm, blocks: integrate rewards from updated economics and revisit block syntactic validity rules vm, blocks: integrate rewards from updated economics Sep 16, 2022
@dshulyak
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Sep 16, 2022
@bors
Copy link

bors bot commented Sep 16, 2022

try

Build succeeded:

@dshulyak dshulyak marked this pull request as ready for review September 17, 2022 04:28
genvm/vm.go Outdated Show resolved Hide resolved
// set the block ID when received
b.Initialize()

if err := vm.ValidateRewards(b.Rewards); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do it here instead of after the de-duplication check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because syntactic validation is cheap. i can move it afterwards, doesn't matter much to me

blocks/handler.go Outdated Show resolved Hide resolved
blocks/handler_test.go Outdated Show resolved Hide resolved
Quo(subsidyReward, relative.Denom())
if !subsidyReward.IsUint64() {
return fmt.Errorf("%w: subsidy reward %v for %v overflows uint64",
core.ErrInternal, subsidyReward, blockReward.Coinbase)
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like you can factor out an utility method to do the division and check uint64

Copy link
Contributor Author

@dshulyak dshulyak Sep 19, 2022

Choose a reason for hiding this comment

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

it is looks cleaner without such method

v.logger.With().Debug("rewards for layer",
lctx.Layer,
log.Uint32("after genesis", layersAfterEffectiveGenesis),
log.Uint64("subsidy estimated", subsidy),
Copy link
Contributor

Choose a reason for hiding this comment

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

why the term "estimated" here and for total?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actual amount computed from how much was transfered

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand - "estimated" here means "before rounding"? or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

before rounding

Copy link
Contributor

@countvonzero countvonzero left a comment

Choose a reason for hiding this comment

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

@dshulyak
Copy link
Contributor Author

i think we should burn the subsidy if there are empty layers

they are already burned

@dshulyak
Copy link
Contributor Author

bors merge

bors bot pushed a commit that referenced this pull request Sep 19, 2022
closes: spacemeshos/pm#151
closes: #3408
closes: #3251

this change enforces next rules for block syntactic validity:
- every block must have at least one reward (empty layers are an obvious exception)
- to make block more compact there should be at most one reward per coinbase (smeshers are not recorded in rewards) - this is already like that in the code that generates block, i am adding a validation for it
- weight is recorded as fractional int using two uint64 (num/denom), if any of those is 0 - block is syntactically invalid

total rewards is a sum of fees and subsidy (subsidy is computed using github.com/spacemeshos/economics). total rewards are splited between coinbases recorded in the block. each coinbase share is a relative to the total weight recorded in the block. share is computed using big.Rat from stdlib on integer values.

absolute value is computed by multiplying total reward by big.Rat numerator and divides by denominator using big.Int from stdlib (no floating operations).
@bors
Copy link

bors bot commented Sep 19, 2022

Pull request successfully merged into develop.

Build succeeded:

@bors bors bot changed the title vm, blocks: integrate rewards from updated economics [Merged by Bors] - vm, blocks: integrate rewards from updated economics Sep 19, 2022
@bors bors bot closed this Sep 19, 2022
@countvonzero
Copy link
Contributor

i think we should burn the subsidy if there are empty layers

they are already burned

@dshulyak sorry i missed that. where does that happen?

@dshulyak
Copy link
Contributor Author

@dshulyak sorry i missed that. where does that happen?

what do you mean? rewards that are not transferred are burned. so in case of empty layer - rewards are always burned

@countvonzero
Copy link
Contributor

@dshulyak sorry i missed that. where does that happen?

what do you mean? rewards that are not transferred are burned. so in case of empty layer - rewards are always burned

but we don't call vm.Apply() on types.EmptyBlockID

@dshulyak
Copy link
Contributor Author

but we don't call vm.Apply() on types.EmptyBlockID

burning doesn't modify any state

@countvonzero
Copy link
Contributor

but we don't call vm.Apply() on types.EmptyBlockID

burning doesn't modify any state

oh ok. its in the math (num layers since genesis). i get it now. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants