Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mtg-1065] Keep metadata inside of mutable storages up to date with occuring changes #339

Open
wants to merge 15 commits into
base: new-main
Choose a base branch
from

Conversation

kstepanovdev
Copy link
Contributor

@kstepanovdev kstepanovdev commented Dec 16, 2024

The issue is metadata as a part of immutable storages may be updated freely off-chain. That leads to an issue when the indexer doesn't provide up-to-date information regarding such changes.

To rectify this, the cache of the metadata has been added along with a new type of metadata that reflects its mutability. The gist of this field is to keep track of the latest changes occurring inside of metadata. For brevity, the pseudocode is simplified:
If cache period + last_metadata_read > curr_time && is_mutable { update_metadata }
Such changes do not provide an instant update, however, it may be configured.

Also, the OffChainData has been moved to flatbuffers encoding, which gives us an opportunity for further changes without writing yet another migration for RocksDb.

@kstepanovdev kstepanovdev force-pushed the fix/keep-metadata-up-to-date branch from 532e834 to 188ef5c Compare December 17, 2024 15:06
@kstepanovdev kstepanovdev changed the title Keep metadata inside of mutable storages up to date with occuring changes [mtg-1065] Keep metadata inside of mutable storages up to date with occuring changes Dec 17, 2024
@kstepanovdev kstepanovdev force-pushed the fix/keep-metadata-up-to-date branch from 188ef5c to fbee2ae Compare December 18, 2024 11:47
@kstepanovdev kstepanovdev marked this pull request as ready for review December 18, 2024 11:47
Copy link
Contributor

@StanChe StanChe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing job, thank you! Improved the alignment of the code in the packages. Proper approach without cutting corners.
I'd suggest we remove old migrations altogether as all of our DBs were started from scratch with those CompleteAssetDetails in flatbuf. Some comments are related to refinement. We should also understand how do we test the change.
I'd suggest we take one of the small environments, copy it and run the migrations. Another approach will be creating a test that fills in some off chain data using the deprecated column and runs the migration afterwards.

rocks-db/src/clients/asset_client.rs Outdated Show resolved Hide resolved
rocks-db/src/columns/offchain_data.rs Show resolved Hide resolved
rocks-db/src/columns/offchain_data.rs Show resolved Hide resolved
rocks-db/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the merge function being set for both the deprecated structure and the new one.

Copy link
Contributor Author

@kstepanovdev kstepanovdev Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see, it doesn't need to have a dedicated merge function. Migrations will be done via migrator, while merge insert in terms of rocksdb doesn't use OffChainData as a separate structure. It has the "empty" implementation (aka merge_keep_existing) because it's a part of AssetCompleteDetails. Have I gotten something wrong?

nvm

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we have the merge function for new data column, that's operating on the old format, which seems wrong. And no merge function for the existing (deprecated) column. This code will not start with the existing db, as it requires a merge function to be present if it was defined once.
I believe you're missing a bit context on how merge works.
the db has the deprecated column already. It may also have a list of "pending merges" for that column. So when you open the DB for the migration, the old column will be read and data in it will be merged using a merge function on that "deprecated" column. Then the data will move to the new column and whenever it'll be merged with new values a different merge function will be called for it (the one defined for the new _V2 column). it'll have the existing value in flatbuffer already, as well as the merge operands (unless you're merging in a different format - which is also possible, but will introduce a bit of confusion). The result should also be in flatbuf

rocks-db/src/migrations/clean_update_authorities.rs Outdated Show resolved Hide resolved
rocks-db/src/migrations/clean_update_authorities.rs Outdated Show resolved Hide resolved
rocks-db/src/migrations/offchain_data.rs Outdated Show resolved Hide resolved
rocks-db/src/migrations/spl2022.rs Outdated Show resolved Hide resolved
@kstepanovdev kstepanovdev force-pushed the fix/keep-metadata-up-to-date branch from 1f2fbc8 to d388b97 Compare December 19, 2024 12:57
@kstepanovdev kstepanovdev requested a review from StanChe December 19, 2024 12:58
Copy link
Contributor

@StanChe StanChe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest creating a test scenario to verify it.

rocks-db/src/clients/asset_client.rs Show resolved Hide resolved
rocks-db/src/columns/offchain_data.rs Outdated Show resolved Hide resolved
rocks-db/src/columns/offchain_data.rs Show resolved Hide resolved
rocks-db/src/migrator.rs Outdated Show resolved Hide resolved
rocks-db/src/migrations/offchain_data.rs Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we have the merge function for new data column, that's operating on the old format, which seems wrong. And no merge function for the existing (deprecated) column. This code will not start with the existing db, as it requires a merge function to be present if it was defined once.
I believe you're missing a bit context on how merge works.
the db has the deprecated column already. It may also have a list of "pending merges" for that column. So when you open the DB for the migration, the old column will be read and data in it will be merged using a merge function on that "deprecated" column. Then the data will move to the new column and whenever it'll be merged with new values a different merge function will be called for it (the one defined for the new _V2 column). it'll have the existing value in flatbuffer already, as well as the merge operands (unless you're merging in a different format - which is also possible, but will introduce a bit of confusion). The result should also be in flatbuf

@kstepanovdev kstepanovdev requested a review from StanChe December 19, 2024 19:21
Copy link
Contributor

@StanChe StanChe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job. I'll add a test to verify it's working

rocks-db/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@StanChe StanChe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test and some fixes. The test is failing - after the migration no data was copied to a new format.
The biggest change required is that the read size was not modified and is still reading as if the data is in bincode. The example of reading flatbuf may be found in AssetComplete details. In short - any of the existing get methods may not be used.

@kstepanovdev kstepanovdev requested a review from StanChe December 21, 2024 17:13
Copy link
Contributor

@StanChe StanChe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, thank you! I'll put it live today on Eclipse

Copy link
Contributor

@StanChe StanChe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait a moment. Any interaction with asset_offchain_data, like batch_get or get should be changed to a flatbuffed version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants