fix(platform): harden v12 to v13 protocol upgrades#4222
Conversation
Keep exact protocol materializations in the system contract cache so speculative upgrade loading cannot leak a newer contract into historical reads. Test would have caught this in CI: ✖ before fix, explicit v12/v8 reads returned the speculative v13/v9 objects; ✔ after fix, protocol-specific cache, rollback/retry, and upgrade lifecycle tests pass.
Bound config migrations to the requested target version and add the beta.3 repair for exact official :4 Platform image pins while preserving custom registries, digests, and tags. Test would have caught this in CI: ✖ before fix, stable pins remained on :4 and beta.3 migrations ran for beta.2 targets; ✔ after fix, all five focused migration tests pass.
Add the shared review skill, release-tag comparison helper, transition checklist, and reviewed v12-to-v13 design and evidence. Release certification now requires immutable tags; mutable development refs are explicitly limited to pre-release review.
📝 WalkthroughWalkthroughAdds a protocol-upgrade review skill and tooling, protocol-versioned system-contract caching, v12-to-v13 transition and restart tests, and Dashmate beta configuration migration safeguards. ChangesProtocol upgrade validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UpgradeTest
participant FullAbciApplication
participant SystemDataContracts
participant DPNS
UpgradeTest->>FullAbciApplication: run rejected v13 transition
FullAbciApplication->>SystemDataContracts: query protocol-specific contracts
SystemDataContracts-->>FullAbciApplication: preserve committed v12 state
UpgradeTest->>FullAbciApplication: retry transition successfully
FullAbciApplication->>SystemDataContracts: materialize v13 contracts
UpgradeTest->>DPNS: execute post-upgrade transfer
DPNS-->>UpgradeTest: return ownership and history results
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Final review complete — no blockers (commit 8b50539) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4222 +/- ##
===========================================
Coverage ? 87.54%
===========================================
Files ? 2667
Lines ? 338191
Branches ? 0
===========================================
Hits ? 296055
Misses ? 42136
Partials ? 0
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR fixes a real bug where find_by_id() explicit protocol-version reads could observe a speculative reload_system_contracts() materialization for a different protocol version; the versioned BTreeMap cache and new regression tests (upgrade_fork_tests.rs, dpns.rs) correctly demonstrate and close that gap. Two moderate-confidence, non-blocking correctness gaps remain in the new find_by_id lazy-fill path: it doesn't replicate the DocumentHistory activation-version override used by reload_system_contracts, and it silently swallows load_system_data_contract errors as a plain not-found. Neither is exploitable at the current max protocol version (13), so this is a COMMENT rather than a blocker.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (completed),claude-sonnet-5— ffi-engineer (completed)
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/cache/system_contracts.rs`:
- [SUGGESTION] packages/rs-drive/src/cache/system_contracts.rs:260-294: find_by_id lazy-fill doesn't honor the DocumentHistory activation-version override
reload_system_contracts (lines ~124-135) special-cases SystemDataContract::DocumentHistory: no matter which platform_version the reload is invoked with, the contract is always materialized using PlatformVersion::get(DOCUMENT_HISTORY_ACTIVATION_PROTOCOL_VERSION) (pinned to 13), because its schema uses index-aggregation keywords (`averageable`, `rangeCountable`, ...) that only that exact meta-schema recognizes.
The new lazy-fill path in find_by_id doesn't apply this override: when a protocol version isn't yet present in contracts_by_protocol_version, line 288 materializes the missing entry with load_system_data_contract(system_contract, platform_version), where platform_version is derived directly from the requested protocol_version (line 260), not from DOCUMENT_HISTORY_ACTIVATION_PROTOCOL_VERSION.
This is unobservable today because DOCUMENT_HISTORY_ACTIVATION_PROTOCOL_VERSION == 13 == LATEST_PLATFORM_VERSION, so the requested version and the forced override version always coincide. Once protocol v14+ ships (and per the doc comment this contract's schema is not expected to change again), any node that reaches the lazy-fill path for DocumentHistory at v14+ (e.g. a node that only reloaded its current version and is later asked for an older explicit version) will materialize the contract using the requested version's meta-schema instead of the pinned v13 one the normal reload path forces. Two nodes populating the same cache key through the two different code paths (reload vs. lazy-fill) could then diverge on what that key holds, which is exactly the footgun this PR's caching mechanism is meant to eliminate.
- [SUGGESTION] packages/rs-drive/src/cache/system_contracts.rs:288: find_by_id silently treats a load_system_data_contract error as "not a system contract"
Before this PR, find_by_id could not fail: it returned Arc::clone(&active.load()) for any id matching a known system contract's static id. With the new lazy-fill path, once the id is confirmed to match a known SystemDataContract variant and the activation-version check has passed, a failure from load_system_data_contract(...) (e.g. a bug in a future protocol version's contract definition, or a PlatformVersionError) is swallowed via `.ok()?` and the function returns None.
The caller, get_system_or_user_contract_with_fee_v0, treats None as "this id is not a cached system contract" and falls through to the normal fee-charged GroveDB contract fetch, which will most likely also fail to find a system contract id, producing a confusing NotFound instead of surfacing the real underlying load error. Distinguish "id doesn't match any system contract" from "id matches but materialization failed" (e.g. log the swallowed error, or return a Result) so a genuine internal defect isn't silently reinterpreted as an ordinary contract-not-found case.
| let platform_version = PlatformVersion::get(protocol_version).ok()?; | ||
| let (active, system_contract) = if id == SystemDataContract::Withdrawals.id() { | ||
| (&self.withdrawals, SystemDataContract::Withdrawals) | ||
| } else if id == SystemDataContract::MasternodeRewards.id() { | ||
| &self.masternode_reward_shares | ||
| ( | ||
| &self.masternode_reward_shares, | ||
| SystemDataContract::MasternodeRewards, | ||
| ) | ||
| } else if id == SystemDataContract::DPNS.id() { | ||
| &self.dpns | ||
| (&self.dpns, SystemDataContract::DPNS) | ||
| } else if id == SystemDataContract::Dashpay.id() { | ||
| &self.dashpay | ||
| (&self.dashpay, SystemDataContract::Dashpay) | ||
| } else if id == SystemDataContract::TokenHistory.id() { | ||
| &self.token_history | ||
| (&self.token_history, SystemDataContract::TokenHistory) | ||
| } else if id == SystemDataContract::KeywordSearch.id() { | ||
| &self.keyword_search | ||
| (&self.keyword_search, SystemDataContract::KeywordSearch) | ||
| } else if id == SystemDataContract::DocumentHistory.id() { | ||
| &self.document_history | ||
| (&self.document_history, SystemDataContract::DocumentHistory) | ||
| } else { | ||
| return None; | ||
| }; | ||
| if active.active_since_protocol_version > protocol_version { | ||
| return None; | ||
| } | ||
| Some(Arc::clone(&active.load())) | ||
| if let Some(contract) = active.load_for_protocol_version(protocol_version) { | ||
| return Some(contract); | ||
| } | ||
|
|
||
| let contract = Arc::new(load_system_data_contract(system_contract, platform_version).ok()?); | ||
| let mut contracts_by_protocol_version = active.contracts_by_protocol_version.write(); | ||
| Some(Arc::clone( | ||
| contracts_by_protocol_version | ||
| .entry(protocol_version) | ||
| .or_insert(contract), | ||
| )) |
There was a problem hiding this comment.
🟡 Suggestion: find_by_id lazy-fill doesn't honor the DocumentHistory activation-version override
reload_system_contracts (lines ~124-135) special-cases SystemDataContract::DocumentHistory: no matter which platform_version the reload is invoked with, the contract is always materialized using PlatformVersion::get(DOCUMENT_HISTORY_ACTIVATION_PROTOCOL_VERSION) (pinned to 13), because its schema uses index-aggregation keywords (averageable, rangeCountable, ...) that only that exact meta-schema recognizes.
The new lazy-fill path in find_by_id doesn't apply this override: when a protocol version isn't yet present in contracts_by_protocol_version, line 288 materializes the missing entry with load_system_data_contract(system_contract, platform_version), where platform_version is derived directly from the requested protocol_version (line 260), not from DOCUMENT_HISTORY_ACTIVATION_PROTOCOL_VERSION.
This is unobservable today because DOCUMENT_HISTORY_ACTIVATION_PROTOCOL_VERSION == 13 == LATEST_PLATFORM_VERSION, so the requested version and the forced override version always coincide. Once protocol v14+ ships (and per the doc comment this contract's schema is not expected to change again), any node that reaches the lazy-fill path for DocumentHistory at v14+ (e.g. a node that only reloaded its current version and is later asked for an older explicit version) will materialize the contract using the requested version's meta-schema instead of the pinned v13 one the normal reload path forces. Two nodes populating the same cache key through the two different code paths (reload vs. lazy-fill) could then diverge on what that key holds, which is exactly the footgun this PR's caching mechanism is meant to eliminate.
| let platform_version = PlatformVersion::get(protocol_version).ok()?; | |
| let (active, system_contract) = if id == SystemDataContract::Withdrawals.id() { | |
| (&self.withdrawals, SystemDataContract::Withdrawals) | |
| } else if id == SystemDataContract::MasternodeRewards.id() { | |
| &self.masternode_reward_shares | |
| ( | |
| &self.masternode_reward_shares, | |
| SystemDataContract::MasternodeRewards, | |
| ) | |
| } else if id == SystemDataContract::DPNS.id() { | |
| &self.dpns | |
| (&self.dpns, SystemDataContract::DPNS) | |
| } else if id == SystemDataContract::Dashpay.id() { | |
| &self.dashpay | |
| (&self.dashpay, SystemDataContract::Dashpay) | |
| } else if id == SystemDataContract::TokenHistory.id() { | |
| &self.token_history | |
| (&self.token_history, SystemDataContract::TokenHistory) | |
| } else if id == SystemDataContract::KeywordSearch.id() { | |
| &self.keyword_search | |
| (&self.keyword_search, SystemDataContract::KeywordSearch) | |
| } else if id == SystemDataContract::DocumentHistory.id() { | |
| &self.document_history | |
| (&self.document_history, SystemDataContract::DocumentHistory) | |
| } else { | |
| return None; | |
| }; | |
| if active.active_since_protocol_version > protocol_version { | |
| return None; | |
| } | |
| Some(Arc::clone(&active.load())) | |
| if let Some(contract) = active.load_for_protocol_version(protocol_version) { | |
| return Some(contract); | |
| } | |
| let contract = Arc::new(load_system_data_contract(system_contract, platform_version).ok()?); | |
| let mut contracts_by_protocol_version = active.contracts_by_protocol_version.write(); | |
| Some(Arc::clone( | |
| contracts_by_protocol_version | |
| .entry(protocol_version) | |
| .or_insert(contract), | |
| )) | |
| let load_platform_version = if system_contract == SystemDataContract::DocumentHistory { | |
| PlatformVersion::get(DOCUMENT_HISTORY_ACTIVATION_PROTOCOL_VERSION) | |
| .map_err(ProtocolError::from) | |
| .ok()? | |
| } else { | |
| platform_version | |
| }; | |
| let contract = Arc::new(load_system_data_contract(system_contract, load_platform_version).ok()?); |
source: ['claude']
| return Some(contract); | ||
| } | ||
|
|
||
| let contract = Arc::new(load_system_data_contract(system_contract, platform_version).ok()?); |
There was a problem hiding this comment.
🟡 Suggestion: find_by_id silently treats a load_system_data_contract error as "not a system contract"
Before this PR, find_by_id could not fail: it returned Arc::clone(&active.load()) for any id matching a known system contract's static id. With the new lazy-fill path, once the id is confirmed to match a known SystemDataContract variant and the activation-version check has passed, a failure from load_system_data_contract(...) (e.g. a bug in a future protocol version's contract definition, or a PlatformVersionError) is swallowed via .ok()? and the function returns None.
The caller, get_system_or_user_contract_with_fee_v0, treats None as "this id is not a cached system contract" and falls through to the normal fee-charged GroveDB contract fetch, which will most likely also fail to find a system contract id, producing a confusing NotFound instead of surfacing the real underlying load error. Distinguish "id doesn't match any system contract" from "id matches but materialization failed" (e.g. log the swallowed error, or return a Result) so a genuine internal defect isn't silently reinterpreted as an ordinary contract-not-found case.
| let contract = Arc::new(load_system_data_contract(system_contract, platform_version).ok()?); | |
| let contract = match load_system_data_contract(system_contract, platform_version) { | |
| Ok(contract) => Arc::new(contract), | |
| Err(e) => { | |
| tracing::error!( | |
| ?system_contract, | |
| protocol_version, | |
| error = ?e, | |
| "failed to lazily materialize system contract for protocol version" | |
| ); | |
| return None; | |
| } | |
| }; |
source: ['claude']
2 participants
Issue being fixed or feature implemented
The v12-to-v13 upgrade review found two concrete release-path defects:
:4tags.The repository also lacked a reusable, release-tag-pinned workflow for reviewing and rehearsing protocol upgrades end to end.
What was done?
(fromVersion, toVersion]and add a beta.3 repair for exact official stable image pins while preserving custom registries, digests, and tags.The review also confirmed that PoSe-banned validators may retain an earlier protocol vote. That behavior is intentionally unchanged here: v12 and v13 both dispatch the existing v0 methods, so changing persisted vote writes during this rolling release could make mixed binaries compute different roots. It needs a future protocol-versioned method change.
How Has This Been Tested?
All executed checks passed:
cargo test -p drive-abci execution::platform_events::protocol_upgrade --lib -- --nocapture— 43 passedcargo test -p drive cache::system_contracts::tests --lib -- --nocapture— 3 passedcargo test -p drive-abci --test strategy_tests run_chain_v12_to_v13_locks_in_before_activation -- --nocapture— 1 passedcargo test -p drive-abci test_v12_username_survives_retried_v13_transition_and_records_transfer --lib -- --nocapture— 1 passedyarn workspace dashmate exec mocha test/unit/config/configFile/migrateConfigFileFactory.spec.js --reporter spec— 5 passedcargo fmt --all -- --check, andgit diff --checkv4.0.0→v4.1.0-beta.2; non-tag targets fail unless explicitly enabled as pre-release reviewThe live three-validator rehearsal rolled published v4.0.0 Platform images to v4.1.0-beta.2 one validator at a time without stopping Core. It locked at height 82796 (
current=12,next=13), activated at height 82912, and reached height 82949 with every node atcurrent=13,next=13,catchingUp=false, and app hashc6845c8ed89d015f954d05ffbe01f9d7529834f4384415bbc81409cafcddb674.This is not labeled full release certification. Three gates remain explicitly skipped:
Their deterministic contract, restart, cache, rollback, and retry equivalents passed.
Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Tests