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

Make estimator return block hashes instead of full block #2222

Merged
merged 12 commits into from Mar 12, 2019

Conversation

@KentShikama
Copy link
Collaborator

commented Mar 2, 2019

Overview

What this PR does, and why it's needed

Some calls to the estimator don't require the full BlockMessage
to be returned. Thus we should only return the hashes and
let the caller fetch the BlockMessage based on those hashes
if they need to.

This also removes the BlockStore effect from the Estimator.

JIRA ticket:

Create it if there isn't one already.

https://rchain.atlassian.net/browse/RCHAIN-3069

Notes

Optional. Add any notes on caveats, approaches you tried that didn't work, or anything else.

Please make sure that this PR:

Bors cheat-sheet:

  • bors r+ runs integration tests and merges the PR (if it's approved),
  • bors try runs integration tests for the PR,
  • bors delegate+ enables non-maintainer PR authors to run the above.

KentShikama added some commits Mar 2, 2019

Rename to LCA where appropriate
This is clean up from the LCA PR: #2205
Make estimator return block hashes instead of BlockMessage
Some calls to the estimator don't require the full BlockMessage
to be returned. Thus we should only return the hashes and
let the caller fetch the BlockMessage based on those hashes
if they need to.

This also removes the BlockStore effect from the Estimator.

@KentShikama KentShikama requested review from itegulov, ArturGajowy, dzajkowski, lukasz-golebiewski and odeac and removed request for itegulov and ArturGajowy Mar 2, 2019

@KentShikama

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 2, 2019

bors try

bors bot added a commit that referenced this pull request Mar 2, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2019

try

Build succeeded

@KentShikama

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 2, 2019

bors try

bors bot added a commit that referenced this pull request Mar 2, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2019

try

Build succeeded

val validatorWeight = childBlock.weightMap.getOrElse(validator, 0L)
acc2.updated(childHash, currScore + validatorWeight)
case _ => acc2
}

This comment has been minimized.

Copy link
@odeac

odeac Mar 5, 2019

Collaborator

My head hurts when I try to understand this function. Please make it more readable. Giving names to parts of the computation would probably be enough

This comment has been minimized.

Copy link
@KentShikama

KentShikama Mar 5, 2019

Author Collaborator

The change is NFC. Hopefully the after version is better than the before. If not I'll just revert this entirely.

This comment has been minimized.

Copy link
@KentShikama

KentShikama Mar 5, 2019

Author Collaborator

I've also just considered removing this part. It was initially an idea to make the estimator a bit more fair but for the amount of thinking required verify its correctness I don't think it is worth it.

This comment has been minimized.

Copy link
@KentShikama

KentShikama Mar 5, 2019

Author Collaborator

On further thought, I'm going to remove this indeed.

Remove implicitly supported validator weights
The complexity of this method is not worth the gains in
perceived fairness of the estimator
@KentShikama

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 6, 2019

bors try

@dzajkowski

This comment has been minimized.

maybe it would make sense to inline map(_.head)?

bors bot added a commit that referenced this pull request Mar 6, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

try

Build succeeded

result <- blocks
.foldM(List.empty[BlockMessage]) {
case (acc, b) =>
acc.forallM(nonConflicting(b)).map { isNonConflictingWithAll =>

This comment has been minimized.

Copy link
@dzajkowski

dzajkowski Mar 6, 2019

Collaborator

the previous version seemed easier to read. why the change?

This comment has been minimized.

Copy link
@KentShikama

KentShikama Mar 7, 2019

Author Collaborator

I thought this version was easier to read; the Monad[F].ifM syntax just doesn't read cleanly to me.

This comment has been minimized.

Copy link
@odeac

odeac Mar 8, 2019

Collaborator

I agree with Dom that the previuos was cleaner. And even if ifM is difficult to read, because of the fact that it's a standard operator it is easy to learn once and then expect the same behaviour everywhere

This comment has been minimized.

Copy link
@KentShikama

KentShikama Mar 9, 2019

Author Collaborator

Ok I'll revert to using ifM.

@odeac

odeac approved these changes Mar 8, 2019

@KentShikama

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 11, 2019

bors try

bors bot added a commit that referenced this pull request Mar 11, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

try

Build succeeded

KentShikama added some commits Mar 12, 2019

@KentShikama

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 12, 2019

bors r+

bors bot added a commit that referenced this pull request Mar 12, 2019

Merge #2222
2222: Make estimator return block hashes instead of full block  r=KentShikama a=KentShikama



Co-authored-by: Kent Shikama <kent@kentshikama.com>
@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@KentShikama

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 12, 2019

bors r+

bors bot added a commit that referenced this pull request Mar 12, 2019

Merge #2222
2222: Make estimator return block hashes instead of full block  r=KentShikama a=KentShikama



Co-authored-by: Kent Shikama <kent@kentshikama.com>
@KentShikama

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 12, 2019

bors r+

@ArturGajowy

This comment has been minimized.

Copy link
Collaborator

commented Mar 12, 2019

bors r-

@ArturGajowy

This comment has been minimized.

Copy link
Collaborator

commented Mar 12, 2019

bors r+

1 similar comment
@ArturGajowy

This comment has been minimized.

Copy link
Collaborator

commented Mar 12, 2019

bors r+

bors bot added a commit that referenced this pull request Mar 12, 2019

Merge #2222
2222: Make estimator return block hashes instead of full block  r=ArturGajowy a=KentShikama



Co-authored-by: Kent Shikama <kent@kentshikama.com>
@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@bors bors bot merged commit 0d00279 into rchain:dev Mar 12, 2019

2 checks passed

bors Build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.