Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces an indirection layer for v2 contract sector roots to make v2 contract renewals/refreshes constant-time by avoiding full root-row duplication, switching to a copy-on-write model keyed by a new contract_v2_roots_map table and map “revision numbers”.
Changes:
- Add
contract_v2_roots_mapand update v2 schema/migration to reference roots via (map_id, map_revision_number) instead of per-contract duplication. - Update v2 renewal/sector-root logic to share a roots-map across renewals (and adjust related benchmarks/tests).
- Add/adjust tests to validate pruning and root consistency under the new model.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
persist/sqlite/contracts.go |
Core implementation: v2 roots-map allocation, renew behavior, root lookup, and sector expiry/GC logic. |
persist/sqlite/migrations.go |
Adds migration v49 to create the roots-map table and migrate existing v2 contracts/roots. |
persist/sqlite/init.sql |
Updates fresh DB schema for v2 contracts/sector roots to use roots-map indirection. |
host/contracts/manager.go |
Loads roots at startup using the updated store API and updates v2 renewal path. |
host/contracts/persist.go |
Adjusts ContractStore interface (notably sector-roots loading and renew signature). |
host/contracts/update.go |
Updates lifecycle processing to only expire v2 contract sectors. |
persist/sqlite/volumes_test.go |
Updates pruning test to use v2 contracts and v2 expiry path. |
persist/sqlite/contracts_test.go |
Updates v2 root-consistency test to use V2SectorRoots() and maintain filesize. |
host/contracts/manager_test.go |
Updates storage metrics expectations and adds extensive v2 roots consistency tests. |
.changeset/...md |
Adds a changeset describing the performance improvement and behavioral change. |
Comments suppressed due to low confidence (1)
persist/sqlite/contracts.go:1183
updateV2ContractSectorsno longer deletes sector-root rows when a contract is truncated (newRoots shorter than oldRoots). Since pruning andHasSectortreat any row incontract_v2_sector_rootsas a reference, truncated sectors will remain referenced indefinitely and will not be eligible for pruning, causing storage leaks and incorrect reference tracking. Reintroduce deletion of rows for the current (map_id, map_revision_number) whereroot_index >= len(newRoots)when the contract shrinks.
func updateV2ContractSectors(tx *txn, contractDBID int64, oldRoots, newRoots []types.Hash256) error {
selectRootIDStmt, err := tx.Prepare(`SELECT id FROM stored_sectors WHERE sector_root=?`)
if err != nil {
return fmt.Errorf("failed to prepare select root ID statement: %w", err)
}
defer selectRootIDStmt.Close()
updateRootStmt, err := tx.Prepare(`INSERT INTO contract_v2_sector_roots (contract_v2_roots_map_id, contract_v2_roots_map_revision_number, sector_id, root_index) VALUES (?, ?, ?, ?) ON CONFLICT (contract_v2_roots_map_id, contract_v2_roots_map_revision_number, root_index) DO UPDATE SET sector_id=excluded.sector_id`)
if err != nil {
return fmt.Errorf("failed to prepare update root statement: %w", err)
}
defer updateRootStmt.Close()
var contractMapID, contractMapRevisionID int64
err = tx.QueryRow(`SELECT contract_v2_roots_map_id, contract_v2_roots_map_revision_number FROM contracts_v2 WHERE id=$1`, contractDBID).Scan(&contractMapID, &contractMapRevisionID)
if err != nil {
return fmt.Errorf("failed to get contract map ID: %w", err)
}
for i, root := range newRoots {
if i < len(oldRoots) && oldRoots[i] == root {
continue
}
var newSectorID int64
if err := selectRootIDStmt.QueryRow(encode(root)).Scan(&newSectorID); err != nil {
return fmt.Errorf("failed to get sector ID: %w", err)
} else if _, err := updateRootStmt.Exec(contractMapID, contractMapRevisionID, newSectorID, i); err != nil {
return fmt.Errorf("failed to update sector root: %w", err)
}
}
delta := len(newRoots) - len(oldRoots)
if err := incrementNumericStat(tx, metricContractSectors, delta, time.Now()); err != nil {
return fmt.Errorf("failed to update contract sectors: %w", err)
}
return nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
peterjan
left a comment
There was a problem hiding this comment.
I think it's solid. I'm going to go through it again tomorrow with a fresh set of eyes though. If it's not merged yet by then.
ChrisSchinnerl
left a comment
There was a problem hiding this comment.
Wasn't able to get through all of it today but leaving what I have so far.
ChrisSchinnerl
left a comment
There was a problem hiding this comment.
Apart from remaining not getting decremented this looks good to me.
db74ed0 to
446bb12
Compare
|
I've changed the contract sectors metric to track based on the file size of active contracts instead. If a contract is not active, it should not contribute to the metrics until it is active. After a contract is resolved, it will be removed from the metrics even if the sectors are still referenced in the database. This is consistent with the intent of the metric and simpler than trying to keep it synced across map revisions while handling orphans from renewals and rejections without double counting. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Claude Code ReviewCode review was not run because your organization is out of credits. An admin can add credits from the billing page in the Anthropic Console. Once credits are available, push a new commit or reopen this pull request to trigger a review. |
ChrisSchinnerl
left a comment
There was a problem hiding this comment.
I have deployed this on my personal 2TB host. Migration was fast and data integrity checks that hosts perform seem to pass so far. Refreshes happen almost once a day so we'll see how that goes soon.
|
@claude review |
|
@claude review |
There was a problem hiding this comment.
This is a large, architecturally significant PR that fundamentally changes the contract sector storage model with copy-on-write semantics, a new indirection table, and schema migration — it warrants careful human review given its self-described criticality.
Extended reasoning...
Overview
This PR introduces a contract_v2_roots_map table as an indirection layer between contracts and their sector roots, making renewals/refreshes O(1) instead of O(n). It touches 12 files across the contracts manager, persist layer, SQLite consensus handling, migrations, recalc logic, and tests. The schema migration (v49) renames existing tables and rebuilds with new columns (sector_count, contract_v2_roots_map_id, contract_v2_roots_map_revision_number). The copy-on-write root resolution query relies on SQLite-specific bare column behavior with MAX() aggregates.
Security risks
No direct injection or auth bypass risks. The main risk is data integrity — incorrect sector root resolution could lead to data loss or corruption for hosted files. The MAX(id)+1 pattern for ID generation is safe given SQLite's serialized transaction model (as discussed in the PR), but the copy-on-write query correctness is critical. The expiry queries (expiredQuery and supersededQuery in deleteExpiredV2ContractSectors) must correctly identify which roots are safe to delete without orphaning data relied upon by active contracts.
Level of scrutiny
This deserves thorough human review. The PR author explicitly warns "Please review extensively. This is a critical component with lots of side effects. There be lots of dragons here." The copy-on-write sector root model is a fundamental architectural change, and the SQL queries implementing root resolution, expiry, and supersession are non-trivial. The reliance on SQLite-specific bare column semantics (documented but unusual) adds a portability concern worth a human judgment call.
Other factors
The PR has had extensive review from peterjan, ChrisSchinnerl, and chris124567, with all inline comments now resolved. Test coverage was significantly expanded with TestV2SectorRootConsistency covering many edge cases (renewal chains, rejected contracts, sector deletion). The ORDER BY was correctly moved to the outer query per reviewer feedback. The two pre-existing dead code bugs in revertFailedV2Contracts and revertFailedContracts (exhaustive if/else-if making subsequent code unreachable) mean the new metricContractSectors increments added at lines 1799 and ~1276 will never execute during chain reorgs — these are flagged as inline comments.
Contract renewals and refreshes previously copied every sector root row when creating a new contract. For hosts with large contracts, this locked the database for a long time and caused performance issues. The original design traded off simplicity for performance while ensuring hosts could fall back to the previous contract if a renewal was rejected on-chain.
Added a
contract_v2_roots_maptable that acts as an indirection layer between contracts and their sector roots. When a contract is renewed or refreshed, the new contract shares the same roots map as the old one at a higher "revision number" instead of duplicating all the rows. Sector roots are then copy-on-write — only modified sectors get new rows at the new window. If a renewed contract is rejected, the original can still see its "view" of the sectors.This does remove a nice guarantee of the old design where one sector root in a contract is guaranteed one row.
Please review extensively. This is a critical component with lots of side effects. There be lots of dragons here.