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.

Sign up for GitHub

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

Jump to bottom

R4R: Distr-PR-5 Implement Distribution #2236

Merged
merged 121 commits into from
Oct 16, 2018
Merged

R4R: Distr-PR-5 Implement Distribution #2236

merged 121 commits into from
Oct 16, 2018

Conversation

rigelrozanski
Copy link
Contributor

@rigelrozanski rigelrozanski commented Sep 4, 2018 •
edited
Loading

Closes #1671
Closes #583

Precursor PRs to merge before this one:

Related work which should be done in new separate PR's after this one

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added entries in PENDING.md with issue #
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@cwgoes
Copy link
Contributor

cwgoes commented Sep 4, 2018 •
edited
Loading

Is this PR intended to contain both spec and implementation?

(I'm confused by it changing the Lamborghini fee spec, which I thought we decided not to implement)

@rigelrozanski
Copy link
Contributor Author

@cwgoes it doesn't change the lamboragini spec, I just moved it's location (and then I moved it back for PR clarity, but for some reason it's still showing up in the git diff, I'll look into it before it's R4R)

@cwgoes cwgoes mentioned this pull request Sep 7, 2018
Closed
23 tasks
@fedekunze fedekunze mentioned this pull request Sep 10, 2018
Closed
@rigelrozanski
Copy link
Contributor Author

@cwgoes - mostly addressed your comments left a few "unresolved" if you care to comment further I'll check up / also - time for a final review!

cwgoes
cwgoes suggested changes Oct 12, 2018
View reviewed changes
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Mostly minor changes req'd, testcases look comprehensive.

cmd/gaia/app/app.go Outdated Show resolved Hide resolved
cmd/gaia/app/app.go Outdated Show resolved Hide resolved
docs/spec/distribution/end_block.md Outdated Show resolved Hide resolved
docs/spec/distribution/state.md Outdated Show resolved Hide resolved
types/context.go Show resolved Hide resolved
x/distribution/keeper/allocation.go Outdated Show resolved Hide resolved
x/distribution/keeper/allocation.go Outdated Show resolved Hide resolved
x/distribution/keeper/allocation.go Outdated Show resolved Hide resolved
x/distribution/keeper/allocation.go Show resolved Hide resolved
x/stake/keeper/delegation.go Show resolved Hide resolved
@jackzampolin jackzampolin mentioned this pull request Oct 12, 2018
Closed
@rigelrozanski rigelrozanski mentioned this pull request Oct 12, 2018
Closed
ValarDragon
ValarDragon reviewed Oct 15, 2018
View reviewed changes
x/distribution/keeper/allocation.go Outdated
// allocated rewards to proposer
percentVotes := k.GetPercentPrecommitVotes(ctx)

proposerMultiplier := sdk.NewDecWithPrec(1, 2).Add(sdk.NewDecWithPrec(4, 2).Mul(percentVotes))
Copy link
Contributor

@ValarDragon ValarDragon Oct 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This 4 should be moved to a constant / var. We can put it into the paramstore in a second PR. This line could also use a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as per previous discussions not sure we care to put this in the params store, if we don't expect to change often at-all - I wonder how that change would affect the performance though? This code does execute every block

Copy link
Contributor

@ValarDragon ValarDragon Oct 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In my opinion, this seems like one of those things that governance should absolutely be able to control. It doesn't matter if we think it will change often.

Copy link
Contributor Author

@rigelrozanski rigelrozanski Oct 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@ValarDragon governance can control it either way with hard upgrade so that shouldn't be the primary motive.. I reckon expected upgrade frequency does play a role if whether or not to add to the params store - as well as performance costs

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the performance costs are substantial at all, especially once we have inter-block caching.

Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't see why we'd want a hard upgrade for something that can be changed live. This is absolutely a negligible performance cost, especially since it can be stored as a decimal in a local cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool - thanks for the details - then agreed - I'll just update it now here, save opening another issue

ValarDragon
ValarDragon reviewed Oct 15, 2018
View reviewed changes
types/decimal.go Outdated Show resolved Hide resolved
@rigelrozanski rigelrozanski mentioned this pull request Oct 15, 2018
Closed
cwgoes
cwgoes approved these changes Oct 15, 2018
View reviewed changes
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

One tiny comment but doesn't need to block merge - utACK 🎈 💰

x/distribution/keeper/key.go Outdated Show resolved Hide resolved
cwgoes
cwgoes suggested changes Oct 15, 2018
View reviewed changes
Copy link
Contributor

@cwgoes cwgoes left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Ah Houston, I think we may have a 🚀 💰 problem after all - the proposer is from the current block, but the percent-voting is from the last commit - that won't incentivize the proposer to include commits...

We might need to process fees for block n in the beginblocker of n+1

@rigelrozanski
Copy link
Contributor Author

Solution from slack:

rige [5 minutes ago]
that’s so confusing - so if percent voting is from the previous block, then the rewards and proposer should be getting stored a block and then distributed at begin-block of the next block, when percent-voting is available
cwgoes [5 minutes ago]
yeah....
cwgoes [4 minutes ago]
it is really confusing (edited)
cwgoes [4 minutes ago]
cc bucky can you double-check our understanding here
rige [3 minutes ago]
Well if what you’re saying is correct - that in begin block, we have the proposer from the current block and the precommits from the previous block then the solution I just proposed would work - still weird though
bucky [2 minutes ago]
BeginBlock includes the proposer from the current block and the commits from the last block
bucky [2 minutes ago]
thats kinda nuts isnt it
cwgoes [1 minute ago]
so we would have to store the proposer one block in the SDK and use it in the next block's BeginBlock
bucky [1 minute ago]
well it makes sense given the proposer is in the header and its the header for this block … but it means you need to remember the header from the last block to know which proposer to reward for the commits you’re hearing aobut in this block
rige [1 minute ago]
Well I think it’s fine if that’s when the info first safely becomes available - however I would suggest renaming those fields to make it explicit that it’s the previous block
cwgoes [1 minute ago]
I think that's fine but ++ on renaming the fields
rige [< 1 minute ago]
👍
rige [< 1 minute ago]
cool - I understand what needs to be changed - will fix tn

@cwgoes
Copy link
Contributor

cwgoes commented Oct 16, 2018

utACK, looks like CI nondeterminism is causing test_cover to fail here 👿.

@cwgoes cwgoes merged commit 2803830 into develop Oct 16, 2018
@cwgoes
Copy link
Contributor

cwgoes commented Oct 16, 2018

Ran CI again twice...

@cwgoes cwgoes deleted the rigel/fee-distribution branch October 16, 2018 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaekwon jaekwon jaekwon left review comments

@ValarDragon ValarDragon ValarDragon left review comments

@cwgoes cwgoes cwgoes approved these changes

@ebuchman ebuchman Awaiting requested review from ebuchman

@zramsay zramsay Awaiting requested review from zramsay

Assignees

@cwgoes cwgoes

Labels
C:x/distribution distribution module related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rigelrozanski @cwgoes @ValarDragon @jaekwon