Fix BLS host functions panicking on non-BLS public keys

[janezpodhostnik]

Closes: #8649

VerifyPOP and AggregatePublicKeys panicked on valid non-BLS public keys (e.g. ECDSA), reachable from Cadence because neither stdlib entry point gates on the signature algorithm. For BLS.aggregatePublicKeys this violated the documented nil return; for PublicKey.verifyPoP the abort is documented, but it surfaced as a panic asserting a false invariant.

Changes

• Both functions gate on SignAlgo and return a NewValueErrorf for non-BLS keys, following the VerifySignatureFromRuntime pattern
• Decode failures propagate invalid-input errors as NewValueErrorf, panic only on unexpected crypto-module errors
• Tests: unit coverage for both host functions, plus e2e assertions that scripts now surface the controlled error / documented nil

For aggregatePublicKeys the Cadence stdlib maps any host error to nil, so this restores the documented behavior with no Cadence change. Gating in the Cadence stdlib remains as optional defense in depth.

⚠️ Requires HCU

This changes execution results and must ship via a Height Coordinated Upgrade:

• BLS.aggregatePublicKeys with non-BLS keys previously aborted the entire transaction (host panic recovered by Cadence); it now returns the documented nil and execution continues. Transactions handling the optional (e.g. ?? fallback) can now commit where they previously failed.
• PublicKey.verifyPoP with a non-BLS key still aborts, but the reported ErrorCode changes from 1101 (CadenceRunTimeError) to 1051 (ValueError), which downstream systems index. E2e tests pin both behaviors.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

Summary by CodeRabbit

• Bug Fixes

  • BLS cryptographic operations now reject non-BLS and malformed keys with clear validation errors instead of unexpectedly crashing.
  • Public-key aggregation reports which inputs are invalid and preserves expected runtime error details.
  • Error responses use valid, consistently formatted encodings compatible with downstream processing.

• Tests

  • Added coverage for valid BLS keys, non-BLS key rejection, malformed key handling, proof-of-possession verification, aggregation, and error-message consistency.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 87f73c33-0954-49b9-820f-4f326d4a598b

📥 Commits

Reviewing files that changed from the base of the PR and between fbb1351 and 7782541.

📒 Files selected for processing (1)

• fvm/crypto/crypto_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• fvm/crypto/crypto_test.go

---

📝 Walkthrough

Walkthrough

BLS host functions now reject non-BLS and invalid public keys with controlled errors. Tests cover successful BLS inputs, non-BLS inputs, UTF-8 and CBOR error handling, and signature behavior.

Changes

BLS host input validation

Layer / File(s)	Summary
Host function error handling fvm/crypto/crypto.go	VerifyPOP and AggregatePublicKeys validate key algorithms and return value errors for recognized invalid key inputs.
Behavior and encoding coverage fvm/crypto/crypto_test.go, fvm/fvm_signature_test.go	Tests cover valid BLS keys, non-BLS keys, UTF-8 and CBOR-safe errors, and expected signature integration messages.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the primary change: preventing BLS host functions from panicking on non-BLS public keys.
Linked Issues check	✅ Passed	The changes address issue #8649 by returning controlled errors for non-BLS and invalid keys while preserving nil aggregation behavior.
Out of Scope Changes check	✅ Passed	All code and test changes directly support the linked issue and stated objectives.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch janezpodhostnik/8649-bls-host-fns-non-bls-keys

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov-commenter]

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
fvm/crypto/crypto.go	86.66%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@fvm/crypto/crypto_test.go`:
- Around line 563-569: Update the test’s pk.SignAlgo value to
runtime.SignatureAlgorithmBLS_BLS12_381 so VerifyPOP and AggregatePublicKeys
reach crypto.DecodePublicKey with the invalid PublicKey bytes and exercise the
intended decode-error path.

In `@fvm/crypto/crypto.go`:
- Line 263: In fvm/crypto/crypto.go, guard the public-key parameter before the
SignAlgo access at lines 263-263 and return a NewValueErrorf for nil pk; also
guard each key before key.SignAlgo at lines 313-313 and return an indexed
NewValueErrorf identifying the nil entry. Ensure both cases are treated as input
validation errors rather than panics or unexpected crypto failures.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0a57a153-7505-4f73-9812-83917f8a13c9

📥 Commits

Reviewing files that changed from the base of the PR and between b73fea2 and 8c4599c.

📒 Files selected for processing (3)

• fvm/crypto/crypto.go
• fvm/crypto/crypto_test.go
• fvm/fvm_signature_test.go

[claude]

FVM review — 4 findings, 1 of them important

The fix itself is the right shape: a non-BLS runtime.PublicKey is user input, so a CodedError (not a panic) is the correct classification, and genuine crypto-module exceptions still panic. My concern is deployment coordination, not the code.

Important

1. This changes execution results and needs a coordinated height upgrade (HCU); the PR description doesn't say so. fvm/crypto/crypto.go:313, fvm/crypto/crypto.go:263

Two distinct result changes:

• BLS.aggregatePublicKeys control flow changes, not just its message. Before: any non-BLS key panicked, Cadence recovered it into a runtime.Error, and the whole transaction/script aborted. After: the host returns a value error, the stdlib maps it to nil, and execution continues. The e2e test at fvm/fvm_signature_test.go:717 only fails now because the script force-unwraps with !. A transaction written as BLS.aggregatePublicKeys(pks) ?? fallback previously always aborted and can now commit — with different state changes and events. That is a strictly larger change than an error-message change.
• The reported ErrorCode on the verifyPoP path very likely changes from 1101 to 1051. findRootCodedError (fvm/errors/errors.go:56-74) reports the deepest coded error, and fvm/errors/errors_test.go:93-99 shows a CodedError nested inside a Cadence ExternalError is picked up in preference to ErrCodeCadenceRunTimeError. Previously the panic carried no coded error, so the result was ErrCodeCadenceRunTimeError (1101); now it carries ErrCodeValueError (1051). TransactionResult.ErrorCode is indexed by downstream systems.

Failure scenario: two ENs on either side of this change execute the same block containing a transaction that passes an ECDSA PublicKey to BLS.aggregatePublicKeys. The old EN produces a failed transaction with no state delta; the new EN produces a committed transaction with state changes and events. Different execution results for the same block — an execution fork. Please state the HCU requirement in the description and route it through the normal HCU process.

Nits

1. fvm/fvm_signature_test.go:505,717 — both e2e assertions check only message text. Since the error code is the part downstream systems contract on, assert it too (e.g. via errors.Find(output.Err, errors.ErrCodeValueError)). That pins whether the code moved 1101 → 1051 and turns the finding above into a regression test.
2. fvm/crypto/crypto_test.go:529-538 — the "control: valid BLS keys" case generates both keys from the same seed, so it aggregates one key with itself. Use distinct seeds so the control actually exercises multi-key aggregation.
3. fvm/crypto/crypto.go:269-276 vs fvm/crypto/crypto.go:169-172 — the new decode paths panic unless crypto.IsInvalidInputsError, while VerifySignatureFromRuntime returns a value error for any decode failure. The new behavior is the stricter and better one, but two adjacent functions now classify the same underlying failure differently; worth a comment or aligning them.
4. fvm/fvm_signature_test.go:719 — asserting on "unexpectedly found nil while forcing an Optional value" couples the test to Cadence-internal wording. Test-only, low risk.

Verified and found correct

• Metering is unchanged. Both host functions charge a flat Intensity: 1 before dispatching into fvm/crypto (fvm/environment/crypto_library.go:263-273, :305-315), so the new early return cannot change computation usage, and there is no warm/cold cache asymmetry.
• No CodedFailure is downgraded. The new returns are CodedError for genuine user input; unexpected crypto-module errors still panic and remain failures.
• No regression for inputs that previously worked. ECDSA P-256 / secp256k1 encodings are 64 bytes and cannot decode as a 96-byte BLS key, so the new SignAlgo gate rejects only inputs that previously panicked.
• No new nondeterminism. hex.EncodeToString and the wrapped crypto error text are deterministic; no map iteration or ambient state enters the error path, so SPoCK read/write sets are unaffected.

_{Produced in collaboration with Claude.}

[janezpodhostnik]

@claude 50c7001

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[claude]

FVM review

4 findings, 1 important — no production bug in the diff itself; the important one is that the e2e assertion meant to pin the HCU-relevant behavior doesn't actually pin anything.

The core change is sound: both host functions now gate on SignAlgo before decoding, mirroring VerifySignatureFromRuntime, and the PR body correctly flags this as HCU-requiring.

Important

fvm/fvm_signature_test.go:721 — the aggregatePublicKeys error-code assertion is vacuous.

HasErrorCode walks the whole error chain, and every script error already contains a CadenceRunTimeError: fvm/runtime/wrapped_cadence_runtime.go:84 routes the runtime error through errors.HandleRuntimeError, which unconditionally wraps it as ErrCodeCadenceRunTimeError (fvm/errors/errors.go:139), and SplitErrorTypes (fvm/script.go:154) then wraps that whole chain. So HasErrorCode(output.Err, ErrCodeCadenceRunTimeError) is true for any failing script.

Concretely: if a future Cadence bump stopped mapping the host error to nil and instead propagated it as an abort, aggregatePublicKeys would go back to aborting the transaction — a silent execution-result change on a path this PR just declared HCU-relevant — and this assertion would still pass. The only assertion doing real work there is the ErrorContains on the nil-force message.

The reported code that downstream systems index is output.Err.Code() (the deepest coded error, per findRootCodedError at fvm/errors/errors.go:56). Pinning it needs:

assert.Equal(t, fvmErrors.ErrCodeCadenceRunTimeError, output.Err.Code())

Nits

1. fvm/fvm_signature_test.go:509 — same mechanism, less severe. This assertion is meaningful (on master the chain has no ValueError at all, so it does catch the change), but the comment above it — "reported as a value error, not a generic Cadence runtime error" — is not what's tested: HasErrorCode(output.Err, ErrCodeCadenceRunTimeError) is simultaneously true. Since the PR body's HCU note is specifically about the reported code changing 1101 → 1051, use assert.Equal(t, fvmErrors.ErrCodeValueError, output.Err.Code()).

2. fvm/crypto/crypto.go:308-310 — the new doc says errors are "NewValueErrorf or a propagated crypto module error for any user error", but the two propagated crypto errors at crypto.go:339-340 (BLSAggregateEmptyList, notBLSKey) are uncoded. If either ever reached the FVM boundary, SplitErrorTypes → AsFailure → NewUnknownFailure (fvm/errors/errors.go:94) would classify it as a fatal CodedFailure, not a user error. They are only safe today because Cadence discards the error entirely. Given this PR is precisely about fixing classification in these two functions, wrapping those two in NewValueErrorf would make the doc true and remove the latent trap.

3. fvm/crypto/crypto.go:271-273 and 324-326 — "any other decode error is unexpected and escalated". It is not escalated in the FVM taxonomy sense: Cadence recovers Go error panics, and HandleRuntimeError then maps them to a non-fatal CadenceRunTimeError (1101), i.e. a user-visible error that lets the transaction commit with fees deducted. The pre-existing e2e test on master demonstrates exactly this (it asserted err == nil, output.Err != nil against the old panic). Suggest either rewording to "panics" or returning errors.NewUnknownFailure(err) so it actually becomes a CodedFailure. This is the file's established pattern, so not introduced here — but the new comment asserts a property the code does not have.

4. HCU note is missing a third behavior change. The PR body covers non-BLS SignAlgo only. Keys declaring SignAlgo == BLS whose bytes fail to decode also change: crypto.go:275 and crypto.go:328 now return a ValueError where master panicked (→ nil for aggregate, → 1051 instead of 1101 for verifyPoP). Unreachable through Cadence today since PublicKey construction pre-validates — which the TODO at crypto.go:320 already notes — but the new unit test constructs exactly such a key, so the path is live and worth one line in the HCU section.

Pre-existing

fvm/environment/crypto_library.go:312 — the whole design here leans on Cadence discarding the host error and returning nil. That also discards the ComputationLimitExceeded error from MeterComputation a few lines up: a transaction that blows its computation limit inside BLS.aggregatePublicKeys gets nil back and keeps executing, only failing at the next metered operation (or not at all, if it is the last one). Deterministic, so not a consensus hazard, but a limit-enforcement gap. Not introduced by this PR; noting it because the PR now routes more cases onto that swallow path.

Verified and found correct

• Metering is unchanged: both wrappers charge before the new gate, with fixed Intensity: 1 (crypto_library.go:262-272, 305-313), so no HCU-relevant metering delta.
• %w inside NewValueErrorf does wrap — NewCodedError bottoms out in fmt.Errorf (fvm/errors/errors.go:266).
• Error-message determinism: the new messages are hex-encoded key bytes plus a loop index, both deterministic; UTF-8/CBOR safety is covered by the new test.
• No callers of crypto.VerifyPOP / crypto.AggregatePublicKeys outside fvm/environment/crypto_library.go.

Note: go test and go vet were not runnable in this review sandbox, so all of the above is static analysis — the statements about test outcomes are reasoning from the error-handling code paths, not from an executed run.

[turbolent]

👍