foldBlocks update #350

Jimbo4350 · 2023-10-31T19:41:36Z

Changelog

- description: |
    Modify foldBlocks to allow the fold to terminate from the accumulator via the FoldStatus type.
    
    Modify foldBlocks to accumulate the chain tip rather than only immutable
    blocks (blocks that are k blocks away from the tip).
    
    Add debug mode to foldBlocks which forces it to error with information 
    about ledger states, client and server tip, number of requests in flight
    and the current IORef state.
# uncomment types applicable to the change:
  type:
  - feature        # introduces a new feature
  - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - improvement    # QoL changes e.g. refactoring
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

Additional context for the PR goes here. If the PR fixes a particular issue please provide a link to the issue.

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

Checklist

Commit sequence broadly makes sense and commits have useful messages
New tests are added if needed and existing tests are updated. See Running tests for more details
Self-reviewed the diff

cardano-api/internal/Cardano/Api/LedgerState.hs

carbolymer · 2023-11-02T11:54:25Z

cardano-api/internal/Cardano/Api/LedgerState.hs

+                                                                           , "k: " <> show (envSecurityParam env)
+                                                                           , "Current IORef State: " <> show currentIORefState
+                                                                           ]
+                          clientIdle_DoneNwithMaybeError n ioRefErr


I'm not sure how soon this gets logged/written somewhere, but it may be worth enforcing strictness here to avoid accumulating thunks

Suggested change

clientIdle_DoneNwithMaybeError n ioRefErr

deepseq ioRefErr $ clientIdle_DoneNwithMaybeError n ioRefErr

So this would be ideal but I would need to derive NFData instances for types in consensus. In a follow up PR I will see if I can easily create some orphan instances.

Right. I guess we can get around using NFData with BangPatterns:

Make DebugError constructor field strict

use let !ioRefError = DebugError . force $ ... - this will evaluate whole expression, without the need for NFData orphans

wrap ioRefErr in Just when calling clientIdle_DoneNwithMaybeError, because Maybe is lazy

https://github.com/input-output-hk/cardano-api/pull/350/files#r1381956300

carbolymer

https://github.com/input-output-hk/cardano-api/pull/350/files#r1379986767

...

carbolymer · 2023-11-03T14:07:49Z

cardano-api/internal/Cardano/Api/LedgerState.hs

@@ -238,13 +240,15 @@ data LedgerStateError
  -- ^ Encountered a rollback larger than the security parameter.
      SlotNo     -- ^ Oldest known slot number that we can roll back to.
      ChainPoint -- ^ Rollback was attempted to this point.
+  | DebugError String


Suggested change

| DebugError String

| DebugError !String

specified in the accumulator via the FoldStatus type Modify foldBlocks to accumulate the chain tip rather than only immutable blocks (blocks that are k blocks away from the tip) Add debug mode to foldBlocks which forces it to error with information about ledger states, client and server tip, number of requests in flight and the current IORef state

carbolymer · 2023-11-03T16:32:34Z

cardano-api/internal/Cardano/Api/LedgerState.hs

+                          let ioRefErr = Just . DebugError
+                                              $ unlines [ "newClientTip: " <> show newClientTip
+                                                        , "newServerTip: " <> show newServerTip
+                                                        , "newLedgerState: " <> show (snd newLedgerState)
+                                                        , "knownLedgerStates: " <> show (extractHistory knownLedgerStates)
+                                                        , "committedStates: " <> show (extractHistory committedStates)
+                                                        , "numberOfRequestsInFlight: " <> show n
+                                                        , "k: " <> show (envSecurityParam env)
+                                                        , "Current IORef State: " <> show currentIORefState
+                                                        ]
+                          clientIdle_DoneNwithMaybeError n ioRefErr


Suggested change

let ioRefErr = Just . DebugError

$ unlines [ "newClientTip: " <> show newClientTip

, "newServerTip: " <> show newServerTip

, "newLedgerState: " <> show (snd newLedgerState)

, "knownLedgerStates: " <> show (extractHistory knownLedgerStates)

, "committedStates: " <> show (extractHistory committedStates)

, "numberOfRequestsInFlight: " <> show n

, "k: " <> show (envSecurityParam env)

, "Current IORef State: " <> show currentIORefState

]

clientIdle_DoneNwithMaybeError n ioRefErr

let !ioRefErr = DebugError . force

$ unlines [ "newClientTip: " <> show newClientTip

, "newServerTip: " <> show newServerTip

, "newLedgerState: " <> show (snd newLedgerState)

, "knownLedgerStates: " <> show (extractHistory knownLedgerStates)

, "committedStates: " <> show (extractHistory committedStates)

, "numberOfRequestsInFlight: " <> show n

, "k: " <> show (envSecurityParam env)

, "Current IORef State: " <> show currentIORefState

]

clientIdle_DoneNwithMaybeError n (Just ioRefErr)

cardano-api/internal/Cardano/Api/LedgerState.hs

Do not allow submitting transactions older than current node era

Jimbo4350 force-pushed the jordan/foldblocks-update branch 4 times, most recently from 4b9a087 to 08fd859 Compare November 1, 2023 14:50

Jimbo4350 marked this pull request as ready for review November 1, 2023 14:51

Jimbo4350 requested review from dcoutts, erikd, newhoggy, carbolymer, CarlosLopezDeLara and disassembler as code owners November 1, 2023 14:51

Jimbo4350 force-pushed the jordan/foldblocks-update branch from 08fd859 to a88ad73 Compare November 1, 2023 14:59

Jimbo4350 mentioned this pull request Nov 1, 2023

Initial ledger events integration into cardano-testnet IntersectMBO/cardano-node#5523

Merged

9 tasks

carbolymer reviewed Nov 2, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/LedgerState.hs Outdated Show resolved Hide resolved

carbolymer reviewed Nov 2, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/LedgerState.hs Outdated Show resolved Hide resolved

carbolymer reviewed Nov 2, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/LedgerState.hs Outdated Show resolved Hide resolved

carbolymer reviewed Nov 2, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/LedgerState.hs Show resolved Hide resolved

carbolymer reviewed Nov 2, 2023

View reviewed changes

carbolymer previously requested changes Nov 2, 2023

View reviewed changes

Jimbo4350 force-pushed the jordan/foldblocks-update branch 2 times, most recently from 54a634f to f3887a4 Compare November 3, 2023 14:05

carbolymer reviewed Nov 3, 2023

View reviewed changes

Jimbo4350 force-pushed the jordan/foldblocks-update branch from f3887a4 to e9494bc Compare November 3, 2023 14:09

carbolymer reviewed Nov 3, 2023

View reviewed changes

carbolymer reviewed Nov 6, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/LedgerState.hs Outdated Show resolved Hide resolved

Review suggestion: fully evaluate error to avoid thunks

cc33aa3

Jimbo4350 force-pushed the jordan/foldblocks-update branch from 17e9f44 to cc33aa3 Compare November 6, 2023 12:55

carbolymer approved these changes Nov 6, 2023

View reviewed changes

Jimbo4350 added this pull request to the merge queue Nov 6, 2023

Merged via the queue into main with commit be8142d Nov 6, 2023
20 checks passed

Jimbo4350 deleted the jordan/foldblocks-update branch November 6, 2023 14:46

newhoggy pushed a commit that referenced this pull request Mar 11, 2024

Merge pull request #350 from input-output-hk/newhoggy/avoid-EraCast

8624c31

Do not allow submitting transactions older than current node era

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

foldBlocks update #350

foldBlocks update #350

Jimbo4350 commented Oct 31, 2023 •

edited

Loading

carbolymer Nov 2, 2023 •

edited

Loading

Jimbo4350 Nov 3, 2023 •

edited

Loading

carbolymer Nov 3, 2023 •

edited

Loading

carbolymer left a comment

carbolymer Nov 3, 2023

carbolymer Nov 3, 2023 •

edited

Loading

	clientIdle_DoneNwithMaybeError n ioRefErr
	deepseq ioRefErr $ clientIdle_DoneNwithMaybeError n ioRefErr

foldBlocks update #350

foldBlocks update #350

Conversation

Jimbo4350 commented Oct 31, 2023 • edited Loading

Changelog

Context

How to trust this PR

Checklist

carbolymer Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

carbolymer Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

carbolymer left a comment

Choose a reason for hiding this comment

carbolymer Nov 3, 2023

Choose a reason for hiding this comment

carbolymer Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 commented Oct 31, 2023 •

edited

Loading

carbolymer Nov 2, 2023 •

edited

Loading

Jimbo4350 Nov 3, 2023 •

edited

Loading

carbolymer Nov 3, 2023 •

edited

Loading

carbolymer Nov 3, 2023 •

edited

Loading