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

Fix bug in clique oracle #2122

Merged
merged 19 commits into from Jan 31, 2019

Conversation

Projects
None yet
4 participants
@KentShikama
Copy link
Collaborator

KentShikama commented Jan 18, 2019

The bug was an off by 1 error in filterChildren calculation

I realized the seesAgreement checks are already covered
by the neverEventuallySeeDisagreement checks, so I remove those.

JIRA ticket:

Create it if there isn't one already.

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

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.
Remove seesAgreement checks
I realized the seesAgreement checks are already covered
by the neverEventuallySeeDisagreement checks
@KentShikama

This comment has been minimized.

Copy link
Collaborator Author

KentShikama commented Jan 18, 2019

bors try

bors bot added a commit that referenced this pull request Jan 18, 2019

@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Jan 18, 2019

@KentShikama KentShikama force-pushed the KentShikama:dev-kent-refactor-safety-oracle-again branch from fb32812 to 4048b04 Jan 18, 2019

Prepare for short circuit of potentialDisagreement compatibility
Currently we end up checking all potential disagreements for
compatibility with the candidate even if the first block fails.

@KentShikama KentShikama force-pushed the KentShikama:dev-kent-refactor-safety-oracle-again branch from 4048b04 to 3749980 Jan 18, 2019

KentShikama added some commits Jan 18, 2019

Fix off by 1 error in filterChildren calculation
We want to go down one block further so that we include
the block that was passed as an arg into filterChildren
Remove unnecessary call to filterF
Since we stop when b == creatorJustificationOrGenesis,
there is no longer a need to filter those below
@KentShikama

This comment has been minimized.

Copy link
Collaborator Author

KentShikama commented Jan 18, 2019

bors try

bors bot added a commit that referenced this pull request Jan 18, 2019

@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Jan 18, 2019

@KentShikama KentShikama reopened this Jan 18, 2019

@KentShikama KentShikama changed the title Remove seesAgreement checks in clique oracle Fix bug in clique oracle Jan 18, 2019

@KentShikama

This comment has been minimized.

Copy link
Collaborator Author

KentShikama commented Jan 18, 2019

bors try

bors bot added a commit that referenced this pull request Jan 18, 2019

@@ -1601,7 +1601,7 @@ class HashSetCasperTest extends FlatSpec with Matchers {
_ <- nodes(0).receive()
_ <- nodes(2).receive()

_ <- nodes(0).casperEff.lastFinalizedBlock shouldBeF genesisWithEqualBonds
_ <- nodes(0).casperEff.lastFinalizedBlock shouldBeF block1

This comment has been minimized.

@KentShikama

KentShikama Jan 18, 2019

Author Collaborator

My mistake was thinking that block1 was genesis. I wish I had thought through this a bit more the first time.

This comment has been minimized.

@KentShikama

KentShikama Jan 18, 2019

Author Collaborator

Note this isn't block2 because there are actually 4 validators involved and so there needs to be 3 total to make a majority.

@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Jan 18, 2019

KentShikama added some commits Jan 22, 2019

Use updated dag with the safety oracle
Previously we were passing in a stale copy of the dag
which prevented the finalization of certain blocks

@KentShikama KentShikama reopened this Jan 22, 2019

KentShikama added some commits Jan 30, 2019

Remove TODO to add justification follows check
Done with Validate.justificationFollows
Ensure that Validate.parents check uses last finalized block
Previously we were still using genesis.
This change should speed up our block validation.
@KentShikama

This comment has been minimized.

Copy link
Collaborator Author

KentShikama commented Jan 30, 2019

bors try

bors bot added a commit that referenced this pull request Jan 30, 2019

@KentShikama

This comment has been minimized.

Copy link
Collaborator Author

KentShikama commented Jan 30, 2019

bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Jan 30, 2019

try

Not awaiting review

@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Jan 30, 2019

try

Build succeeded

_ <- lock.release
} yield ()
_ <- lock.release
dag <- getRepresentation

This comment has been minimized.

@lukasz-golebiewski

lukasz-golebiewski Jan 30, 2019

Collaborator

Shouldn't the dag be fetched before releasing the lock?

This comment has been minimized.

@KentShikama

KentShikama Jan 30, 2019

Author Collaborator

getRepresentation requires the lock

This comment has been minimized.

@KentShikama

KentShikama Jan 30, 2019

Author Collaborator

@itegulov Is it re-entrant?

This comment has been minimized.

@dzajkowski

dzajkowski Jan 31, 2019

Collaborator

it acquries the lock for itself:

  def getRepresentation: F[BlockDagRepresentation[F]] =
    for {
      _              <- lock.acquire
      latestMessages <- latestMessagesRef.get
      childMap       <- childMapRef.get
      dataLookup     <- dataLookupRef.get
      topoSort       <- topoSortRef.get
      sortOffset     <- sortOffsetRef.get
      _              <- lock.release
    } yield FileDagRepresentation(latestMessages, childMap, dataLookup, topoSort, sortOffset)

This comment has been minimized.

@itegulov

itegulov Jan 31, 2019

Collaborator

@KentShikama no, the lock is not re-entrant. As @dzajkowski suggested, what you can do for now is just move the current implementation of getRepresentation into a private method which won't take any locks (while keeping the current method which will take locks and call the private version) and you will be able to compose it without having to worry about locks.

Show resolved Hide resolved casper/src/main/scala/coop/rchain/casper/util/ProtoUtil.scala
Show resolved Hide resolved casper/src/main/scala/coop/rchain/casper/SafetyOracle.scala Outdated
@lukasz-golebiewski

This comment has been minimized.

Copy link
Collaborator

lukasz-golebiewski commented Jan 30, 2019

I'd say it would also make sense to add some finer grained unit tests rather than relying on high-level HashSetCasperTest

@KentShikama

This comment has been minimized.

Copy link
Collaborator Author

KentShikama commented Jan 30, 2019

@lukasz-golebiewski On which functions would you like to see them?

@KentShikama

This comment has been minimized.

Copy link
Collaborator Author

KentShikama commented Jan 30, 2019

bors try

bors bot added a commit that referenced this pull request Jan 30, 2019

@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Jan 30, 2019

try

Build succeeded

@lukasz-golebiewski

This comment has been minimized.

Copy link
Collaborator

lukasz-golebiewski commented Jan 30, 2019

@KentShikama testing all would be ideal, but right now also an overkill. I'd suggest following the boy scout rule and starting off with testing one of the functions you've changed

@dzajkowski

This comment has been minimized.

Copy link
Collaborator

dzajkowski commented Jan 31, 2019

I think it should be possible to enable the "allow bonding in an existing network" test.

none[(Validator, Long)].pure[F]
}
} yield result
blockDag.latestMessageHash(validator).flatMap {

This comment has been minimized.

@dzajkowski

dzajkowski Jan 31, 2019

Collaborator

looks like OptionT would also work here.

This comment has been minimized.

@KentShikama

KentShikama Jan 31, 2019

Author Collaborator

I think this is simple enough not to warrant its use.

@dzajkowski
Copy link
Collaborator

dzajkowski left a comment

approving, minor remarks, please follow up on #blockchain channel

KentShikama added some commits Jan 31, 2019

@KentShikama

This comment has been minimized.

Copy link
Collaborator Author

KentShikama commented Jan 31, 2019

bors r+

bors bot added a commit that referenced this pull request Jan 31, 2019

Merge #2122
2122: Fix bug in clique oracle r=KentShikama a=KentShikama

The bug was an off by 1 error in filterChildren calculation 

I realized the seesAgreement checks are already covered
by the neverEventuallySeeDisagreement checks, so I remove those.

### JIRA ticket:
<sup>_Create it if there isn't one already._</sup>

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

### Notes
<sup>_Optional. Add any notes on caveats, approaches you tried that didn't work, or anything else._</sup>



### Please make sure that this PR:
- [x] is at most 200 lines of code (excluding tests),
- [x] meets [RChain development coding standards](https://rchain.atlassian.net/wiki/spaces/DOC/pages/28082177/Coding+Standards),
- [x] includes tests for all added features,
- [x] has a reviewer assigned,
- [x] has [all commits signed](https://rchain.atlassian.net/wiki/spaces/DOC/pages/498630673/How+to+sign+commits+to+rchain+rchain).

### [Bors](https://bors.tech/) 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.


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

This comment has been minimized.

Copy link
Contributor

bors bot commented Jan 31, 2019

@bors bors bot merged commit 0ec3d81 into rchain:dev Jan 31, 2019

2 checks passed

bors Build succeeded
Details
continuous-integration/drone/pr the build was successful
Details

@KentShikama KentShikama referenced this pull request Feb 1, 2019

Merged

Remove unnecessary getCreatorJustificationAsList func #2162

5 of 5 tasks complete

bors bot added a commit that referenced this pull request Feb 5, 2019

Merge #2162
2162: Remove unnecessary getCreatorJustificationAsList func r=KentShikama a=KentShikama

Just some simple refactoring work following from #2122 

### JIRA ticket:
<sup>_Create it if there isn't one already._</sup>

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

### Notes
<sup>_Optional. Add any notes on caveats, approaches you tried that didn't work, or anything else._</sup>



### Please make sure that this PR:
- [x] is at most 200 lines of code (excluding tests),
- [x] meets [RChain development coding standards](https://rchain.atlassian.net/wiki/spaces/DOC/pages/28082177/Coding+Standards),
- [x] includes tests for all added features,
- [x] has a reviewer assigned,
- [x] has [all commits signed](https://rchain.atlassian.net/wiki/spaces/DOC/pages/498630673/How+to+sign+commits+to+rchain+rchain).

### [Bors](https://bors.tech/) 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.


Co-authored-by: Kent Shikama <kent@kentshikama.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment