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

3222 estimate user reward for layer #3296

Closed
wants to merge 3 commits into from

Conversation

abergasov
Copy link
Contributor

@abergasov abergasov commented Jun 24, 2022

Motivation

Closes #3222

Changes

implement endpoint, which try estimate reward for layer X.

for past layers - simply look at db.
for current or future layers - calc it from weight and activations

pr for api modify spacemeshos/api#167

Test Plan

TODO

  • Explain motivation or link existing issue(s)
  • Test changes and document test plan
  • Update documentation as needed

DevOps Notes

  • This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
  • This PR does not affect public APIs
  • This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
  • This PR does not make changes to log messages (which monitoring infrastructure may rely on)

@abergasov abergasov linked an issue Jun 24, 2022 that may be closed by this pull request
@abergasov abergasov force-pushed the 3222-tweak-smesherservice-api branch from c4c1a49 to f1b45df Compare June 24, 2022 12:39
@abergasov abergasov force-pushed the 3222-tweak-smesherservice-api branch from fd1115a to 9d32ecd Compare June 24, 2022 15:18
@abergasov abergasov changed the title estimate user reward for layer 3222 estimate user reward for layer Jun 27, 2022
@@ -54,3 +54,22 @@ func List(db sql.Executor, coinbase types.Address) (rst []*types.Reward, err err
})
return
}

// RewardPerLayer returns the reward per layer from `rewards` table.
func RewardPerLayer(db sql.Executor, layerID uint32) (rwd *types.Reward, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please don't use uint32 for layer id in the code, the type should be consistent with the rest of the codebase

Copy link
Contributor

@dshulyak dshulyak Jun 28, 2022

Choose a reason for hiding this comment

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

but in general i don't get the idea. there are on average 50 rewards per layer for different coinbases. all of them may have different relative weight. which one do you want to get?

Copy link
Contributor Author

@abergasov abergasov Jun 28, 2022

Choose a reason for hiding this comment

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

there are on average 50 rewards per layer for different coinbases. all of them may have different weight behind them. which one do you want to get?

but node will get reward only for block, which will be constructed after Hair. It can be several blocks in hair output, but is should be extraordinary situations, and our estimation will not work in that cases

Copy link
Contributor

Choose a reason for hiding this comment

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

its hare, not hair

it always outputs a single block, but i don't understand why is it even important

i don't know how to explain it better, but this function will return a first reward in the requested layer.. it may not even be related to your smesher id, and will be based on the different relative weight. thats why it doesn't make any sense to me

moreover i don't understand where idea to get an estimated reward for a previous layer came from. api can provide already applied rewards for applied layers. i think this need to be removed from the change completely

Copy link
Contributor

Choose a reason for hiding this comment

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

based on your messages i thought that you are going to provide an estimate for this layer based on the previous layers results, but this is not what is going on in this change

Copy link
Contributor Author

@abergasov abergasov Jun 28, 2022

Choose a reason for hiding this comment

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

yep, mistake in hare)

it always outputs a single block, but i don't understand why is it even important

i thought, that node receive reward only after Hare create block. so in that case not important, how many block can be before

api can provide already applied rewards for applied layers

yep, we have AccountDataQuery endpoint, but you should specify coinbaseAddress for load reward per layer, which can be problem if coinbaseAddress was changed

provide an estimate for this layer based on the previous layers results

only if it make prediction more accurate. Idea was just show approximately, how much reward user will get in current|next layer. If for some reasons need be calculated reward for previous layers - that's why layerID was added in request

@@ -54,3 +54,22 @@ func List(db sql.Executor, coinbase types.Address) (rst []*types.Reward, err err
})
return
}

// RewardPerLayer returns the reward per layer from `rewards` table.
func RewardPerLayer(db sql.Executor, layerID uint32) (rwd *types.Reward, err error) {
Copy link
Contributor

@dshulyak dshulyak Jun 28, 2022

Choose a reason for hiding this comment

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

but in general i don't get the idea. there are on average 50 rewards per layer for different coinbases. all of them may have different relative weight. which one do you want to get?

if err != nil {
return 0, errors.Wrap(err, "failed to estimate number of eligible slots")
}
return layerReward / uint64(expNumSlots), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't make sense

layer reward is divided between builders of the block, based on their relative weight

Copy link
Contributor Author

Choose a reason for hiding this comment

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

layerReward is just basic reward, which can be get for layer. Now it hardcoded value, but later it will be replaces with inflation formula

so we need manually divided to all block builders

Copy link
Contributor

@dshulyak dshulyak Jun 28, 2022

Choose a reason for hiding this comment

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

exp num slots is not a number of block builders (proposers) in a single layer. expected num slots is the number of proposals a particular smesher is eligible for in a whole epoch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, because totalWeight now is calculation of weight for all epoch

if totalWeight somehow way will be result of weight of layer, it should be set number of block builders to expNumSlots, right?

@abergasov abergasov force-pushed the 3222-tweak-smesherservice-api branch from f5b8035 to 114b69d Compare June 28, 2022 08:47
@dshulyak dshulyak closed this Oct 4, 2022
@fasmat fasmat deleted the 3222-tweak-smesherservice-api branch November 7, 2022 08:50
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.

Tweak SmesherService API
2 participants