Use waiting for blocks instead epochs, when waiting for new UTXOs #5843

carbolymer · 2024-05-16T17:02:30Z

Description

This PR provides new functions allowing to extract slot number and block number from epoch state view, and waiting for new blocks. This cuts down the test execution time a bit. Waiting for an epoch in a plutus test takes ~70s, where one block wait is around 20s.

Checklist

Commit sequence broadly makes sense and commits have useful messages
New tests are added if needed and existing tests are updated. These may include:
- golden tests
- property tests
- roundtrip tests
- integration tests
  See Runnings tests for more details
Any changes are noted in the CHANGELOG.md for affected package
The version bounds in .cabal files are updated
CI passes. See note on CI. The following CI checks are required:
- Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
- Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
- Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.

cardano-testnet/src/Testnet/Components/Query.hs

carbolymer · 2024-05-17T11:39:06Z

cardano-testnet/src/Testnet/Components/Query.hs

-            if currentEpoch > timeout
-              then pure Nothing
-              else do
-                H.threadDelay 10_000


@palas why we were using 10ms polling interval?

Feel free to change it. My reasoning was, computers do more than 10^9 operations per second so doing 100 polls per second seems that it is not a lot of processing wasted waiting (less than 0.00001%), and at the same time the latency is almost imperceptible for humans (100th of a second)

Jimbo4350

Nice work. I am approving but I would like this comment addressed before merging: #5843 (comment)

Jimbo4350 · 2024-05-17T14:09:32Z

cardano-testnet/src/Testnet/Components/Query.hs

+    watchEpochStateUpdate epochStateView (EpochInterval maxBound) $ \(_, _, BlockNo blockNumber) ->
+      pure $
+        if blockNumber >= startingBlockNumber + numberOfBlocks
+        then Just blockNumber


Not for this PR but we might want to consider a sum type to make it clear that Nothing means we will recurse.

#5843 (comment)

cardano-testnet/src/Testnet/EpochStateProcessing.hs

Jimbo4350 · 2024-05-17T15:57:32Z

cardano-testnet/src/Testnet/Property/Util.hs

@@ -75,3 +80,27 @@ runInBackground act = void . H.evalM $ allocate (H.async act) cleanUp

 decodeEraUTxO :: (IsShelleyBasedEra era, MonadTest m) => ShelleyBasedEra era -> Aeson.Value -> m (UTxO era)
 decodeEraUTxO _ = H.jsonErrorFail . Aeson.fromJSON
+
+-- TODO upstream to hedgehog-extras
+waitForJustM


findLargestUtxoWithAddress should recurse until an input is found or we should expose a separate function that does this.

I want to discourage "helper" functions like that that allow us introduce additional threadDelay's.

Do you mean, to make a waiting and looping an implementation detail? But then functions like findLargestUtxoWithAddress would require a timeout value as an input additionally - which complicates things a bit. How do you find the correct timeout value?

After considering it for a while I think we should also measure time differently - in blocks, slots, and epochs instead.

After considering it for a while I think we should also measure time differently - in blocks, slots, and epochs instead.

Agreed. In this instance we can wait 2/3 blocks which should be enough time for the tx input to become available. The point is we need to continue to stay away from threadDelay as much as possible or move it to "lower level" functions.

Edit: k blocks would be better for small k. However we can actually enforce this at the foldEpochState level which will eliminate test flakiness due to rollbacks.

You can't really get rid of threadDelays because we're using polling in waiting.

Jimbo4350 · 2024-05-17T15:59:59Z

cardano-testnet/test/cardano-testnet-test/Cardano/Testnet/Test/Gov/DRepActivity.hs

@@ -203,13 +201,14 @@ activityChangeProposalTest execConfig epochStateView ceo work prefix

  void $ waitForEpochs epochStateView minWait


Add a TODO here to explain why we are waiting.

Added a comments with explanation. Why TODO? What's left to do here?

Jimbo4350 · 2024-05-17T16:04:49Z

cardano-testnet/src/Testnet/Components/Query.hs

-                         ]
+assertNewEpochState epochStateView sbe maxWait lens expected = withFrozenCallStack $ do
+  mStateView <- watchEpochStateUpdate epochStateView maxWait (const checkEpochState)
+  when (isNothing mStateView) $ do


watchEpochStateUpdate returns Nothing when the timeout has been exceeded (i.e the value of interest is not present in the ledger state). It's like we are checking "one last time here" which I'm not sure makes sense or is slightly misleading. Even more reason to not return a Maybe but instead a sum type that clearly says the value hasn't been found within the max wait period.

Technically, the epoch state could change between watchEpochStateUpdate and getFromEpochStateForEra, so we could see different values. We're logging observed value so we need to call that function anyway - so doing check again won't hurt.

This highlights that actually a product type isomorphic to (Bool, a) would be the best suited for our watching functions. We could return the current value and conditionally break the loop.

Technically, the epoch state could change between watchEpochStateUpdate and getFromEpochStateForEra, so we could see different values. We're logging observed value so we need to call that function anyway - so doing check again won't hurt.

Add a comment making this clear.

Co-authored-by: Pablo Lamela <[email protected]> Review remarks part 1

carbolymer force-pushed the mgalazyn/test/wait-for-blocks-using-epochstateview-polling branch 3 times, most recently from fb26618 to ba4aae4 Compare May 16, 2024 17:29

carbolymer marked this pull request as ready for review May 16, 2024 17:32

carbolymer requested a review from a team as a code owner May 16, 2024 17:32

carbolymer mentioned this pull request May 16, 2024

cardano-testnet: Waiting for blocks using EpochStateView. #5836

Closed

9 tasks

carbolymer changed the title ~~Use waiting for blocks instead epochs, when waiting for transaction~~ Use waiting for blocks instead epochs, when waiting for new UTXOs May 16, 2024

smelc approved these changes May 17, 2024

View reviewed changes

cardano-testnet/src/Testnet/Components/Query.hs Outdated Show resolved Hide resolved

carbolymer force-pushed the mgalazyn/test/wait-for-blocks-using-epochstateview-polling branch 3 times, most recently from a142218 to bd3e138 Compare May 17, 2024 11:27

carbolymer commented May 17, 2024

View reviewed changes

Jimbo4350 approved these changes May 17, 2024

View reviewed changes

carbolymer force-pushed the mgalazyn/test/wait-for-blocks-using-epochstateview-polling branch from d213753 to b4e6c9c Compare May 23, 2024 10:42

Use waiting for blocks instead epochs, when waiting for transaction

fcd06be

Co-authored-by: Pablo Lamela <[email protected]> Review remarks part 1

carbolymer force-pushed the mgalazyn/test/wait-for-blocks-using-epochstateview-polling branch from b4e6c9c to fcd06be Compare May 24, 2024 11:26

carbolymer enabled auto-merge May 24, 2024 11:27

carbolymer added this pull request to the merge queue May 24, 2024

Merged via the queue into master with commit 2770221 May 24, 2024
24 checks passed

carbolymer deleted the mgalazyn/test/wait-for-blocks-using-epochstateview-polling branch May 24, 2024 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use waiting for blocks instead epochs, when waiting for new UTXOs #5843

Use waiting for blocks instead epochs, when waiting for new UTXOs #5843

carbolymer commented May 16, 2024

carbolymer May 17, 2024

palas May 17, 2024

Jimbo4350 left a comment •

edited

Loading

Jimbo4350 May 17, 2024

carbolymer May 23, 2024

Jimbo4350 May 17, 2024

carbolymer May 23, 2024 •

edited

Loading

Jimbo4350 May 23, 2024 •

edited

Loading

carbolymer May 24, 2024

Jimbo4350 May 17, 2024 •

edited

Loading

carbolymer May 22, 2024 •

edited

Loading

Jimbo4350 May 17, 2024

carbolymer May 23, 2024

carbolymer May 23, 2024 •

edited

Loading

Jimbo4350 May 23, 2024

carbolymer May 24, 2024

		@@ -203,13 +201,14 @@ activityChangeProposalTest execConfig epochStateView ceo work prefix

		void $ waitForEpochs epochStateView minWait

Use waiting for blocks instead epochs, when waiting for new UTXOs #5843

Use waiting for blocks instead epochs, when waiting for new UTXOs #5843

Conversation

carbolymer commented May 16, 2024

Description

Checklist

Note on CI

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 left a comment • edited Loading

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

carbolymer May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 May 17, 2024 • edited Loading

Choose a reason for hiding this comment

carbolymer May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbolymer May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 left a comment •

edited

Loading

carbolymer May 23, 2024 •

edited

Loading

Jimbo4350 May 23, 2024 •

edited

Loading

Jimbo4350 May 17, 2024 •

edited

Loading

carbolymer May 22, 2024 •

edited

Loading

carbolymer May 23, 2024 •

edited

Loading