cardano-testnet | Remove redundant `watchEpochStateUpdate` by carbolymer · Pull Request #6535 · IntersectMBO/cardano-node

carbolymer · 2026-04-15T14:36:07Z

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 Running 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-9.6 and ghc-9.12
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.

palas

Very nice simplification 👍

There is a lot of small nuance in some changes, and probably most of it doesn't really make a difference in practice, so I think we can ignore it.

I pointed out a couple of small things that may be worth changing or double-checking

palas · 2026-04-16T18:59:12Z


+-- | Core retry loop. Repeats the action every 300ms until it returns 'Right'
+-- or the timeout is reached, in which case the last 'Left' is returned.
+retryByTimeout


I think the name is misleading. To me, retryByTimeout suggests that it will retry once when the timeout hits. Maybe retryUntilTimeout instead?

retryUntil is better but there is already retryUntilM and they are both monadic... If what we want is making them easier to differentiate, I would focus on naming the difference, maybe simpleRetryUntilM or eitherRetryUntilM or something like that.

I think I'll go with retryUntilRightM

palas · 2026-04-16T19:13:37Z

+  -> m (Either e a)
+retryByTimeout esv timeout act = withFrozenCallStack $ do
+  startingValue <- getCurrentValue
+  go $ startingValue + timeoutW64


Nice, I was thinking about this recalculation before but I didn't want to mention it

palas · 2026-04-16T19:55:52Z

+  retryUntilM epochStateView (WaitForBlocks numberOfBlocks)
+    (getBlockNumber epochStateView)
+    (\(BlockNo bn) -> bn >= startingBlockNumber + numberOfBlocks)


I think it is not very nice to have both the timeout and the action counting blocks, they may get out of sync too. I think it would be better to use retryByTimeout and rely on the timeout only, making the action always Left =<< getBlockNumber epochStateView, or Left () and then getting the block number afterwards.

Same as in waitForEpochs

Good point.

palas · 2026-04-16T20:20:28Z

-  mGovernanceActionTxIx H.=== Nothing
+  void $ retryUntilM epochStateView (WaitForEpochs $ EpochInterval 2)
+    (maybeExtractGovernanceActionIndex governanceActionTxId <$> getEpochState epochStateView)
+    isNothing


Apparently, this is weakening the test, because it was originally testing that:

The action is not available at the beginning of the call to watchEpochStateUpdate

The action remained unavailable during two epochs

But I am not sure the later was intentional.

On the other hand, line 136 was clearly waiting for the proposal to expire. And the new usage of retryUntilM is allowing for that to happen 2 epochs later potentially, so that is not good.

We could either merge this with the line 136 and reduce the timeout to adjust it to the original expectation. Or if we think that verifying that "the action does not reappear for two extra epochs" is important, we would need to invert this retrying and make the timeout the success

Good catch.

it was originally testing that (...) The action remained unavailable during two epochs

Looks like a double-check to me to deal with rollbacks. I don't expect rollbacks in one SPO testnets, but we can add a one epoch wait for a good measure, in case rollback happens on epoch boundary.

palas

Just one comment for clarification and one thing that I think is weird in the test gov action test. Otherwise, looking good 👍

palas · 2026-04-21T19:57:52Z

      | ti1 == L.extractHash ti2 = Just gai
    compareWithTxId _ x _ _ = x

+-- | Look up the @gasExpiresAfter@ epoch for the governance action submitted


I was super confused until I found out that gas meant GovActionState. Maybe it would be good to say that, for example say "look up the expiration epoch in the governance action state, through the gasExpiresAfter lens", or something like that

I've renamed the function parameter

Jimbo4350

Approved based on the work in: github.com//pull/6538

carbolymer force-pushed the mgalazyn/refactor/remove-watch-epoch-state-update branch 7 times, most recently from c076427 to 6bd49c2 Compare April 16, 2026 12:49

carbolymer self-assigned this Apr 16, 2026

carbolymer marked this pull request as ready for review April 16, 2026 12:50

carbolymer requested a review from a team as a code owner April 16, 2026 12:50

Base automatically changed from mgalazyn/fix/testnet-make-conditions-wait-longer to master April 16, 2026 14:45

palas approved these changes Apr 16, 2026

View reviewed changes

carbolymer force-pushed the mgalazyn/refactor/remove-watch-epoch-state-update branch 2 times, most recently from 82f0f20 to 75aece7 Compare April 21, 2026 07:47

palas approved these changes Apr 21, 2026

View reviewed changes

carbolymer force-pushed the mgalazyn/refactor/remove-watch-epoch-state-update branch from 75aece7 to 47aa730 Compare April 22, 2026 12:06

Jimbo4350 self-requested a review April 23, 2026 15:00

Jimbo4350 approved these changes Apr 23, 2026

View reviewed changes

Remove watchEpochStateUpdate. Simplify retryUntilJustM.

344c313

carbolymer force-pushed the mgalazyn/refactor/remove-watch-epoch-state-update branch from 47aa730 to 344c313 Compare April 24, 2026 06:57

carbolymer enabled auto-merge April 24, 2026 06:58

carbolymer added this pull request to the merge queue Apr 24, 2026

carbolymer removed this pull request from the merge queue due to a manual request Apr 24, 2026

carbolymer merged commit 3a12571 into master Apr 24, 2026
29 checks passed

carbolymer deleted the mgalazyn/refactor/remove-watch-epoch-state-update branch April 24, 2026 08:16

Conversation

carbolymer commented Apr 15, 2026

Checklist

Note on CI

Uh oh!

palas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palas Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jimbo4350 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

palas Apr 16, 2026 •

edited

Loading