Skip to content

Conversation

hstove
Copy link
Contributor

@hstove hstove commented Sep 9, 2025

This PR implements a fix for throwing the expected error (UndefinedVariable) instead of a CostError when the contract contains a top-level variable that isn't defined.

@hstove hstove requested review from a team as code owners September 9, 2025 22:06
@hstove hstove linked an issue Sep 9, 2025 that may be closed by this pull request
jferrant
jferrant previously approved these changes Sep 10, 2025
@kantai
Copy link
Contributor

kantai commented Sep 10, 2025

Is this change in error types consensus-breaking? I think it would be good to add a convincing test that demonstrates that it is not consensus breaking.

It should be possible to setup a test kind of like this one (https://github.com/stacks-network/stacks-core/blob/master/stackslib/src/chainstate/nakamoto/coordinator/tests.rs#L880) where instead of producing a contract which uses block-info, we have a contract that exercises the error. Then, after the block is processed, the test can assert that the transaction is processed (or not processed) with a given return value, the evaluated cost matches some exact number, and maybe even that the block hash matches an exact hash. Then we could run the test in develop (with a local cherry-pick) and on this branch and ensure it passes in both places.

kantai
kantai previously approved these changes Sep 12, 2025
jferrant
jferrant previously approved these changes Sep 12, 2025
@hstove hstove dismissed stale reviews from jferrant and kantai via 424ec11 September 15, 2025 14:26
@hstove hstove requested review from jferrant and kantai September 15, 2025 14:26
@hstove
Copy link
Contributor Author

hstove commented Sep 15, 2025

@kantai @jferrant mind re-reviewing? I had a conflict that needed to be fixed.

@hstove hstove added this pull request to the merge queue Sep 16, 2025
Merged via the queue into stacks-network:develop with commit a8aa32b Sep 16, 2025
296 of 299 checks passed
@hstove hstove deleted the fix/cost-error-undefined-var branch September 16, 2025 03:35
Copy link

codecov bot commented Sep 16, 2025

Codecov Report

❌ Patch coverage is 20.65217% with 73 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.33%. Comparing base (c1a989f) to head (424ec11).
⚠️ Report is 11 commits behind head on develop.

Files with missing lines Patch % Lines
stacks-node/src/tests/signer/v0.rs 0.00% 64 Missing ⚠️
stackslib/src/chainstate/stacks/miner.rs 0.00% 7 Missing ⚠️
stackslib/src/clarity_vm/tests/analysis_costs.rs 83.33% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6448      +/-   ##
===========================================
+ Coverage    78.86%   79.33%   +0.46%     
===========================================
  Files          558      558              
  Lines       343239   343329      +90     
===========================================
+ Hits        270699   272372    +1673     
+ Misses       72540    70957    -1583     
Files with missing lines Coverage Δ
clarity/src/vm/analysis/type_checker/v2_05/mod.rs 90.45% <100.00%> (+0.04%) ⬆️
clarity/src/vm/analysis/type_checker/v2_1/mod.rs 90.37% <100.00%> (+0.02%) ⬆️
...c/vm/analysis/type_checker/v2_1/tests/contracts.rs 95.87% <100.00%> (+<0.01%) ⬆️
stackslib/src/clarity_vm/tests/analysis_costs.rs 98.14% <83.33%> (-0.88%) ⬇️
stackslib/src/chainstate/stacks/miner.rs 80.36% <0.00%> (+3.73%) ⬆️
stacks-node/src/tests/signer/v0.rs 0.46% <0.00%> (-4.15%) ⬇️

... and 95 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1a989f...424ec11. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected error for undefined variable
3 participants