Skip to content

Constant time contract renewals and refreshes#938

Open
n8mgr wants to merge 11 commits intomasterfrom
nate/constant-renewals
Open

Constant time contract renewals and refreshes#938
n8mgr wants to merge 11 commits intomasterfrom
nate/constant-renewals

Conversation

@n8mgr
Copy link
Member

@n8mgr n8mgr commented Mar 5, 2026

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_map table 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.

@n8mgr n8mgr self-assigned this Mar 5, 2026
Copilot AI review requested due to automatic review settings March 5, 2026 00:36
@n8mgr n8mgr moved this to In Review in Sia Mar 5, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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_map and 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

  • updateV2ContractSectors no longer deletes sector-root rows when a contract is truncated (newRoots shorter than oldRoots). Since pruning and HasSector treat any row in contract_v2_sector_roots as 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) where root_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.

Copy link
Member

@peterjan peterjan left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ChrisSchinnerl ChrisSchinnerl left a comment

Choose a reason for hiding this comment

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

Wasn't able to get through all of it today but leaving what I have so far.

Copy link
Member

@ChrisSchinnerl ChrisSchinnerl left a comment

Choose a reason for hiding this comment

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

Apart from remaining not getting decremented this looks good to me.

@n8mgr
Copy link
Member Author

n8mgr commented Mar 10, 2026

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.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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
Copy link

claude bot commented Mar 11, 2026

Claude Code Review

Code 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.

Copy link
Member

@ChrisSchinnerl ChrisSchinnerl left a comment

Choose a reason for hiding this comment

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

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.

@n8mgr
Copy link
Member Author

n8mgr commented Mar 12, 2026

@claude review

@n8mgr
Copy link
Member Author

n8mgr commented Mar 13, 2026

@claude review

Copy link

@claude claude bot 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 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

5 participants