Skip to content

Conversation

Jiloc
Copy link
Contributor

@Jiloc Jiloc commented Aug 27, 2025

Description

This PR adds test coverage for consensus-critical error conversions in clarity-serialization, specifically to address and prevent the type of consensus-breaking bug introduced in the first implementation of consuming clarity-serialization from clarity (#6310

One of the main mistakes was incorrectly treating InterpreterError::Expects as CheckErrors::Expect during the conversion from CodecError -> Error. This introduced consensus-breaking behavior.

The goal of this PR is to systematically add tests that:

  • Succeed under the current implementation of clarity.
  • Fail under the original buggy implementation (where InterpreterError was threaded incorrectly).
  • Succeed in the latest implementation of feat: use clarity-serialization in clarity #6310 proving that the bug is resolved and that our test suite enforces these consensus-critical invariants going forward.

Currently, unit test coverage in clarity-serialization is minimal (~10%), did not attempt to add full test coverage for all functions moved into clarity-serialization. I limited scope to the moved functions whose returned InterpeterErrors. I also marked the ones that are consensus-critical.

I spent a bit of time in looking into which functions were actually affected by the change in Error types and identify all that are consensus-critical. Below is a subset.

Function Return type covered by Tests? Returns InterpreterError?
StandardPrincipalData::new ✅ Yes ✅ Yes
SequenceData::element_size ❌ No
SequenceData::element_at ✅ Yes ✅ Yes
SequenceData::replace_at ❌ No
SequenceData::contains ❌ No
SequenceData::concat ❌ No
SequenceData::slice ❌ No
SequencedValue::type_signature ❌ No
Value::some ✅ Yes (already covered by test_constructors)
Value::okay ✅ Yes (already covered by test_constructors)
Value::error ✅ Yes (already covered by test_constructors)
Value::cons_list ❌ No
Value::buff_from ✅ Yes (already covered by test_constructors)
Value::string_ascii_from_bytes ✅ Yes (already covered by test_constructors)
Value::string_utf8_from_bytes ❌ No
Value::expect_u128 ✅ Yes ✅ Yes
Value::expect_optional ✅ Yes ✅ Yes
Value::expect_principal ✅ Yes ✅ Yes
Value::expect_callable ✅ Yes ✅ Yes
ASCIIData::to_value ✅ Yes ✅ Yes
BuffData::len ✅ Yes ✅ Yes
BuffData::append ❌ No ❌ (it cannot fail)
BuffData::to_value ❌ No ❌ (it cannot fail)
ListData::append ❌ No ❌ (it cannot fail)
ListData::to_value ❌ No ❌ (it cannot fail)
TupleData::new ❌ No ❌ (it cannot fail)
TupleData::from_data ❌ No
TupleData::get_owned ❌ No
TupleData::shallow_merge ❌ No ❌ (it cannot fail)
Utf8Data::to_value ✅ Yes ✅ Yes
Value::try_deserialize_bytes_exact ❌ No
Value::serialize_to_vec ✅ Yes ✅ Yes
Value::serialize_to_hex ✅ Yes ✅ Yes
TypeSignature::type_of ❌ No
TypeSignature::inner_size ❌ No

Below is a list of functions moved to clarity-serialization that returns Result, but the errors are not consensus-critical. Some because they are not directly used in consensus-critical code, some because they are used, but their errors are directly mapped to other types without even checking their current value.

Function Return type covered by Tests? Returns InterpreterError?
QualifiedContractIdentifier::local ✅ Yes ❌ No
QualifiedContractIdentifier::parse ✅ Yes ❌ No
TraitIdentifier::parse_fully_qualified ✅ Yes ❌ No
TraitIdentifier::parse_sugared_syntax ✅ Yes ❌ No
TraitIdentifier::parse ✅ Yes ❌ No
Value::list_with_type ✅ Yes (already tested by test_constructors) ✅ Yes
Value::cons_list_unsanitized ❌ No ❌ No
Value::string_utf8_from_string_utf8_literal ❌ No ✅ Yes (but unreachable)
Value::expect_ascii ✅ Yes ✅ Yes
Value::expect_i128 ✅ Yes ✅ Yes
Value::expect_buff ✅ Yes ✅ Yes
Value::expect_list ✅ Yes ✅ Yes
Value::expect_buff_padded ✅ Yes ✅ Yes
Value::expect_bool ✅ Yes ✅ Yes
Value::expect_tuple ✅ Yes ✅ Yes
Value::expect_result ✅ Yes ✅ Yes
Value::expect_result_ok ✅ Yes ✅ Yes
Value::expect_result_err ✅ Yes ✅ Yes
ListData::len ❌ No ✅ Yes (but unreachable, list should contain more than u32 items)
ASCIIData::len ❌ No ✅ Yes (but unreachable, ascii should contain more than MAX_VALUE_SIZE items)
ASCIIData::append ❌ No ❌ No (cannot fail)
UTF8Data::len ❌ No ✅ Yes (but unreachable, utf8 should contain more than MAX_VALUE_SIZE items)
UTF8Data::appen ❌ No ❌ No (cannot fail)
PrincipalData::parse_standard_principal ✅ Yes ❌ No

Applicable issues

  • fixes #

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@Jiloc Jiloc requested a review from kantai August 27, 2025 16:22
@Jiloc Jiloc self-assigned this Aug 27, 2025
@Jiloc
Copy link
Contributor Author

Jiloc commented Aug 27, 2025

I had to remove the returned error .into() in 7760293 because of clippy, but in reality any changes to the return type would be consensus breaking only if they are not converted to the expected types! in the clarity-serialization branch I'll revert this change

@Jiloc Jiloc added this to the 3.2.0.0.2 milestone Aug 27, 2025
@Jiloc Jiloc added the testing label Aug 27, 2025
Copy link
Contributor

@kantai kantai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me!

@Jiloc Jiloc marked this pull request as ready for review August 29, 2025 15:40
@Jiloc Jiloc requested review from a team as code owners August 29, 2025 15:40
@aldur aldur requested review from rdeioris and fdefelici and removed request for rdeioris September 1, 2025 15:10
Copy link
Contributor

@fdefelici fdefelici left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT!

@github-project-automation github-project-automation bot moved this to Status: 💻 In Progress in Stacks Core Eng Sep 2, 2025
@Jiloc Jiloc moved this from Status: 💻 In Progress to Status: In Review in Stacks Core Eng Sep 2, 2025
Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic!

@github-project-automation github-project-automation bot moved this from Status: In Review to Status: 💻 In Progress in Stacks Core Eng Sep 3, 2025
@Jiloc Jiloc added this pull request to the merge queue Sep 3, 2025
Merged via the queue into stacks-network:develop with commit 5656fdd Sep 3, 2025
285 of 289 checks passed
@Jiloc Jiloc deleted the chore/test-clarity-serialization-interpreter-errors branch September 3, 2025 11:22
@github-project-automation github-project-automation bot moved this from Status: 💻 In Progress to Status: ✅ Done in Stacks Core Eng Sep 3, 2025
Copy link

codecov bot commented Sep 3, 2025

Codecov Report

❌ Patch coverage is 30.43478% with 112 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.73%. Comparing base (d22bad7) to head (7760293).
⚠️ Report is 81 commits behind head on develop.

Files with missing lines Patch % Lines
clarity/src/vm/types/mod.rs 23.48% 101 Missing ⚠️
clarity/src/vm/types/serialization.rs 50.00% 11 Missing ⚠️

❌ Your project status has failed because the head coverage (49.73%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

❗ There is a different number of reports uploaded between BASE (d22bad7) and HEAD (7760293). Click for more details.

HEAD has 12 uploads less than BASE
Flag BASE (d22bad7) HEAD (7760293)
131 119
Additional details and impacted files
@@             Coverage Diff              @@
##           develop    #6427       +/-   ##
============================================
- Coverage    71.24%   49.73%   -21.51%     
============================================
  Files          556      556               
  Lines       351138   351299      +161     
============================================
- Hits        250171   174726    -75445     
- Misses      100967   176573    +75606     
Files with missing lines Coverage Δ
clarity/src/vm/types/signatures.rs 60.09% <100.00%> (-30.25%) ⬇️
clarity/src/vm/types/serialization.rs 75.80% <50.00%> (-12.08%) ⬇️
clarity/src/vm/types/mod.rs 68.61% <23.48%> (-16.32%) ⬇️

... and 437 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 d22bad7...7760293. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants