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

Fix NFT race condition #172

Merged

Conversation

pmantica11
Copy link
Collaborator

Summary

Context:

  1. The assets table is a denormalized of data derived from token, mint and metadata accounts. Each of the different kinds of accounts updates a particular subset of columns within the assets table.
  2. The NFT ingester makes no guarantees to the order of how account updates are processed. So to prevent earlier updates from overriding later ones we store in all tables the slot_updated column and when processing an account only update the table if the slot is greater than or equal to the current slot_updated of the table.

Issue:
The fundamental issue is that even though the assets table holds denormalized data for 3 different kind of account updates, it only has one slot_updated column. This means that, for instance, token accounts updates get ignored if a metadata account processed with a greater slot was already processed. This is clearly erroneous because the token account update and metadata affect different columns. So metadata account updates should in no way interfere from token account data being updated. For instance, in this case the owner column, which is updated through token account updates, doesn’t get properly updated and reflects stale ownership data.

Solution:
Added slot_updated_mint_account, slot_updated_token_account and slot_updated_metadata_account columns to avoid metadata/token/mint updates from interfering with each other. Also added helper functions to make sure that in each account update we only modify the columns that each account is responsible for.

One issue that I had to deal with is we use the slot_updated column for sorting. So I created a db trigger so that it's always the max of the slot_updated columns. I also created a slot_updated_cnft_transaction so that we keep track of cNFT slot updates.

Tests

Ran new integration tests and added an account_updates_tests.rs file that tests the race condition alongside general update logic.

Copy link
Collaborator

@kespinola kespinola left a comment

Choose a reason for hiding this comment

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

Using the additional slot columns is a good way to manage updates to the asset table given its a compilation of the metadata, token, cnft, and ta accounts. I do believe tthese updates will likely reduce the ingestion rate (see other comments) but is still worth releasing to improve data correctness.

I will be follow up with an issue for us to discuss the pros/cons of normalizing the asset so there are no dependencies between the accounts in question. It requires more joins on the read side so there are draw backs but this logic is complicated and intensive. I believe correctness should be the number one concern for project so I'm personally willing to take a hit on the query for clearer ingestion flow. We can introduce conventional caching technics for the reads to keep them fast. Anyhow look for the issue we can talk more there.

I am blocking to encourage discussion about the use of a trigger. I do also encourage the changes to the migration.

I'm chatting with the Triton team to get this tested because we too have been experiencing issues related to ownership updates for traditional nfts which this addresses. Thank you 👍

Cargo.toml Outdated Show resolved Hide resolved
migration/src/lib.rs Show resolved Hide resolved
nft_ingester/src/program_transformers/asset_upserts.rs Outdated Show resolved Hide resolved
nft_ingester/src/program_transformers/asset_upserts.rs Outdated Show resolved Hide resolved
nft_ingester/src/program_transformers/token/mod.rs Outdated Show resolved Hide resolved
@pmantica11 pmantica11 merged commit a1f7047 into metaplex-foundation:main Feb 7, 2024
3 checks passed
niks3089 added a commit to helius-labs/digital-asset-rpc-infrastructure that referenced this pull request Feb 19, 2024
* Fix cNFT update metadata indexing (metaplex-foundation#167)

* Update to use Token Metadata (and latest Bubblegum) Rust clients (metaplex-foundation#168)

* Use Token Metadata and latest Bubblegum Rust clients, new blockbuster

* Remove candy machine and candy guard

* Update lock file

* Improve Master Edition V1 and V2 indexing

* Update to use blockbuster published crate

* Update lock file

* Update Token Metadata and Blockbuster deps (metaplex-foundation#170)

* Update to latest token metadata and blockbuster crates

* Update Cargo lock file

* Add DAS integration tests (metaplex-foundation#169)

* Add das integration tests

* Delete unused snapshots

* Delete unnecessary test data

* Clean up dependencies

* Nit

* Address comments

* Add README.md

* Fix warnings

* feat(ops): add ops crate for bundling misc tools. add a tree backfiller based on cl_audits_v2.

* Fix NFT race condition (metaplex-foundation#172)

* Add das integration tests

* Delete unused snapshots

* Delete unnecessary test data

* Clean up dependencies

* Nit

* Address comments

* Add README.md

* Fix warnings

* Fix asset race condition

* Add missing files

* Fix warnings

* Fix missing slot updated

* Address comments

* Nit

* Update Rust version and missing dirs in docker build (metaplex-foundation#173)

---------

Co-authored-by: Nicolas Pennie <[email protected]>
Co-authored-by: Michael Danenberg <[email protected]>
Co-authored-by: pmantica11 <[email protected]>
Co-authored-by: Kyle Espinola <[email protected]>
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.

3 participants