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

go/txsource: commission schedule amendments workload #2766

Merged
merged 2 commits into from
Mar 24, 2020

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented Mar 13, 2020

Changing commission schedule workload, part of: #2506

TODO:

  • minor todo's see code comments

@ptrus ptrus force-pushed the ptrus/feature/txsource-delegation-commission branch from 49eff4c to 01543f1 Compare March 13, 2020 13:18
@codecov
Copy link

codecov bot commented Mar 13, 2020

Codecov Report

Merging #2766 into master will increase coverage by 0.25%.
The diff coverage is 76.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2766      +/-   ##
==========================================
+ Coverage   62.99%   63.25%   +0.25%     
==========================================
  Files         388      389       +1     
  Lines       36828    37047     +219     
==========================================
+ Hits        23200    23433     +233     
+ Misses      10661    10640      -21     
- Partials     2967     2974       +7     
Impacted Files Coverage Δ
...oasis-node/cmd/debug/txsource/workload/workload.go 67.07% <ø> (ø)
go/staking/api/api.go 71.42% <ø> (ø)
go/consensus/tendermint/staking/staking.go 69.37% <60.00%> (-0.31%) ⬇️
go/staking/api/grpc.go 58.47% <64.70%> (+0.38%) ⬆️
...sis-node/cmd/debug/txsource/workload/commission.go 77.43% <77.43%> (ø)
go/consensus/tendermint/apps/staking/query.go 73.17% <100.00%> (+1.37%) ⬆️
go/staking/api/commission.go 98.66% <100.00%> (ø)
go/worker/common/host/interface.go 38.46% <0.00%> (-15.39%) ⬇️
...ompute/txnscheduler/algorithm/batching/batching.go 78.66% <0.00%> (-6.67%) ⬇️
go/worker/compute/txnscheduler/committee/node.go 62.34% <0.00%> (-4.02%) ⬇️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea2c74d...f0da05c. Read the comment docs.

@ptrus ptrus force-pushed the ptrus/feature/txsource-delegation-commission branch from 01543f1 to 16443da Compare March 16, 2020 09:11
@ptrus ptrus marked this pull request as ready for review March 16, 2020 09:36
@kostko
Copy link
Member

kostko commented Mar 16, 2020 via email

@ptrus
Copy link
Member Author

ptrus commented Mar 16, 2020

Wouldn't it be better if the CLI argument to configure the runtime ID would be part of the runtime workload definition?

yeah (also probably ment for: #2759 :) )

@kostko
Copy link
Member

kostko commented Mar 16, 2020 via email

@ptrus ptrus force-pushed the ptrus/feature/txsource-delegation-commission branch from a300ac6 to cc245c9 Compare March 16, 2020 11:51
@ptrus ptrus self-assigned this Mar 16, 2020
@ptrus ptrus force-pushed the ptrus/feature/txsource-delegation-commission branch from cc245c9 to 5e53567 Compare March 17, 2020 13:15
go/oasis-node/cmd/debug/txsource/workload/commission.go Outdated Show resolved Hide resolved
go/oasis-node/cmd/debug/txsource/workload/commission.go Outdated Show resolved Hide resolved
go/oasis-node/cmd/debug/txsource/workload/commission.go Outdated Show resolved Hide resolved
go/oasis-node/cmd/debug/txsource/workload/commission.go Outdated Show resolved Hide resolved
go/oasis-node/cmd/debug/txsource/workload/commission.go Outdated Show resolved Hide resolved
go/oasis-node/cmd/debug/txsource/workload/commission.go Outdated Show resolved Hide resolved
go/oasis-node/cmd/debug/txsource/workload/commission.go Outdated Show resolved Hide resolved
go/oasis-node/cmd/debug/txsource/workload/commission.go Outdated Show resolved Hide resolved
go/oasis-node/cmd/debug/txsource/workload/commission.go Outdated Show resolved Hide resolved
go/oasis-node/cmd/debug/txsource/workload/commission.go Outdated Show resolved Hide resolved
@ptrus ptrus force-pushed the ptrus/feature/txsource-delegation-commission branch from 5e53567 to adb80b0 Compare March 18, 2020 16:58
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

I want to know about the schedule pruning. Where is that done?

go/oasis-node/cmd/debug/txsource/workload/commission.go Outdated Show resolved Hide resolved
go/oasis-node/cmd/debug/txsource/workload/commission.go Outdated Show resolved Hide resolved

// Check existing bound steps. In case there are existing steps for epoch
// before `nextAlignedBoundChangeEpoch`, those cannot get amended and also
// won't be pruned yet. Therefore we need to count those to not go over the
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 see that we're checking for steps that would be pruned

Copy link
Member Author

Choose a reason for hiding this comment

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

We aren't checking for steps that would be pruned. We are checking for existing steps that won't get amended by the amendment that we are making (since the earliest step in the amendment we are submitting will be at nextAlignedBoundChangeEpoch, existing steps for epochs smaller than it (and greater than current epoch) will be kept).

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

link seems to be out of date

Copy link
Contributor

Choose a reason for hiding this comment

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

We aren't checking for steps that would be pruned.

Shouldn't we though? Because we can make the amendment longer if we take that into account

Copy link
Contributor

Choose a reason for hiding this comment

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

and the comment mentions that it only counts steps that wouldn't be pruned. but the code counts steps that would be pruned too

Copy link
Member Author

Choose a reason for hiding this comment

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

oh i think i know what you're saying, so the existingCommissionSchedule obtained by account.Escrow.CommissionSchedule could itself contain non pruned steps (meaning contain more than one past step with epoch.Start<currentEpoch) - since we actually only prune it on amendments and not automatically on every epoch.

I don't think that can actually happen at the moment since we are constantly amending schedules, the existing schedules should get pruned more or less on every epoch, but nevertheless i'll do pruning on the obtained existingCommissionSchedule to cover the case.

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 that can actually happen at the moment since we are constantly amending schedules, the existing schedules should get pruned more or less on every epoch

ah yeah good point

go/oasis-node/cmd/debug/txsource/workload/commission.go Outdated Show resolved Hide resolved
go/oasis-node/cmd/debug/txsource/workload/commission.go Outdated Show resolved Hide resolved
@ptrus ptrus force-pushed the ptrus/feature/txsource-delegation-commission branch 3 times, most recently from c1b3ea2 to 393d7e7 Compare March 23, 2020 10:46
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

lg thanks for the changes

@ptrus ptrus force-pushed the ptrus/feature/txsource-delegation-commission branch from 393d7e7 to f0da05c Compare March 24, 2020 16:37
@ptrus
Copy link
Member Author

ptrus commented Mar 24, 2020

thanks for the reviews

@ptrus ptrus merged commit 57b86bb into master Mar 24, 2020
@ptrus ptrus deleted the ptrus/feature/txsource-delegation-commission branch March 24, 2020 17:11
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.

None yet

3 participants