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

Add fix and test for foldBlocks #4627

Closed
wants to merge 4 commits into from

Conversation

eyeinsky
Copy link
Contributor

@eyeinsky eyeinsky commented Nov 9, 2022

Creating this draft pull request in relation to the foldBlocks issue here #4624.

Due to adding genesis hashes to node config file (72e6017) cardano-testnet now does start (without adding the hashes it exceptioned out as described in the issue), but foldBlocks still doesn't receive ledger states and hangs indefinitely.

(Should I break out the 72e6017 to a separate PR so we could merge that?)

@eyeinsky
Copy link
Contributor Author

Cardano testnet now succesfully starts and foldBlocks test (occasionally) succeeds.

I say occasionally because the test is flaky: testnet nodes do start but quite frequently in the test runs they don't start posting anything to the local chain sync protocol and the test fails with a timeout.

The good thing is that the config files now allow the testnet to start, but why the socket is not written to remains to be researched.

Another thing that might be good to pursue is that while monitoring linux processes, the cardano node's start relatively early in the test, but the local chainsync socket starts to receive updates ~20 seconds in -- is that time required, e.g spending it on reaching consensus, or could it be cut much shorter? It would greatly benefit the convenience of testing if that time could be cut shorter.

@eyeinsky eyeinsky force-pushed the ml/foldblocks-test branch 6 times, most recently from 703f476 to fcda2fa Compare November 16, 2022 11:11
@eyeinsky
Copy link
Contributor Author

eyeinsky commented Nov 16, 2022

This PR is now ready for review.

In addition to amending the node configuration file, there were two other issues that made this test flaky:

  • foldBlocks doesn't fold received blocks forever but returns when latest block (or error) is reached. Sometimes this happened before any permanent blocks => permanent ledger states have been created (foldBlocks only emits permanent ledger states). We run foldBlocks in a loop to overcome this.
  • The chain is not extended fast enough for the two node/one pool testnet configuration that is created by default, changing the activeSlotsCoeff from 0.2 to 0.9 fixes this.

@eyeinsky eyeinsky marked this pull request as ready for review November 16, 2022 11:18
@eyeinsky eyeinsky changed the title Add test for foldBlocks Add fix and test for foldBlocks Nov 17, 2022
cardano-testnet/src/Testnet/Cardano.hs Outdated Show resolved Hide resolved
cardano-testnet/test/Main.hs Outdated Show resolved Hide resolved
cardano-testnet/test/Test/FoldBlocks.hs Outdated Show resolved Hide resolved
either (throw . FoldBlocksException) (\_ -> pure ()) e
link a -- Throw async thread's exceptions in main thread

_ <- liftIO $ IO.readMVar lock
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain further what this test is testing? Specifically wrt the put/readMVar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The MVar is written to from within the handler that is passed to foldBlocks, it simply tests that a ledger state is received and the handler is called (which writes to the lock and allows the test to finish).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right I understand. Can you include this in a comment? I'll approve after.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, will also add all fixes done here to the other PR as well (the one that's against the release/1.35 branch).

@eyeinsky eyeinsky mentioned this pull request Nov 29, 2022
3 tasks
@eyeinsky
Copy link
Contributor Author

Closing this PR in favor of this one: #4679

@eyeinsky eyeinsky closed this Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants