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

Volatile DB: let the getters return more info #1684

Closed
mrBliss opened this issue Feb 24, 2020 · 1 comment · Fixed by #1772
Closed

Volatile DB: let the getters return more info #1684

mrBliss opened this issue Feb 24, 2020 · 1 comment · Fixed by #1772
Assignees
@kderme
Labels
consensus issues related to ouroboros-consensus technical debt volatile db
Milestone
S8 2020-03-12

Comments

@mrBliss
Copy link
Contributor

mrBliss commented Feb 24, 2020

Unify getIsMember and getPredecessor, and return BlockInfo. Possibly unify it with getSuccessors too, e.g., using return a pair of both results. The laziness of the pair can save us the lookup in the successors map when we don't need it.
@mrBliss mrBliss added consensus issues related to ouroboros-consensus technical debt volatile db labels Feb 24, 2020
@mrBliss mrBliss added this to the S8 2020-03-12 milestone Feb 24, 2020
@kderme kderme mentioned this issue Feb 26, 2020
Merged
iohk-bors bot added a commit that referenced this issue Feb 27, 2020
@iohk-bors @kderme
Merge #1701
7d7ad84 
1701: VolatileDB: Unify and extend getters r=mrBliss a=kderme

Related issue: #1684
This is done:
> Unify getIsMember and getPredecessor, and return BlockInfo

This is not done yet:
> Possibly unify it with getSuccessors too, e.g., using return a pair of both results.

Co-authored-by: kderme <[email protected]>
iohk-bors bot added a commit that referenced this issue Feb 27, 2020
@iohk-bors @kderme @mrBliss
Merge #1701
c6df49d 
1701: VolatileDB: Unify and extend getters r=mrBliss a=kderme

Related issue: #1684
This is done:
> Unify getIsMember and getPredecessor, and return BlockInfo

This is not done yet:
> Possibly unify it with getSuccessors too, e.g., using return a pair of both results.

Co-authored-by: kderme <[email protected]>
Co-authored-by: Thomas Winant <[email protected]>
@mrBliss
Copy link
Contributor Author

mrBliss commented Mar 9, 2020

@kderme I was thinking about the remaining work to do in this issue after #1701: unifying the following two functions:

getSuccessors :: HasCallStack => STM m (WithOrigin blockId -> Set blockId)
getBlockInfo  :: HasCallStack => STM m (blockId -> Maybe (BlockInfo blockId))

Looking at the types, in particular, WithOrigin blockId vs. blockId, it became clear to me that we should not unify these two.

Moreover, getSuccessors has a deceptive name, because it's based on the information we have about predecessors, not about successors.

@edsko had the good idea to rename it to filterByPredecessor. Can you make the change? That would complete the work of this issue.

@kderme kderme mentioned this issue Mar 10, 2020
Merged
@mrBliss mrBliss linked a pull request Mar 12, 2020 that will close this issue
Rename getSuccessors to filterByPredecessor #1772
Merged
iohk-bors bot added a commit that referenced this issue Mar 12, 2020
@iohk-bors @kderme
Merge #1772
07b1c52 
1772: Rename getSuccessors to filterByPredecessor r=mrBliss a=kderme

Last piece of #1684

Co-authored-by: kderme <[email protected]>
@iohk-bors iohk-bors bot closed this as completed in #1772 Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kderme kderme

Labels
consensus issues related to ouroboros-consensus technical debt volatile db
Projects
None yet
Milestone
S8 2020-03-12
Development

Successfully merging a pull request may close this issue.

Rename getSuccessors to filterByPredecessor IntersectMBO/ouroboros-network
2 participants
@mrBliss @kderme