Skip to content

fix(escrow): use TREE_HEIGHT constant in test assertions#78

Open
0xKyungmin wants to merge 2 commits intosolana-foundation:mainfrom
0xKyungmin:fix/release-funds-test-tree-height
Open

fix(escrow): use TREE_HEIGHT constant in test assertions#78
0xKyungmin wants to merge 2 commits intosolana-foundation:mainfrom
0xKyungmin:fix/release-funds-test-tree-height

Conversation

@0xKyungmin
Copy link
Copy Markdown

Summary

Several tests in release_funds.rs and smt_utils.rs hardcode the tree height as 16 (or use indices assuming TREE_HEIGHT >= 16) instead of using the TREE_HEIGHT constant. When compiled with the test-tree feature (TREE_HEIGHT = 3), this causes compilation errors:

error[E0277]: can't compare `[[u8; 32]; 3]` with `[[u8; 32]; 16]`
error: this operation will panic at runtime
  sibling_proofs[10] = test_hash(444);
  index out of bounds: the length is 3 but the index is 10

Without test-tree, the default TREE_HEIGHT = 16 happens to match the hardcoded values, so the errors are currently masked.

Changes

release_funds.rs

  • [2u8; 512] -> [2u8; TREE_HEIGHT * 32]
  • [[2u8; 32]; 16] -> [[2u8; 32]; TREE_HEIGHT]

smt_utils.rs

  • Replace hardcoded sibling proof indices ([5], [7], [10]) with [TREE_HEIGHT - 1]
  • Replace hardcoded corruption indices ([3]) with [TREE_HEIGHT - 1]
  • Replace hardcoded loop bounds (.take(6), .take(8)) with .take(TREE_HEIGHT)

All changes are consistent with the rest of the test suite which already uses the TREE_HEIGHT constant.

Test plan

  • cargo test -p contra-escrow-program — 40 passed
  • cargo test -p contra-escrow-program --features test-tree — 40 passed

Replace hardcoded tree height values with the TREE_HEIGHT constant
in test_process_release_funds_instruction_data_valid to prevent
compilation errors when building with the test-tree feature.
Replace hardcoded sibling proof indices and loop bounds with
TREE_HEIGHT-derived values to prevent out-of-bounds compilation
errors when building with the test-tree feature.
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 16, 2026

Greptile Summary

This PR replaces hardcoded tree-height values (16, 512, specific array indices) in test code with expressions derived from the TREE_HEIGHT constant, enabling compilation under the test-tree feature flag (TREE_HEIGHT = 3).

  • release_funds.rs: Straightforward substitution of [2u8; 512][2u8; TREE_HEIGHT * 32] and [[2u8; 32]; 16][[2u8; 32]; TREE_HEIGHT] in instruction-data parsing tests. No issues.
  • smt_utils.rs: Sibling-proof indices and loop bounds are updated to use TREE_HEIGHT - 1 and .take(TREE_HEIGHT). This fixes compilation, but the two early-termination tests (test_verify_smt_exclusion_proof_early_termination and test_verify_smt_inclusion_proof_early_termination) now iterate over all levels, so they no longer exercise the early-return path in the verifiers.

Confidence Score: 3/5

  • Test-only changes — safe to merge with no production risk, but test coverage for early termination is silently lost.
  • The compilation fix is correct and necessary. However, two early-termination tests are now functionally equivalent to full-proof tests, meaning the early-return code path is no longer covered. This is a test-quality regression, not a production bug.
  • contra-escrow-program/program/src/processor/shared/smt_utils.rs — early termination tests at lines 367 and 527 need adjustment to use a value less than TREE_HEIGHT.

Important Files Changed

Filename Overview
contra-escrow-program/program/src/processor/release_funds.rs Replaces hardcoded 512 and 16 with TREE_HEIGHT * 32 and TREE_HEIGHT in test assertions. Straightforward, correct change.
contra-escrow-program/program/src/processor/shared/smt_utils.rs Replaces hardcoded indices and loop bounds with TREE_HEIGHT-derived values. Fixes test-tree compilation, but early termination tests now iterate all levels, no longer exercising the early-return code path.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Test Setup: sibling_proofs array of size TREE_HEIGHT"] --> B["Compute expected root"]
    B --> C{".take(N) — how many levels?"}
    C -->|"N < TREE_HEIGHT (old behavior)"| D["Intermediate hash at level N"]
    C -->|"N = TREE_HEIGHT (new behavior)"| E["Full root hash at level TREE_HEIGHT"]
    D --> F["Verifier matches at level N → early return Ok"]
    E --> G["Verifier matches at final level → normal return Ok"]
    F --> H["Early termination path exercised ✓"]
    G --> I["Early termination path NOT exercised ✗"]
Loading

Comments Outside Diff (1)

  1. contra-escrow-program/program/src/processor/shared/smt_utils.rs, line 367-397 (link)

    Early termination test no longer tests early termination

    With .take(TREE_HEIGHT) (line 381), this test now computes the hash over all TREE_HEIGHT levels — which is the full root, not an intermediate hash. The verification function (verify_smt_exclusion_proof) also iterates over all TREE_HEIGHT levels, so the computed hash will match only at the final level, meaning no early termination actually occurs. The test still passes, but it is now a duplicate of a standard full-proof verification test, not an early-termination test.

    Previously, .take(8) with TREE_HEIGHT = 16 computed an intermediate hash at level 8, which the verifier would match before reaching the final level — genuinely exercising the early-return path.

    To preserve the original intent, consider using .take(TREE_HEIGHT - 1) (or another value strictly less than TREE_HEIGHT) so that the root is computed at a level before the final one, which will trigger the early return Ok(()) inside the verifier. The same issue applies to test_verify_smt_inclusion_proof_early_termination at line 527.

Last reviewed commit: ad318d2

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant