fix: make 1-based indexing in tbls implementation

[varex83]

Fixes the implementation difference between Rust and Go pointed by @emlautarom1 in the PR

[emlautarom1]

LGTM. The tests that rely on test-cluster where the issue was found work as intended, so we're good to go.

[varex83]

@claude can you review this PR?

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[varex83]

@claude review this PR

[claude]

Claude finished @varex83's task in 2m 47s —— View job

---

PR Review

• Gather context and understand the request
• Read changed files
• Analyze indexing fix correctness
• Compare logic against Go behavior
• Provide review feedback

---

Summary

This PR fixes a parity gap with the Go (charon) implementation: the Rust threshold_split was inserting shares at 0-based keys (0, 1, …, n-1) instead of the correct 1-based polynomial evaluation points (1, 2, …, n). The callers in recover_secret, threshold_aggregate, and test_cluster.rs were compensating with internal +1 conversions; those compensations are now correctly removed. The exit.rs caller, which was relying on the now-removed internal conversion, is also correctly updated.

---

Findings

[Low] MathError::IntegerOverflow is now dead code

The variant MathError::IntegerOverflow was used in the previous versions of recover_secret and threshold_aggregate to signal the checked_add(1) overflow. Both of those paths are removed by this PR.

Evidence: crates/crypto/src/types.rs:201

The variant is still declared but never constructed anywhere in the workspace. Because it's pub, the compiler won't emit a dead_code warning, but it's confusing to future readers. Consider removing it or documenting why it's kept. Fix this →

---

[Low] HashMap key/value iteration order in recover_secret and threshold_aggregate

This is a pre-existing pattern (not introduced by this PR) but worth noting:

// crates/crypto/src/blst_impl.rs:115-122
let share_points: Vec<Index> = shares.keys().copied().collect();
let share_secrets: Vec<BlstSecretKey> = shares.values().map(…).collect()?;

The Lagrange interpolation at lagrange_interpolate_secret(&share_points, &share_secrets) requires that share_points[i] corresponds to share_secrets[i]. Collecting .keys() and .values() independently relies on both iterating the HashMap in the same order. Rust's std::collections::HashMap does guarantee consistent ordering between .keys() and .values() for the same map instance in a single-threaded context, but this is fragile. Prefer:

let (share_points, share_secrets): (Vec<Index>, Vec<BlstSecretKey>) = shares
    .iter()
    .map(|(&idx, bytes)| {
        BlstSecretKey::from_bytes(bytes)
            .map(|sk| (idx, sk))
            .map_err(|e| Error::InvalidSecretKey(e.into()))
    })
    .collect::<Result<Vec<_>, _>>()?
    .into_iter()
    .unzip();

The same applies to threshold_aggregate (indices + signatures).

---

Parity Matrix

Component	Go	Rust (before)	Rust (after)	Match
Share IDs from threshold_split	1-indexed	0-indexed	1-indexed	✅
recover_secret input contract	1-indexed	0-indexed (internal +1)	1-indexed	✅
threshold_aggregate input contract	1-indexed	0-indexed (internal +1)	1-indexed	✅
exit.rs API array → share ID conversion	+1 at call site	missing	+1 at call site	✅
test_cluster.rs share access	shares[i+1]	shares[i]	shares[i+1]	✅

---

Tests

• test_threshold_split_returns_1_indexed_keys — new test directly verifies the 1-based indexing invariant. ✅
• test_verify_aggregate_from_data — contains a hardcoded Go-derived signature vector; this test passing confirms end-to-end parity with the Go implementation. ✅
• test_threshold_split_and_recover, test_recover_secret_with_all_shares, test_threshold_aggregate_matches_direct_sign — roundtrip tests all exercise the corrected paths. ✅

---

Overall

The core fix is correct and the approach is clean. The review feedback from @iamquang95 has been addressed with proper typed error variants (FailedToConvertShareIndexToU8, MathOverflow). The only actionable item is cleaning up the now-unused MathError::IntegerOverflow variant; everything else is low-severity or pre-existing.

[emlautarom1]

Guess we need to remove MathError::IntegerOverflow now.