fix(tests): add missing min_cost parameter to evalTCOExpression FFI binding#355
Open
tvolk131 wants to merge 1 commit intoBlockstreamResearch:masterfrom
Open
fix(tests): add missing min_cost parameter to evalTCOExpression FFI binding#355tvolk131 wants to merge 1 commit intoBlockstreamResearch:masterfrom
min_cost parameter to evalTCOExpression FFI binding#355tvolk131 wants to merge 1 commit intoBlockstreamResearch:masterfrom
Conversation
…FI binding The C function `evalTCOExpression` gained a `minCost` parameter (between `len` and `budget`), but the Rust FFI binding was not updated to match. This caused undefined behavior due to argument mismatch in the foreign function call. Changes: - Add `min_cost: ubounded` parameter to `simplicity_evalTCOExpression` extern declaration to match the C signature. - Pass `0` as `min_cost` in the `simplicity_evalTCOProgram` wrapper, matching the C inline `evalTCOProgram` helper in eval.h. - Fix `budget.map(|b| b as *const _)` to `budget.as_ref().map(|b| b as *const _)` in `run_program` to avoid moving the `Option<u32>` value and instead take a reference to its contents, producing a valid pointer to the stack-local budget value. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes undefined behavior in the simplicity-sys test harness by aligning the Rust test-only FFI binding for evalTCOExpression with the actual C signature and correcting how an optional budget pointer is derived.
Changes:
- Add the missing
min_cost: uboundedparameter to thesimplicity_evalTCOExpressionextern declaration insimplicity-sys/src/tests/ffi.rs. - Update the
simplicity_evalTCOProgramwrapper to passmin_cost = 0, matching the C inline helper behavior. - Fix
run_programto usebudget.as_ref().map(...)so the budget pointer refers to valid memory instead of casting the integer value into an address.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| simplicity-sys/src/tests/mod.rs | Fixes construction of budget_ptr to correctly borrow the optional budget value when passing into FFI. |
| simplicity-sys/src/tests/ffi.rs | Updates the FFI signature to include min_cost and adjusts the Rust wrapper call to match the C ABI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The C function
rustsimplicity_0_6_evalTCOExpressionhas aminCostparameter (of typeubounded) betweenlenandbudget, but the Rust FFI binding in the test module was missing this parameter. This caused an argument mismatch in the foreign function call, leading to undefined behavior at runtime.min_cost: uboundedparameter to thesimplicity_evalTCOExpressionextern declaration insimplicity-sys/src/tests/ffi.rsto match the C signature ineval.h.0asmin_costin thesimplicity_evalTCOProgramRust wrapper, matching the behavior of the C inlineevalTCOProgramhelper ineval.h.budget.map()tobudget.as_ref().map()inrun_program(simplicity-sys/src/tests/mod.rs) — the previous code moved theOption<u32>value rather than borrowing it, which produced a dangling pointer instead of a valid pointer to the stack-local budget value.Context
The C signatures (from
depend/simplicity/eval.h):Test plan
cargo test) to confirm no regressions🤖 Generated with Claude Code