-
Notifications
You must be signed in to change notification settings - Fork 59
fix(drive-abci): refresh the contract cache when the v13 migration rewrites DPNS #4232
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
Closed
Closed
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
c0764de
fix(drive-abci): refresh the contract cache when the v13 migration re…
QuantumExplorer f2dfd0f
refactor(drive-abci): only refresh DPNS in the v13 cache refresh
QuantumExplorer 89d2602
fix(drive): reject stale lower-version inserts into the data contract…
QuantumExplorer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,2 @@ | ||
| mod refresh_contract_cache; | ||
| mod strip_unknown_document_schema_properties; |
177 changes: 177 additions & 0 deletions
177
packages/rs-drive/src/drive/contract/migration/refresh_contract_cache.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,177 @@ | ||
| use crate::drive::Drive; | ||
| use crate::error::Error; | ||
| use dpp::version::PlatformVersion; | ||
| use grovedb::TransactionArg; | ||
|
|
||
| impl Drive { | ||
| /// Re-reads a data contract from state and re-seeds the in-memory data contract cache | ||
| /// with it. Call this from a protocol-upgrade migration for every contract the | ||
| /// migration **rewrites** — i.e. any contract that existed before the migration and | ||
| /// so may already sit in a node's cache. A contract the migration introduces for the | ||
| /// first time needs no refresh: the cache holds no negative entries, so no node can | ||
| /// have a stale copy of a contract that never existed. | ||
| /// | ||
| /// CONSENSUS-CRITICAL. Contracts written by a state transition go through the drive | ||
| /// operation batch, whose `RemoveDataContractFromCache` finalization task evicts the | ||
| /// superseded copy from the cache. Migrations write contracts directly | ||
| /// (`insert_contract` / `apply_contract`) and bypass that machinery entirely, so | ||
| /// without this call a node that already holds the pre-migration contract in its | ||
| /// global cache keeps serving that copy for the rest of the process lifetime, while a | ||
| /// node whose cache is cold (freshly restarted, or the entry was evicted under | ||
| /// capacity pressure) reads the migrated one. The two nodes then serialize documents | ||
| /// of that contract against different `DataContract`s — a difference that reaches | ||
| /// state, because `DocumentV0::serialize` picks its serialization version from the | ||
| /// contract's config version — and produce different app hashes from the same block. | ||
| /// | ||
| /// The refreshed contract is placed in the **block** cache, not the global cache: at | ||
| /// this point the migration's write is still uncommitted, and the block cache is the | ||
| /// only cache that transactional reads consult first. It is promoted to the global | ||
| /// cache once the block commits (`merge_and_clear_block_cache`), and dropped at the | ||
| /// start of the next block if the block never commits (`clear_block_cache`). | ||
| /// | ||
| /// The read deliberately bypasses both caches rather than going through | ||
| /// `get_contract_with_fetch_info`: a concurrent read-only query thread (which reads | ||
| /// committed state, with no transaction, and does populate the global cache) could | ||
| /// otherwise race a pre-migration copy back into the global cache between the | ||
| /// eviction and the re-seed. | ||
| /// | ||
| /// Billing is unaffected. `fetch_contract_v0` computes the cached `OperationCost` | ||
| /// with grovedb value caching disabled precisely so the cost of a contract fetch is | ||
| /// deterministic, and derives `fee` from that cost only when an epoch is supplied — | ||
| /// so a cache hit seeded here bills identically to the cold fetch it replaces. | ||
| pub fn refresh_data_contract_cache_from_state( | ||
| &self, | ||
| contract_id: [u8; 32], | ||
| transaction: TransactionArg, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<(), Error> { | ||
| // Cache-bypassing read: the contract exactly as state now holds it. | ||
| let maybe_fetch_info = self.fetch_contract_and_add_operations( | ||
| contract_id, | ||
| None, | ||
| transaction, | ||
| &mut vec![], | ||
| platform_version, | ||
| )?; | ||
|
|
||
| // Drop the pre-migration copy from both the block and the global cache. | ||
| self.cache.data_contracts.remove(contract_id); | ||
|
|
||
| // Re-seed with what state now holds. A migration always writes the contract it | ||
| // asks us to refresh, so `None` here means the contract genuinely is not in | ||
| // state; leaving the caches empty is then the correct outcome. | ||
| if let Some(fetch_info) = maybe_fetch_info { | ||
| self.cache | ||
| .data_contracts | ||
| .insert(fetch_info, transaction.is_some()); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use crate::drive::contract::DataContractFetchInfo; | ||
| use crate::util::test_helpers::setup::setup_drive_with_initial_state_structure; | ||
| use dpp::block::block_info::BlockInfo; | ||
| use dpp::data_contract::accessors::v0::{DataContractV0Getters, DataContractV0Setters}; | ||
| use dpp::system_data_contracts::{load_system_data_contract, SystemDataContract}; | ||
| use dpp::version::PlatformVersion; | ||
| use std::sync::Arc; | ||
|
|
||
| /// A contract written directly to state must replace a stale cached copy, so that a | ||
| /// warm node and a cold node read the same contract afterwards. | ||
| #[test] | ||
| fn test_refresh_replaces_stale_cached_contract() { | ||
| let drive = setup_drive_with_initial_state_structure(None); | ||
| let platform_version = PlatformVersion::latest(); | ||
| let transaction = drive.grove.start_transaction(); | ||
|
|
||
| let dpns = load_system_data_contract(SystemDataContract::DPNS, platform_version) | ||
| .expect("expected to load DPNS"); | ||
| let contract_id = dpns.id().to_buffer(); | ||
|
|
||
| // Warm the global cache with a contract that does NOT match state: a distinct | ||
| // version stands in for the pre-migration copy a long-lived node would hold. | ||
| let mut stale = dpns.clone(); | ||
| stale.set_version(u32::MAX); | ||
| drive.cache.data_contracts.insert( | ||
| Arc::new(DataContractFetchInfo { | ||
| contract: stale, | ||
| storage_flags: None, | ||
| cost: Default::default(), | ||
| fee: None, | ||
| }), | ||
| false, | ||
| ); | ||
|
|
||
| drive | ||
| .apply_contract( | ||
| &dpns, | ||
| BlockInfo::default(), | ||
| true, | ||
| None, | ||
| Some(&transaction), | ||
| platform_version, | ||
| ) | ||
| .expect("expected to apply contract"); | ||
|
|
||
| // Without the refresh this still reads the stale copy out of the global cache. | ||
| drive | ||
| .refresh_data_contract_cache_from_state( | ||
| contract_id, | ||
| Some(&transaction), | ||
| platform_version, | ||
| ) | ||
| .expect("expected to refresh cache"); | ||
|
|
||
| let refreshed = drive | ||
| .get_contract_with_fetch_info(contract_id, false, Some(&transaction), platform_version) | ||
| .expect("expected to fetch contract") | ||
| .expect("expected the contract to be present"); | ||
|
|
||
| assert_eq!(refreshed.contract.version(), dpns.version()); | ||
| } | ||
|
|
||
| /// The refresh must seed the block cache, not the global cache: the migration's write | ||
| /// is still uncommitted, so a non-transactional reader must not see it yet. | ||
| #[test] | ||
| fn test_refresh_seeds_block_cache_not_global_cache() { | ||
| let drive = setup_drive_with_initial_state_structure(None); | ||
| let platform_version = PlatformVersion::latest(); | ||
| let transaction = drive.grove.start_transaction(); | ||
|
|
||
| let dpns = load_system_data_contract(SystemDataContract::DPNS, platform_version) | ||
| .expect("expected to load DPNS"); | ||
| let contract_id = dpns.id().to_buffer(); | ||
|
|
||
| drive | ||
| .apply_contract( | ||
| &dpns, | ||
| BlockInfo::default(), | ||
| true, | ||
| None, | ||
| Some(&transaction), | ||
| platform_version, | ||
| ) | ||
| .expect("expected to apply contract"); | ||
|
|
||
| drive | ||
| .refresh_data_contract_cache_from_state( | ||
| contract_id, | ||
| Some(&transaction), | ||
| platform_version, | ||
| ) | ||
| .expect("expected to refresh cache"); | ||
|
|
||
| assert!( | ||
| drive.cache.data_contracts.get(contract_id, true).is_some(), | ||
| "transactional reads must see the refreshed contract" | ||
| ); | ||
| assert!( | ||
| drive.cache.data_contracts.get(contract_id, false).is_none(), | ||
| "the uncommitted contract must not be visible in the global cache" | ||
| ); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Blocking: Delayed DAPI cache fill can overwrite migrated DPNS after promotion
The remove-and-reseed sequence is not synchronized with cache-populating non-transactional reads. After
remove, a DAPI document query can miss the global cache, fetch the old DPNS from committed GroveDB throughget_contract_with_fetch_info_and_add_to_operations_v0, and pause before its unconditional global-cache insertion. The migration meanwhile inserts the new contract into the block cache, andupdate_drive_cachepromotes it throughmerge_and_clear_block_cache; when the query resumes, its later global-cache insert replaces the migrated entry with the old contract. The query service's committed-height retry does not repair this because the cache mutation occurs before the height recheck and is not rolled back; a retry can then read the same stale cache entry. At the next block,clear_block_cacheleaves that global entry intact, so transactional execution can again serialize DPNS documents using the stale00format instead of02, producing node-dependent app hashes. Publication must use per-key synchronization, generation-aware conditional insertion, or equivalent commit/snapshot ordering that prevents a fetch begun against old committed state from overwriting the migrated entry.source: ['codex']