Versioned Storage for Modifications History #2233

acerone85 · 2024-09-20T17:09:28Z

Linked Issues/PRs

Related to #2095

Description

Adds a ModificationsHistoryV2 column in the Historical RocksDB, with keys encoded in big endian
Adds a flag to the historical RocksDB that keeps track whether the migration is still in progress
Change HistoricalRocksDB to alway insert historical changes from V2 table. When the migration is in progress, delete key-value pairs from both V2 and V2, read key-value pairs from V2 falling back to V1 if no value in V2 is found

#TODO (either this or next MR):

Small task that performs the migration of key-value pairs from V1 to V2 (Done in Feature/migrate modification history #2260)

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests
The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

I have reviewed the code myself
I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

Rust SDK
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

crates/fuel-core/src/state/historical_rocksdb.rs

rafal-ch · 2024-09-27T10:35:01Z

crates/fuel-core/src/state/historical_rocksdb.rs

@@ -300,12 +355,95 @@ where

        Ok(())
    }
+
+    /// Migrates a ModificationHistory key-value pair from V1 to V2.
+    /// The migration fails if other


Seems like an incomplete comment.

Changed: 87ace1b. Although I'll wait also for @xgreenx to check if my comment is correct, because from the code it does not look like we use the ConflictPolicy::Fail flag when writing directly to rocksDB.

crates/fuel-core/src/state/historical_rocksdb.rs

rafal-ch · 2024-09-27T11:11:01Z

crates/fuel-core/src/state/historical_rocksdb.rs

+// `ModificationsHistoryV1` and return it if no value for `ModificationsHistoryV2`
+// was found. This is necessary to avoid scenarios where it is possible to
+// roll back twice to the same block height
+fn multiversion_take<Description>(


multiversion_take() and multiversion_replace() are very similar, please consider some deduplication. We should be able to provide, let's say, multiversion_op() which also takes a generic F argument.

Then, on the callsite we can use something like this:

let old_changes = multiversion_op( storage_transaction, height_u64, self.migration_in_progress, |storage| storage.replace(&height_u64, &reverse_changes), )?;

or

let last_changes = multiversion_op( &mut storage_transaction, height_to_rollback, self.migration_in_progress, |storage| storage.take(&height_to_rollback), )? .ok_or(not_found!(ModificationsHistoryV1<Description>))?;

However, due to the complex types, the definition of F may become a mess, so I'm leaving it up to you if this is worth changing.

F: FnOnce( StorageMut< '_, StructuredStorage< InMemoryTransaction<&rocks_db::RocksDb<Historical<Description>>>, >, ModificationsHistoryV2<Description>, >, ) -> StorageResult<Option<Changes>>,

Tried, the signature itself is not a problem, but I spent quite a bit of time fighitng with lifetimes and eventually gave up.

Please see here how I imagined it :)
#2277

But again, up to you. Feel free to close the PR and leave it implemented with two separate functions if you find it more convenient.

Thanks, I had a look :)
I changed this PR quite a lot, so still fighting with typechecking when trying to rebase.
But I definitely like it how you have done it

crates/fuel-core/src/state/historical_rocksdb.rs

crates/fuel-core/src/state/historical_rocksdb/modifications_history.rs

xgreenx

It also would be nice to have a tests that inserts 1000 blocks, and tries to rollback them by calling HistoricalRocksDB::rollback_last_block

xgreenx · 2024-10-01T08:00:35Z

crates/fuel-core/src/state/historical_rocksdb.rs

+    ) -> StorageResult<()> {
+        let mut migration_transaction = StorageTransaction::transaction(
+            &self.db,
+            ConflictPolicy::Fail,


Conflict policy works only when you try to commit one in-memory transaction into another. It doesn't work with the Rocksdb.

The migration process should accumulate changes in the memory and dump them inside of the commit_changes function(when upper services like block importer or relayer call this function).

Thanks. In the latest version of the MR I switched to having cumulative changes in the rocksDB state, and I handle race conditions by locking on them for the whole duration of the migration/rollback/commit_changes function. Unfortunately I cannot use more fine-grained guards because of potential consistency issues (explanation in the comments).

xgreenx · 2024-10-01T08:04:41Z

crates/fuel-core/src/state/historical_rocksdb.rs

@@ -94,6 +103,7 @@ where
        Ok(Self {
            state_rewind_policy,
            db,
+            modifications_history_migration_in_progress: true,


It would be nice to add logic that checks do we have any entries in the V1. If not, set this to false

Yes, the latest version of the PR does not have this check anymore, and instead we check directly if we are able to fetch at least one change for the v1 history.

acerone85 · 2024-10-04T10:40:06Z

It also would be nice to have a tests that inserts 1000 blocks, and tries to rollback them by calling HistoricalRocksDB::rollback_last_block

Done. There is a new test that checks that commits are rollback consistently to the latest height, when we use V2. 0d2c490

rafal-ch · 2024-10-29T10:08:55Z

Cargo.lock

@@ -735,7 +735,7 @@ dependencies = [
 "percent-encoding",
 "pin-project-lite",
 "tracing",
- "uuid 1.10.0",
+ "uuid 1.11.0",


I learned here that we don't usually update dependencies in an arbitrary PR.

crates/fuel-core/src/state/historical_rocksdb.rs

rafal-ch · 2024-10-29T10:41:11Z

crates/fuel-core/src/state/historical_rocksdb.rs

+        // This is to avoid a scenario where the migration changes overwrites changes to the modification history
+        // coming from other transactions.
+        // If we were to release the lock as soon as we take the cumulative changes, and then acquiring it again when
+        // writing the updatet set of changes, then we could have the following schedule of events.


A nit:

Suggested change

// writing the updatet set of changes, then we could have the following schedule of events.

// writing the updated set of changes, then we could have the following schedule of events.

crates/fuel-core/src/state/historical_rocksdb.rs

rafal-ch · 2024-10-29T11:11:31Z

crates/fuel-core/src/state/historical_rocksdb/modifications_history.rs

+    type Blueprint = Plain<Primitive<8>, Postcard>;
+    type Column = Column<Description>;
+
+    // We store key-value pairs using the same column family as for V1


I'm a bit confused here, since I see that the column is different, not the same.

yes, stale comment. Removed in f162887

crates/fuel-core/src/state/historical_rocksdb/modifications_history.rs

crates/fuel-core/src/state/historical_rocksdb.rs

AurelienFT

I agree with some comments of rafal and added some

crates/fuel-core/src/state/historical_rocksdb/description.rs

crates/fuel-core/src/state/historical_rocksdb.rs

xgreenx · 2024-10-29T16:52:37Z

CHANGELOG.md

-### Changed
-


Could you return it back, please?=)

xgreenx · 2024-10-29T16:53:09Z

crates/fuel-core/src/state/historical_rocksdb.rs

@@ -441,15 +717,58 @@ where
        height: Option<Description::Height>,
        changes: Changes,
    ) -> StorageResult<()> {
-        let mut storage_transaction =
-            StorageTransaction::transaction(&self.db, ConflictPolicy::Overwrite, changes);
+        // cumulative_changes_lock_guard is defined to be Some only when the migration is in progress.


I think the current solution is overly complicated. We shouldn't be afraid of losing the migration state. The migration process just should re-migrate lost migration later.

In this case, this function will be simple and only insert migration into final changes.

I think just returning Option<Changes> should be enough without lock.

For clarity: do you mean the function take_migration_changes should return only Option<Changes>?
If we do not care about keeping the migration changes I can remove keeping the lock guard here.
I also don't need to readd the migration changes later on.

Note that this works under the assumption that commit_changes always commit changes at a height that is not present already in the underlying db, so I want to validate this first. If this is not the case, then there is the risk of having a stale version from a migration overwriting the newer version that has been written into the DB by commit_changes.

do you mean the function take_migration_changes should return only Option?

Yes, it seems should be enough.

Note that this works under the assumption that commit_changes always commit changes at a height that is not present already in the underlying db, so I want to validate this first. If this is not the case, then there is the risk of having a stale version from a migration overwriting the newer version that has been written into the DB by commit_changes.

Why we can't apply changes from the store_modifications_history on top of the changes from migration? In this case we will not lose changes from the store_modifications_history.

If you worry that later migration will override modification history, then it shouldn't happen because you migrate only v1 entries, while store_modifications_history uses v2. Only in the case of rollback can you face the situation where migration with old modifications can override new ones. But as I mentioned in another comment, we can handle this situation just during the migration process by tracking what is the maximum height of the migration(and decreasing this height during the rollback event)

Why we can't apply changes from the store_modifications_history on top of the changes from migration? In this case we will not lose changes from the store_modifications_history.

The scenario I was concerned about goes as follows (As I mention this can happen only if we commit twice at the same height, which does not seem something that can happen without rollbacking first)

You start the function to commit at a height for which there is a V1 key present already, with a different set of changes. This function takes the migration changes to commit, and find that they are empty

You migrate the same key from V1 to V2. The changes (which are different from the ones from V1) are persisted in memory

The commit transaction commits into rocksDB. Later on, a second commit transaction at a different height gets the stale migration changes that have been persisted in memory.

So if the following above cannot happen then I am convinced that it should be fine to not keep the lock for long

xgreenx · 2024-10-29T17:02:54Z

crates/fuel-core/src/state/historical_rocksdb.rs

+        let (cumulative_changes, maybe_cumulative_changes_guard) =
+            self.take_migration_changes();


Could you elaborate more on why we need to change how rollback_block_to should work?

How migration is going shouldn't affect the work of the rollback_block_to method. Migration may have a state higher than the chain's height (after rollback), and it can be a problem. But we can handle this case in the commit_changes function and remove modifications that are higher than blockchain height.

I follow only partially here. I was under the impression that rollback_block_to reverts all the changes from the current height to the one specified in the function, but this does not seem to be the case. This leaves me with the question: why is it okay to revert only the changes at the block height specified in input? Do we use this function only to rollback to the last block (which seems to be the case from the codebase)?

Regarding your question, my flow of thought is as follows: Migration changes are piggybacked into transactions whenever they are committed to the rocksDB instance. We do this in two places: rollback_block_to and <HistoricalRocksDB as TransactableStorage>::commit_changes. In the first case we compute the last_changes by taking into account the migration changes from the in memory transaction, so even if the key for that height is being migrated from V1 to V2, the migrated changes will be captured in last_changes and removed. Please let me know if i am missing something.

If the cumulative changes were not flushed here, they would be kept in memory and flush when we later commit a block. Now we could have a scenario where we rollbacked a block several time, but we did not commit the migration changes for blocks at a higher height.

This leaves me with the question: why is it okay to revert only the changes at the block height specified in input? Do we use this function only to rollback to the last block (which seems to be the case from the codebase)?

It is a temporary workaround because rollback_last_block doesn't work as expected.

Migration changes are piggybacked into transactions whenever they are committed to the rocksDB instance.

For us it will be enough to commit changes from the migration only in commit_changes.

If the cumulative changes were not flushed here, they would be kept in memory and flush when we later commit a block. Now we could have a scenario where we rollbacked a block several time, but we did not commit the migration changes for blocks at a higher height.

I don't see any problems in keeping them in the memory until we start importing new blocks(it is okay if we need to rollback a lot of blocks. Usually it is how we use rollback_block_to).

I don't see any problems in keeping them in the memory until we start importing new blocks(it is okay if we need to rollback a lot of blocks. Usually it is how we use rollback_block_to).

My main concern in this case is a scenario that would go as follows (note that unless I have misunderstood the rollback and commit functions, this can happen):

Say the max height of the modification history is 100. We rollback the last block 50 times, using a strategy where we do not commit the migration changes in rocksDB.

During this time the migration process for all blocks completes and the changes are persisted in memory,

The next time the changes for a block height are committed, all the migration changes (including those at height > 50) will be committed, leaving the DB in an inconsistent state.

You mentioned to keep track of the maximum height of the migration, but if rollbacks are not frequent a simpler alternative would be to wipe all migration changes before committing a transaction. Though this would require the lock to be kept while the commit to rocksDB happens to avoid data races.

(Or we can simply wipe all migration changes higher than the block height to be committed, when committing)

xgreenx · 2024-10-29T17:09:20Z

crates/fuel-core/src/state/historical_rocksdb.rs

+    /// are simply recorded, and will be flushed to the database when it
+    /// commits or rollbacks to a new height.
+    #[cfg(test)]
+    fn migrate_modifications_history_at_height(&self, height: u64) -> StorageResult<()> {


Side question: Why do we want to implement a function in this PR? It can be part of the migration PR.

I think the migration process should work with the iterator of the historical modification table instead of fetching each modification height by height. In this case, you can easily fetch butch of modifications up to some limit, migrate them, and store them in one Changes type inside of the migration_changes by just locking it once. Then the migration process can wait until we take changes from migration_changes during commit_changes, and continue after.

With this solution, you only lock the migration_changes changes once for very fast operation. Plus, the iteration over the storage will be much faster(and it will work even if we do rollback or alter the history somehow because while you are holding an iterator over RocksDB, it will not lose the data).

Sounds good. I can make the change, although probably it makes sense to do it in the migration PR.
Btw in this PR that function is only for testing purposes, so if that's okay I would keep it and change the migration process using the rocksDB iterator in the follow-up.

It seems this function is only used inside of the migrate_modifications_history_works tests that is dedicated to this function=D I think it is better to test in the PR where we really plan to have final migration

acerone85 requested review from xgreenx, Dentosal and MitchTurner as code owners September 20, 2024 17:09

acerone85 marked this pull request as draft September 23, 2024 08:57

acerone85 self-assigned this Sep 23, 2024

acerone85 force-pushed the feature/version-modifications-history branch 3 times, most recently from ee8edeb to ffae32a Compare September 26, 2024 08:24

acerone85 changed the title ~~WIP - Versioned Storage for Modifications History~~ Versioned Storage for Modifications History Sep 26, 2024

acerone85 marked this pull request as ready for review September 26, 2024 12:32

acerone85 mentioned this pull request Sep 26, 2024

Feature/migrate modification history #2260

Open

11 tasks

acerone85 requested a review from a team September 26, 2024 21:50

rafal-ch reviewed Sep 27, 2024

View reviewed changes

acerone85 force-pushed the feature/version-modifications-history branch from 910186c to fe3e453 Compare September 30, 2024 15:55

xgreenx reviewed Oct 1, 2024

View reviewed changes

acerone85 force-pushed the feature/version-modifications-history branch 3 times, most recently from f787cf6 to 308c34a Compare October 3, 2024 15:57

acerone85 added 2 commits October 3, 2024 17:09

Add ModificationsHistoryV2 with right encoding

6e19507

HistoricalRocksDB starts using ModificationHistory V2 for new changes

3c9eff4

acerone85 force-pushed the feature/version-modifications-history branch from 308c34a to 3c9eff4 Compare October 3, 2024 16:45

Additional test that rollback works with V2

0d2c490

acerone85 added 5 commits October 10, 2024 16:19

Merge branch 'master' into feature/version-modifications-history

eb75cd6

Update CHANGELOG

092e12f

Typo

832432f

Merge branch 'master' into feature/version-modifications-history

ec9a907

Update CHANGELOG

5fb672d

rafal-ch reviewed Oct 29, 2024

View reviewed changes

crates/fuel-core/src/state/historical_rocksdb/modifications_history.rs Outdated Show resolved Hide resolved

crates/fuel-core/src/state/historical_rocksdb.rs Outdated Show resolved Hide resolved

crates/fuel-core/src/state/historical_rocksdb.rs Show resolved Hide resolved

AurelienFT reviewed Oct 29, 2024

View reviewed changes

crates/fuel-core/src/state/historical_rocksdb/description.rs Outdated Show resolved Hide resolved

crates/fuel-core/src/state/historical_rocksdb.rs Show resolved Hide resolved

acerone85 added 12 commits October 29, 2024 15:18

Merge branch 'master' into feature/version-modifications-history

82e7db8

Revert changes to Cargo.lock

b8b22b5

Cargo.lock

56c6428

Updatet -> Updated

07d43e2

Remove stale todo

f996101

Generic implementation of multiversion_op

5ea1bbc

N -> Version in comment

315172c

Remove Spurious comment

d56f351

Add comment why commit cannot fail

4dc2643

Better pattern matching in column prefix bytes

a5cc0bb

Revert taken cumulative changes if writing modification history fails

2739d61

Add Given/When/Then in tests

9f050e5

acerone85 force-pushed the feature/version-modifications-history branch from d552e7c to 9f050e5 Compare October 29, 2024 16:49

acerone85 added 2 commits October 29, 2024 16:50

Merge branch 'master' into feature/version-modifications-history

fc58a71

Remove stale comment

f162887

xgreenx reviewed Oct 29, 2024

View reviewed changes

WIP - Commit only valid migrated changes when committing a transaction

df975db

acerone85 force-pushed the feature/version-modifications-history branch from 5e36072 to df975db Compare October 30, 2024 13:52

acerone85 added 3 commits October 30, 2024 18:31

Remove changes related to migration of modification history

a343938

Polish code

04ba994

Fix Changelog

1022595

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Versioned Storage for Modifications History #2233

Versioned Storage for Modifications History #2233

acerone85 commented Sep 20, 2024 •

edited

Loading

rafal-ch Sep 27, 2024

acerone85 Sep 30, 2024

rafal-ch Sep 27, 2024

acerone85 Sep 30, 2024

rafal-ch Oct 2, 2024 •

edited

Loading

acerone85 Oct 4, 2024

xgreenx left a comment

xgreenx Oct 1, 2024

acerone85 Oct 4, 2024

xgreenx Oct 1, 2024

acerone85 Oct 4, 2024

acerone85 commented Oct 4, 2024

rafal-ch Oct 29, 2024

rafal-ch Oct 29, 2024

acerone85 Oct 29, 2024

rafal-ch Oct 29, 2024

acerone85 Oct 29, 2024

AurelienFT left a comment

xgreenx Oct 29, 2024

xgreenx Oct 29, 2024

acerone85 Oct 29, 2024

xgreenx Oct 29, 2024

acerone85 Oct 30, 2024

xgreenx Oct 29, 2024

acerone85 Oct 29, 2024

xgreenx Oct 29, 2024

acerone85 Oct 30, 2024

acerone85 Oct 30, 2024

xgreenx Oct 29, 2024

acerone85 Oct 29, 2024

xgreenx Oct 29, 2024

	// writing the updatet set of changes, then we could have the following schedule of events.
	// writing the updated set of changes, then we could have the following schedule of events.

		let (cumulative_changes, maybe_cumulative_changes_guard) =
		self.take_migration_changes();

		### Changed

Versioned Storage for Modifications History #2233

Are you sure you want to change the base?

Versioned Storage for Modifications History #2233

Conversation

acerone85 commented Sep 20, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

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

rafal-ch Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx left a comment

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

Choose a reason for hiding this comment

acerone85 commented Oct 4, 2024

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

Choose a reason for hiding this comment

AurelienFT left a comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acerone85 commented Sep 20, 2024 •

edited

Loading

rafal-ch Oct 2, 2024 •

edited

Loading