piecrust: reject contract merkle position collisions

[d-sonuga]

Add missing check to reject deployment of a contract that maps to an unavailable Merkle position.

[Copilot]

Pull request overview

This PR hardens Piecrust’s contract storage by rejecting deployments/migrations where a distinct ContractId would map (via position_from_contract) to an already-occupied Contracts Merkle position, preventing silent overwrites/collisions in the sparse Merkle tree.

Changes:

• Add session-level checks to reject new contract deployments whose Merkle position is already occupied (including base commits).
• Add migration/replace-time checks to reject final-ID Merkle position collisions.
• Add regression tests covering same-session, base-session, and migration collision scenarios; document the fix in the changelog.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
piecrust/src/store/session.rs	Adds collision detection for deploy and replace/migrate paths.
piecrust/src/store/tree.rs	Exposes ContractsMerkle::contains_position to query occupied positions.
piecrust/src/store/commit.rs	Adds Commit::contains_contract_position to surface occupancy checks to sessions.
piecrust/tests/deploy.rs	Adds tests asserting collision rejection for same-session and base-root deploys.
piecrust/tests/persistence.rs	Adds migration test for collision rejection on absent-ID migration targets.
piecrust/CHANGELOG.md	Records the behavior change under “Fixed”.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[moCello]

APPROVE — Correct, well-tested fix. A deploy whose id folds onto an already-occupied Merkle position used to silently overwrite the existing leaf; this rejects it, checking both pending session state and the committed base tree (in-memory and disk-reconstruction paths both covered). Both prior-round comments are addressed.

One thing for rollout, not a blocker here: a deploy that collides with an existing contract's Merkle position is now rejected where it used to be accepted (non-colliding deploys are unaffected), so on a live network mixed-version nodes would diverge on such a deploy. When it's pulled into rusk, gate it behind a coordinated activation height. The one note below is minor.

[Neotamandua]

The stringly-typed domain errors are being passed through io::Error::other and then at other places are wrapped as Error::PersistenceError.

The collision case is a deterministic VM/state invariant, not really an I/O persistence failure. The change creates a collision error as io::Error::other(format!(...)), returns it directly from the deploy-side helper, but in other places wraps the same kind of error into Error::PersistenceError from a helper.

I would like to see that we add a typed error variant ourselves, for example:

pub enum Error {
    // ...

    #[error("Contract already exists: {0}")]
    ContractAlreadyExists(ContractId),

    #[error("Contract '{contract_id}' collides at Merkle position {pos}")]
    ContractPositionCollision {
        contract_id: ContractId,
        pos: u64,
    },
}

If I see it correctly, for the “not found” case, we could reuse the existing Error::ContractDoesNotExist(ContractId) variant.

[Neotamandua]

LGTM