updated costs_5#7389
Conversation
Coverage Report for CI Build 29114246895Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Warning No base build found for commit Coverage: 86.537%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
|
It feels a bit like we should have https://github.com/rob-stacks/stacks-core/tree/feat/costs_bench_valgrind PR'd/reviewed/merged before merging in costs based on it? Or, what does everyone else think? |
I'm not too worried about merging that branch before the costs -- historically, the benchmarks lived in a separate repo entirely, and the cost changes were reviewed more or less exactly this way. Not that this is the best process in the world, but I'm not convinced that we should hold up review of the PR to change that. |
There was a problem hiding this comment.
Pull request overview
This PR updates the Clarity 6 / Epoch 4.0 runtime cost schedule (costs_5.rs) by replacing many Costs4 forwards with new bench-derived formulas (plus associated snapshot/changelog updates), based on Callgrind instruction-count measurements from clarity-cost-bench.
Changes:
- Recalibrates many
Costs5runtime cost functions with new constants/linear formulas derived from benchmarking. - Updates an existing chainstate test snapshot to reflect the new runtime cost outputs.
- Adds a changelog fragment documenting the new calibration and a crypto-cost correction.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| clarity/src/vm/costs/costs_5.rs | Replaces many forwarded costs with new bench-derived runtime formulas/constants for Epoch 4.0. |
| stackslib/src/chainstate/tests/snapshots/blockstack_lib__chainstate__tests__static_analysis_tests__tuple_merge_overflow_unused_runtime_ccall.snap | Updates expected runtime cost values to match the recalibrated schedule. |
| changelog.d/costs-5-valgrind-calibration.changed | Documents the calibration methodology/baseline and notable cost changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Updated the tool to correctly profile index-of and comparison functions. Set the baseline to 80k per unit (a bit more conservative). Re-uploaded costs.txt with the new values |
|
Changed base branch to |
|
Throwing another Copilot sanity-check at it quick |
The base branch was changed.
brice-stacks
left a comment
There was a problem hiding this comment.
this looks good, but can we also remove costs-5.clar here (along with its deployment), so that don't end up with a cost contract that doesn't match the actual costs?
With cost-voting removed, we no longer need to publish the costs in a Clarity contract.
2fb646a
into
stacks-network:release/4.0.0
Description
This patch assigns the new runtime costs based on the clarity-cost-bench tool (valgrind based) available at https://github.com/rob-stacks/stacks-core/tree/feat/costs_bench_valgrind.
This is the (current) results/analysis of the tool
costs.txt
Every entry has a detailed comment explaining the logic behind the math/values
Additional Notes
Applicable issues
Additional info (benefits, drawbacks, caveats)
Checklist
docs/property-testing.md)changelog.d/README.md)rpc/openapi.yamlfor RPC endpoints,event-dispatcher.mdfor new events)clarity-benchmarkingrepo