RCHAIN-993 unit test showing state not catching up with a fix. #2060

dzajkowski · 2019-01-02T19:42:13Z

Overview

Introduces a unit test that shows that a change in how the blocks are added to the 2-way graph fixes a catch up problem.

essentially the fix boils down to:
if a block is a parent of 2 block then this fact needs to make its way to blockBufferDependencyDag. it is not sufficient to hold only one of the dependencies (out of potentially many children).
The clash happens when a parent block is also a justification for a younger block (c2 and f2).

JIRA ticket:

https://rchain.atlassian.net/projects/RCHAIN/issues/RCHAIN-993

Notes

none really. it's a step in identifying a bug that's been in the project for over 4 months.

Please make sure that this PR:

is at most 200 lines of code (excluding tests),
meets RChain development coding standards,
includes tests for all added features,
has a reviewer assigned,
has all commits signed.

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 · 2019-01-03T08:33:07Z

casper/src/test/scala/coop/rchain/casper/HashSetCasperTest.scala

+      for {
+        _ <- nodes(0).receive()
+        _ <- nodes(1).receive()
+        _ <- nodes(2).receive() //nodes(2) gets the 5th block and starts requesting the dag


This tests uses a mocked version of the transport layer. When you call receive on a node, it takes things off its mocked network queue and then processes them. If you want to simulate the process of node A asking for a block from node B and having node B send it back, you have to do something like

// node A sends block request nodeB.receive() // node B receives block request and sends the block nodeA.receive() // node A receives block

that's why it's done 20 times - to let the network do its thing.

I see; so in theory you only need to do like 8~9 times but you just do it 20 times to be sure?

in theory yes BUT:
I've observed a funny "self healing" mechanism - if the network layer holds enough requested blocks it will make the catch up mechanism look like it works (it processes stale requested blocks). This way I'm making sure all the blocks are processed.

lukasz-golebiewski · 2019-01-03T11:23:33Z

casper/src/test/scala/coop/rchain/casper/HashSetCasperTest.scala

+
+      br <- deploy(nodes(0), deployDatasFs(0).apply())
+
+      _ <- propagate(nodes)


Do you think it would make sense to put those propagates into a collection and traverse it?

dzajkowski · 2019-01-03T15:25:33Z

bors try

bors · 2019-01-03T16:07:59Z

try

Build succeeded

continuous-integration/drone/push

dzajkowski · 2019-01-03T16:57:44Z

bors try

bors · 2019-01-03T17:43:20Z

try

Build succeeded

continuous-integration/drone/push

lukasz-golebiewski

LGTM

KentShikama · 2019-01-04T02:58:14Z

casper/src/test/scala/coop/rchain/casper/HashSetCasperTest.scala

+
+      br <- deploy(nodes(0), deployDatasFs(0).apply())
+
+      _ <- List.fill(22)(propagate(nodes)).sequence//make the network chooch


What does chooch mean? No results on my dictionary.

KentShikama · 2019-01-04T03:00:11Z

casper/src/test/scala/coop/rchain/casper/HashSetCasperTest.scala

+      for {
+        _ <- nodes(0).receive()
+        _ <- nodes(1).receive()
+        _ <- nodes(2).receive() //nodes(2) gets the 5th block and starts requesting the dag


I see; so in theory you only need to do like 8~9 times but you just do it 20 times to be sure?

KentShikama · 2019-01-04T03:10:04Z

Nice catch. Thanks for cleaning up after me; I always had a single parent block model in my head to simplify things but it backfired here.

KentShikama

Wait nvm this PR has nothing to do with multiple parents

KentShikama · 2019-01-04T03:27:23Z

casper/src/main/scala/coop/rchain/casper/MultiParentCasperImpl.scala

-      _ <- CommUtil.sendBlockRequest[F](BlockRequest(Base16.encode(hash.toByteArray), hash))
+        blockBufferDependencyDag =>
+          DoublyLinkedDagOperations
+            .add[BlockHash](blockBufferDependencyDag, hash, childBlock.blockHash)


blockBufferDependencyDag represents blocks that aren't in the block buffer yet. If the block is in the block buffer why add the block to the blockBufferDependencyDag?

because it can be a missing parent of a block that's already been added to blockBuffer.

IOW two blocks can have one parent.

when the "missing blocks" status is being resolved for a block - this block will be added to the blockBufferDependencyDag (with its parents) and to the block buffer.

the blockBufferDependencyDag is used in reattempt to get the dependencyFree blocks.

now if a block is already in blockBuffer but it's a parent of another block (which was requested but not yet processed) then it will never be added to blockBufferDependencyDag (because it's in the blockBuffer).
This results in the processing getting "stuck" - the blockBuffer holds a block that should be processed (so it's not allowing new blocks via the network) and the internal "catch up" mechanism isn't aware that the second block has a parent that is already available and should be added (hence blocking the catch up process).

Oh whoops; I see my mistake... essentially blockBufferDependencyDag represents blocks that aren't in the block dag. And so blocks in the block buffer don't count.

KentShikama · 2019-01-04T03:35:06Z

But I'll have to think a bit more. I don't quite understand why this change makes the tests pass.

KentShikama · 2019-01-04T03:41:47Z

casper/src/test/scala/coop/rchain/casper/HashSetCasperTest.scala

@@ -1057,6 +1057,78 @@ class HashSetCasperTest extends FlatSpec with Matchers {
    } yield result
  }

+  it should "ask peers for blocks it is missing and add them" in effectTest {


Is it possible to draw out a diagram of what this test looks like?

Ignore this unless you've already started. I quickly drew it out.

Well thanks anyways

KentShikama

Actually one small thing before you merge: can you rename the blockBufferDependencyDag to dependencyDag (or something else) as it represents blocks that are both missing and in the block buffer?

dzajkowski · 2019-01-04T11:27:04Z

bors r+

2060: RCHAIN-993 unit test showing state not catching up with a fix. r=dzajkowski a=dzajkowski Co-authored-by: Dominik Zajkowski <dzajkowski@invisibl.es>

bors · 2019-01-04T12:24:45Z

Build failed

continuous-integration/drone/push

dzajkowski · 2019-01-04T12:33:08Z

bors r+

2060: RCHAIN-993 unit test showing state not catching up with a fix. r=dzajkowski a=dzajkowski Co-authored-by: Dominik Zajkowski <dzajkowski@invisibl.es>

bors · 2019-01-04T13:58:46Z

Build succeeded

continuous-integration/drone/push

RCHAIN-993 unit test showing state not catching up.

3112176

dzajkowski requested a review from lukasz-golebiewski January 2, 2019 19:56

KentShikama reviewed Jan 3, 2019

View reviewed changes

lukasz-golebiewski reviewed Jan 3, 2019

View reviewed changes

RCHAIN-993 Fix for blocks going missing on catch up.

f80f883

dzajkowski changed the title ~~RCHAIN-993 unit test showing state not catching up.~~ RCHAIN-993 unit test showing state not catching up with a fix. Jan 3, 2019

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

Try #2060:

25eef36

RCHAIN-993 Cleanup unit test network simulation.

2652ff2

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

Try #2060:

7b83035

lukasz-golebiewski approved these changes Jan 3, 2019

View reviewed changes

RCHAIN-993 Dropped a named variable in unit test.

bbb3df6

KentShikama reviewed Jan 4, 2019

View reviewed changes

KentShikama approved these changes Jan 4, 2019

View reviewed changes

KentShikama requested changes Jan 4, 2019

View reviewed changes

KentShikama reviewed Jan 4, 2019

View reviewed changes

RCHAIN-993 Applied PR remarks regarding comments.

1aefe03

KentShikama approved these changes Jan 4, 2019

View reviewed changes

KentShikama reviewed Jan 4, 2019

View reviewed changes

dzajkowski added 2 commits January 4, 2019 12:17

RCHAIN-993 Added remark about justifications.

3a3b8bd

RCHAIN-993 Renamed blockBufferDependencyDag to dependencyDag.

fb0b7c2

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

Merge #2060

80c00d0

2060: RCHAIN-993 unit test showing state not catching up with a fix. r=dzajkowski a=dzajkowski Co-authored-by: Dominik Zajkowski <dzajkowski@invisibl.es>

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

Merge #2060

0a8f863

2060: RCHAIN-993 unit test showing state not catching up with a fix. r=dzajkowski a=dzajkowski Co-authored-by: Dominik Zajkowski <dzajkowski@invisibl.es>

bors bot merged commit fb0b7c2 into rchain:dev Jan 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RCHAIN-993 unit test showing state not catching up with a fix. #2060

RCHAIN-993 unit test showing state not catching up with a fix. #2060

dzajkowski commented Jan 2, 2019 •

edited

KentShikama Jan 3, 2019

dzajkowski Jan 3, 2019

KentShikama Jan 4, 2019 •

edited

dzajkowski Jan 4, 2019

lukasz-golebiewski Jan 3, 2019

ArturGajowy Jan 3, 2019

dzajkowski commented Jan 3, 2019

bors bot commented Jan 3, 2019

dzajkowski commented Jan 3, 2019

bors bot commented Jan 3, 2019

lukasz-golebiewski left a comment

KentShikama Jan 4, 2019

KentShikama Jan 4, 2019 •

edited

KentShikama commented Jan 4, 2019

KentShikama left a comment

KentShikama Jan 4, 2019

dzajkowski Jan 4, 2019

KentShikama Jan 4, 2019

KentShikama commented Jan 4, 2019

KentShikama Jan 4, 2019

KentShikama Jan 4, 2019

KentShikama Jan 4, 2019

KentShikama left a comment •

edited

dzajkowski commented Jan 4, 2019

bors bot commented Jan 4, 2019

dzajkowski commented Jan 4, 2019

bors bot commented Jan 4, 2019


		br <- deploy(nodes(0), deployDatasFs(0).apply())

		_ <- propagate(nodes)


		br <- deploy(nodes(0), deployDatasFs(0).apply())

		_ <- List.fill(22)(propagate(nodes)).sequence//make the network chooch

RCHAIN-993 unit test showing state not catching up with a fix. #2060

RCHAIN-993 unit test showing state not catching up with a fix. #2060

Conversation

dzajkowski commented Jan 2, 2019 • edited

Overview

JIRA ticket:

Notes

Please make sure that this PR:

Bors cheat-sheet:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KentShikama Jan 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzajkowski commented Jan 3, 2019

bors bot commented Jan 3, 2019

try

Build succeeded

dzajkowski commented Jan 3, 2019

bors bot commented Jan 3, 2019

try

Build succeeded

lukasz-golebiewski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KentShikama Jan 4, 2019 • edited

Choose a reason for hiding this comment

KentShikama commented Jan 4, 2019

KentShikama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KentShikama commented Jan 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KentShikama left a comment • edited

Choose a reason for hiding this comment

dzajkowski commented Jan 4, 2019

bors bot commented Jan 4, 2019

Build failed

dzajkowski commented Jan 4, 2019

bors bot commented Jan 4, 2019

Build succeeded

dzajkowski commented Jan 2, 2019 •

edited

KentShikama Jan 4, 2019 •

edited

KentShikama Jan 4, 2019 •

edited

KentShikama left a comment •

edited