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

Add more stake-related ledger queries #2161

Merged
merged 1 commit into from
May 28, 2020
Merged

Add more stake-related ledger queries #2161

merged 1 commit into from
May 28, 2020

Conversation

intricate
Copy link
Contributor

@intricate intricate commented May 28, 2020 •
edited

@intricate intricate added the consensus issues related to ouroboros-consensus label May 28, 2020
@intricate intricate self-assigned this May 28, 2020
@intricate intricate requested review from dcoutts and mrBliss May 28, 2020 02:11
@intricate intricate marked this pull request as ready for review May 28, 2020 05:03
@intricate intricate changed the title Add GetFilteredRegisteredStakeCreds and GetFilteredDelegations ledger queries Add more stake-related ledger queries May 28, 2020
mrBliss
mrBliss approved these changes May 28, 2020
View reviewed changes
Copy link
Contributor

@mrBliss mrBliss left a comment

Choose a reason for hiding this comment

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

LGTM

dcoutts
dcoutts requested changes May 28, 2020
View reviewed changes
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

I think we will not need the GetFilteredRegisteredStakeCreds and just need the other two. I'm double checking this...

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Ledger.hs Outdated Show resolved Hide resolved
ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Ledger.hs Outdated
Comment on lines 340 to 342
GetFilteredRegisteredStakeCreds
:: Set (SL.Credential 'SL.Staking c)
-> Query (ShelleyBlock c) (SL.StakeCreds c)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm double checking, but I think this one will not be needed, since the StakeCreds does not contain any useful information, and indeed will soon be changed to be just a Set once we remove decaying rewards.

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed!

@dcoutts
Copy link
Contributor

dcoutts commented May 28, 2020

After consulting with @JaredCorduan we think that indeed we do not need to look up the _stkCreds at all, and just need to look at the _rewards and _delegations. The _rewards will have an entry when the address is registered at all, and _delegations will only have an entry when there is a current/active delegation choice.

So we probably only want a single query, given the set of stake key (hashes): look up in both the _rewards and _delegations and return both maps. So I think you can do that by doing the Map.restrictKeys on both maps as you do now. We can merge them on the client side if we need to.

@intricate intricate force-pushed the intricate/more-stake-cred-ledger-queries branch from dc7d1cb to 6538fac Compare May 28, 2020 17:49
@intricate intricate requested a review from dcoutts May 28, 2020 17:50
dcoutts
dcoutts approved these changes May 28, 2020
View reviewed changes
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

LGTM!

@dcoutts
Copy link
Contributor

dcoutts commented May 28, 2020

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 28, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 7826943 into master May 28, 2020
@iohk-bors iohk-bors bot deleted the intricate/more-stake-cred-ledger-queries branch May 28, 2020 21:23
coot pushed a commit that referenced this pull request May 16, 2022
@iohk-bors @intricate
Merge #2161
8572312 
2161: Add more stake-related ledger queries r=dcoutts a=intricate

Requirement for IntersectMBO/cardano-node#1053

Co-authored-by: Luke Nadur <19835357+intricate@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JaredCorduan JaredCorduan JaredCorduan left review comments

@mrBliss mrBliss mrBliss approved these changes

@dcoutts dcoutts dcoutts approved these changes

Assignees

@intricate intricate

Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@intricate @dcoutts @mrBliss @JaredCorduan