[4] Rollback diff - #333

JonoPrest · 2024-11-11T16:28:35Z

Sorry for all the changes in 1 PR, it was very difficult to get independent parts to work correctly without all the changes.

This PR finishes the code necessary to complete a rollback successfully from the new entity history tables.

It allows

generating a rollback diff,
rolling back fetch state to start each chain fetching again from where necessary
on the first batch processed rolling back the entity history

JonoPrest · 2024-11-14T10:30:38Z

In the next PR, I'm going to add a whole bunch more tests for entity history 👍🏼, and add some refactors including removing dead code from before.

JonoPrest · 2024-11-14T10:44:00Z

  ) => {
    let (optPreviousEventIdentifier, entityHistoryItems) = prev

-    let {eventIdentifier, shouldSaveHistory, entityUpdateAction, entityId} = entityUpdate


I've refactored entity history in the in memory table to simplify things (and exposed a bug).

The inMemTable would hold "latestItem" and "history", where history did not include latest item. The problem is that each item would need to keep track of whether it should be saved to history or not, and the latest item would need to be added to history when requesting history.

Now it does not need to keep track of that, when calling "set" purely based on the "shouldSaveHistory" flag it either adds to history or not. The only minor knock is that you have a duplicate value at the head of "history" and in the "latestItem" position.

JonoPrest · 2024-11-14T10:45:02Z

-      if !shouldSaveHistory ||
-      //Rollback initial state cases should not save history
-      !previous_values.latest.shouldSaveHistory ||
      // This prevents two db actions in the same event on the same entity from being recorded to the history table.
-      previous_values.latest.eventIdentifier == entityUpdate.eventIdentifier =>


This logic was not fully sound. It meant that if 2 updates came from the same event, the history item wold be that of the first and not the latest.

I don't know, but this looks wrong 😱

Clutchplay recently reported that they can't find an entity that is clearly set. I need to doublecheck whether it's related or not.

JonoPrest · 2024-11-14T10:46:43Z

    let {entityRow, entityIndices} = switch inMemTable.table->get(entityUpdate.entityId) {
-    | Some({entityRow: InitialReadFromDb(entity_read), entityIndices}) =>
-      let entityRow = Types.Updated({
-        initial: Retrieved(entity_read),


"initial" was not being used anywhere so I removed it.

JonoPrest · 2024-11-14T10:48:41Z

+module EntityTables = {
+  type t = dict<InMemoryTable.Entity.t<Entities.internalEntity>>
+  exception UndefinedEntity(string)
+  let make = (entities: array<module(Entities.InternalEntity)>): t => {
+    let init = Js.Dict.empty()
+    entities->Belt.Array.forEach(entity => {
+      let module(Entity) = entity
+      init->Js.Dict.set(Entity.key, InMemoryTable.Entity.make())
+    })
+    init
+  }
+
+  let get = (type entity, self: t, entityMod: module(Entities.Entity with type t = entity)) => {
+    let module(Entity) = entityMod
+    switch self->Utils.Dict.dangerouslyGetNonOption(Entity.key) {
+    | Some(table) =>
+      table->(
+        Utils.magic: InMemoryTable.Entity.t<Entities.internalEntity> => InMemoryTable.Entity.t<
+          entity,
+        >
+      )
+
+    | None =>
+      UndefinedEntity(Entity.key)->ErrorHandling.mkLogAndRaise(
+        ~msg="Unexpected, entity InMemoryTable is undefined",
+      )
+    }
+  }


I've refactored inMemoryStore to be library like where it does not codegen a record for each entity but uses a dict to hold each entity.

If you look at the commits you should be able to see the refactor diff before the file move.

JonoPrest · 2024-11-14T10:50:57Z

-let executeBatch = async (sql, ~inMemoryStore: InMemoryStore.t, ~isInReorgThreshold) => {
+let executeBatch = async (sql, ~inMemoryStore: InMemoryStore.t, ~isInReorgThreshold, ~config) => {
  let entityDbExecutionComposer =
-    RegisterHandlers.getConfig()->Config.shouldSaveHistory(~isInReorgThreshold)


The problem with referencing the codegen config is that tests which pass in a different config have mixed states. I had this problem with a test that was using unordered multichain mode for some functions and not for others resulting in some weird behaviours

I didn't do this in the beginning for simplicity, but this was the goal to have it like this

JonoPrest · 2024-11-14T10:57:04Z

+      let diff = await DbFunctions.sql->DbFunctions.EntityHistory.getRollbackDiff(
+        isUnorderedMultichainMode
+          ? UnorderedMultichain({
+              reorgChainId: chainId,
+              safeBlockNumber: blockNumber,
+            })
+          : OrderedMultichain({
+              safeBlockTimestamp: blockTimestamp,
+              reorgChainId: chainId,
+              safeBlockNumber: blockNumber,
+            }),
+        ~entityMod,
+      )


The diffing now has a different way to treat ordered/unordered multichain mode there's a good explanation in DbFunctions.res

JonoPrest · 2024-11-14T10:58:25Z

-type existingValueInDb<'entityType> =
-  | Retrieved(entityValueAtStartOfBatch<'entityType>)
-  // NOTE: We use an postgres function solve the issue of this entities previous value not being known.
-  | Unknown
-
 type updatedValue<'entityType> = {
-  // Initial value within a batch
-  initial: existingValueInDb<'entityType>,


Was not used

JonoPrest · 2024-11-14T10:58:39Z

-  shouldSaveHistory: bool,
  entityId: id,
  entityUpdateAction: entityUpdateAction<'entityType>,
 }

-let mkEntityUpdate = (~shouldSaveHistory=true, ~eventIdentifier, ~entityId, entityUpdateAction) => {
+let mkEntityUpdate = (~eventIdentifier, ~entityId, entityUpdateAction) => {
  entityId,
-  shouldSaveHistory,


No longer needed after inMemoryTable refactor

JonoPrest · 2024-11-14T10:59:16Z

+let allEntities: array<module(InternalEntity)> =
+  [


Expose an array of each entity module rather than just the table

JonoPrest · 2024-11-14T11:01:13Z

+  let rec rollBackToBlockNumberLtInternal = (
+    ~blockNumber: int,


We now just rollback to a state where all blocks are lower than the block of the first change to an entity on the given chain.

JonoPrest · 2024-11-14T11:02:07Z

+    /**
+    Uses two different methods for determining the first change event after rollback block
+
+    This is needed since unordered multichain mode only cares about any changes that 
+    occurred after the first change on the reorg chain. To prevent skipping or double processing events
+    on the other chains. If for instance there are no entity changes based on the reorg chain, the other
+    chains do not need to be rolled back, and if the reorg chain has new included events, it does not matter
+    that if those events are processed out of order from other chains since this is "unordered_multichain_mode"
+
+    Ordered multichain mode needs to ensure that all chains rollback to any event that occurred after the reorg chain
+    block number. Regardless of whether the reorg chain incurred any changes or not to entities.
+    */
+    let makeGetFirstChangeSerial = (self: t, ~entityName) =>


This comment helps explain the two different approaches of rolling back/getting a diff.

JonoPrest · 2024-11-14T11:03:26Z

-          }
-
-          Some({entity, eventIdentifier})
+  let deleteAllEntityHistoryAfterEventIdentifier = (~isUnorderedMultichainMode) => (


This just kept it's naming/signature but I plan on doing some refactoring for all the delete functions that happen on a rollback in the next PR

DZakh

Amazing work. After looking at the recent PRs I want to start calling you an SQL wizard at Envio 😂 Left some nitpicks, probably the most important comment is about the serial column.

DZakh · 2024-11-14T14:03:45Z

-          history
-          ->Belt.Array.concat([latest])
-          ->getEntityHistoryItems
+        let entityHistoryItems = history->getEntityHistoryItems


non-blocking: Not super urgent, considering the importance of the rollbacks fix, but the reduce looks very slow (relatively to the rest js execution). I'd suggest to rewrite it to a forEach with pushes

I'm adding it to the refactor list I have but this update hasn't introduced the reduce 👍🏼

DZakh · 2024-11-14T14:16:36Z

-let executeBatch = async (sql, ~inMemoryStore: InMemoryStore.t, ~isInReorgThreshold) => {
+let executeBatch = async (sql, ~inMemoryStore: InMemoryStore.t, ~isInReorgThreshold, ~config) => {
  let entityDbExecutionComposer =
-    RegisterHandlers.getConfig()->Config.shouldSaveHistory(~isInReorgThreshold)


I didn't do this in the beginning for simplicity, but this was the goal to have it like this

DZakh · 2024-11-14T14:18:40Z

-        inMemStore.{{entity.name.uncapitalized}}->InMemoryTable.Entity.set(
-          Delete->Types.mkEntityUpdate(~eventIdentifier=rollBackEventIdentifier, ~entityId, ~shouldSaveHistory),
-          ~shouldSaveHistory,
+    await Utils.Array.awaitEach(Entities.allEntities, async entityMod => {


Promise.all? I think you mentioned this place

DZakh · 2024-11-14T14:41:08Z

+@genType
+type rawEventsKey = {


Why @Gentype here?

Not sure possibly a relic from tests. I didn't add it now 👍🏼

DZakh · 2024-11-14T14:41:12Z

+@genType
+type dynamicContractRegistryKey = {


Why @Gentype here?

Same as above ☝🏼

DZakh · 2024-11-14T15:09:57Z


-  let rec rollBackToBlockTimestampLteInternal = (
-    ~blockTimestamp: int,
+  let min = (arrInt: array<int>) => {


There's https://rescript-lang.org/docs/manual/latest/api/js/math#value-minMany_int

DZakh · 2024-11-14T15:29:30Z

-      sql`${constrQuery(sql)}${i === dynQueryConstructors.length - 1 ? sql`` : sql`, `
-        }`,
+      sql`${constrQuery(sql)}${
+        i === dynQueryConstructors.length - 1 ? sql`` : sql`, `


Is sql call needed here?

Sorry this is just formatting changes. But yes it's all part of building dynamic queries with postgresjs. I actually think I prefer building these queries with eval. So another potential refactor

https://github.com/porsager/postgres?tab=readme-ov-file#building-queries

DZakh · 2024-11-14T15:38:19Z

+    COALESCE(before.id, after.id) AS id,
+    COALESCE(before.action, 'DELETE') AS action,
+    -- Deleting at 0 values will work fine for future rollbacks
+    COALESCE(before.entity_history_block_number, 0) AS entity_history_block_number,
+    COALESCE(before.entity_history_block_timestamp, 0) AS entity_history_block_timestamp,
+    COALESCE(before.entity_history_chain_id, 0) AS entity_history_chain_id,
+    COALESCE(before.entity_history_log_index, 0) AS entity_history_log_index
+  FROM
+    -- Use a RIGHT JOIN, to ensure that nulls get returned if there is no "before" row
+    public.${sql(entityName + "_history")} before
+    RIGHT JOIN rollback_ids after ON before.id = after.id
+    AND before.entity_history_block_timestamp = after.previous_entity_history_block_timestamp
+    AND before.entity_history_chain_id = after.previous_entity_history_chain_id
+    AND before.entity_history_block_number = after.previous_entity_history_block_number
+    AND before.entity_history_log_index = after.previous_entity_history_log_index;


Looks complicated 😅

Yes, but this one gives me less headaches reading than the first one I implemented 😓

DZakh · 2024-11-14T15:42:07Z

+    -- In the case where no previous row exists, coalesce the needed values since this new entity
+    -- will need to be deleted
+    COALESCE(before.id, after.id) AS id,
+    COALESCE(before.action, 'DELETE') AS action,
+    -- Deleting at 0 values will work fine for future rollbacks


Should we maybe handle it on js side with rescript-schema instead of using COALESCE here?

No particular reason, besides the fact that the query looks already quite complex

We can't, we just get null values then. We could handle it with just coalescing id and then have a more advanced schema for inferring delete and 0 values for the others. But I think this works fine.

DZakh · 2024-11-14T15:47:06Z


  let actionField = mkField(actionFieldName, Custom(Enums.EntityHistoryRowAction.enum.name))

+  let serialField = mkField("serial", Serial, ~isNullable=true)


I think it should have an index

Or even a PRIMARY KEY

Also, I don't understand why it's nullable

I think it should have an index

I agree, I want to add a few indexes actually.

Or even a PRIMARY KEY

It could be primary key but we need a unique constraint on chainId, blockNumber, logIndex and id. Well it doesn't need to be enforced technically but it's already helped catch a few bugs.

Also, I don't understand why it's nullable,

It's just because the serial is calculated on postgres side. Marking "nullable" here simply means the the field doesn't get a "NOT NULL" added on table creation. And you can pass "Null" on inserts to let postgres deal with the row creation.

What about something like NOT NULL DEFAULT nextval('table_name_id_seq')?

If it complicates things we can keep it as nullable

JonoPrest marked this pull request as draft November 11, 2024 16:28

JonoPrest marked this pull request as ready for review November 14, 2024 09:59

JonoPrest requested a review from DZakh November 14, 2024 10:38

JonoPrest commented Nov 14, 2024

View reviewed changes

DZakh approved these changes Nov 14, 2024

View reviewed changes

JonoPrest force-pushed the jp/handle-setting-history-in-io branch from 6cb09a7 to 8c734a3 Compare November 18, 2024 15:28

Base automatically changed from jp/handle-setting-history-in-io to main November 18, 2024 16:11

JonoPrest added 11 commits November 18, 2024 18:12

Implement entity history tables for each entity

d4fddc0

Wip implement per table insert history item function with tests

b79031f

Add entity history schema creator

fff1784

Add eval function for inserting entity history

976e2e4

Fix spelling of variable

11d16ad

Move EntityHistory to its own file

595f6e0

Add serial to entity history

f31127f

Make InMemomoryStore static

dc667f5

Move InMemomoryStore to static templates dir

8ffb824

Add new rollback diffing queries

99fc82d

Implement delete rolled back entity history

fca919c

JonoPrest added 11 commits November 18, 2024 18:14

Implement multichain rollback filters

ecaac1e

Implement unordered and ordered multichain versions of rollbacks

8ee8a57

Make tests compile

a193568

Fix broken magic in memory table function

41c5864

Fix double quoted dynamic table name in query

ac3f06c

Add new test expectations

f036764

Refactor simpler in memory table entity history state

22e603e

Fix diff and delete entity history functions

08ec0fb

Fix tests

2c2015f

Fix inverted rollback check

b973060

Include rollback multichain test in suite

fe8a277

JonoPrest force-pushed the jp/entity-history-diff branch from 64e94b6 to fe8a277 Compare November 18, 2024 16:15

JonoPrest merged commit 59918ee into main Nov 18, 2024

JonoPrest deleted the jp/entity-history-diff branch November 18, 2024 16:26

		let rec rollBackToBlockNumberLtInternal = (
		~blockNumber: int,


		let actionField = mkField(actionFieldName, Custom(Enums.EntityHistoryRowAction.enum.name))

		let serialField = mkField("serial", Serial, ~isNullable=true)

Uh oh!

Conversation

JonoPrest commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JonoPrest commented Nov 14, 2024

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!

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!

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!

DZakh 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!

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!

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!

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!

JonoPrest commented Nov 11, 2024 •

edited

Loading