Skip to content

Conversation

fdefelici
Copy link
Contributor

@fdefelici fdefelici commented Oct 10, 2025

Description

This PR does 2 things:

  • add StacksTransactionReceipt::vm_error to the ExpectedTransactionOutput
  • try to override ExpectedTransactionOutput::vm_error for the snapshot appending a NON-CONSENSUS BREAKING info message. Basically we would like something like this:
       ExpectedTransactionOutput(
          ...,
          vm_error: Some("whatever"),   //NON-CONSENSUS BREAKING
       ),   
    

With the current solution, based on serde, we can only achieve this:

     ExpectedTransactionOutput(
        ...,
        vm_error: "whatever  //NON-CONSENSUS BREAKING",
     ),   

Basically because they are json fields we are not allowed to add text outside the field.

Here I used a "static" approach so applying the trasformation directly to the ExpectedTransactionOutput definition, but we will have same result using the "dynamic" reduction approach done inside the test execution (because rely on serde/json).

Alternative Approach
I also find a way to do what we want but requires to implement the Debug trait doing all the snaphoshot formatting "manually". Here a short example:

#[derive(Serialize)]
struct MyStruct {
    id: u32,
    name: Option<String>,
}

impl fmt::Debug for MyStruct {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        writeln!(f, "MyStruct(")?;
        writeln!(f, "    id: {},", self.id)?;
        match &self.name {
            Some(n) => writeln!(f, "    name: Some({:?}), // REVIEWED", n)?,
            None => writeln!(f, "    name: None, // REVIEWED")?,
        }
        write!(f, ")")
    }
}

#[test]
fn test_debug() {
    let data_some = MyStruct {
        id: 20,
        name: Some("Alice".to_string()),
    };

    insta::assert_debug_snapshot!(data_some);
}

This will produce the following snapshot:

1 │+MyStruct(
2 │+    id: 20,
3 │+    name: Some("Alice"), // REVIEWED
4 │+)

So, for sure this is nice and give us the best flexibility (also for future formatting requirements), but we have to take into account for the whole struct formatting by ourselves.

What do you think?

Applicable issues

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

@fdefelici fdefelici self-assigned this Oct 10, 2025
@fdefelici fdefelici added the aac Avoiding Accidental Consensus label Oct 10, 2025
@fdefelici fdefelici moved this to Status: 💻 In Progress in Stacks Core Eng Oct 10, 2025
@fdefelici fdefelici requested review from Jiloc and jferrant October 10, 2025 10:06
Copy link
Contributor

@Jiloc Jiloc left a comment

Choose a reason for hiding this comment

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

I am ok with the serde approach, the message is clear, i am not concerned with the fact that it's included in the string itself. (note that there is a rust fmt error)

@jferrant
Copy link
Contributor

Hm I have mixed feelings. its annoying to custom write the entire debug struct, but I think the fields are pretty stable so once its written, it won't be so bad and I think its better to be explicit and then we don't actually modify the original struct. So I think I actually prefer the alternative approach if you don't mind XD but its rare that I have strong opinions and even when I do...they are loosely held so if you choose to ignore me, I will approve regardless.

@fdefelici
Copy link
Contributor Author

fdefelici commented Oct 13, 2025

I gave a try to the Debug based solution. I found this caveat:

Example with DEBUG

         88 │+    Success(
         89 │+        ExpectedBlockOutput {
         90 │+            marf_hash: 66eed8c0ab31db111a5adcc83d38a7004c6e464e3b9fb9f52ec589bc6d5f2d32,
         91 │+            transactions: [
         92 │+                ExpectedTransactionOutput {
         93 │+                  return_type: Response(ResponseData { committed: true, data: Bool(true) }),
         94 │+                  cost: ExecutionCost { write_length: 0, write_count: 0, read_length: 0, read_count: 0, runtime: 0 },
         95 │+                  vm_error: None, // NON-CONSENSUS BREAKING
         96 │+                },

Example with RON (serde)

  163       │-  Success(ExpectedBlockOutput(
  164       │-    marf_hash: "66eed8c0ab31db111a5adcc83d38a7004c6e464e3b9fb9f52ec589bc6d5f2d32",
  165       │-    transactions: [
  166       │-      ExpectedTransactionOutput(
  167       │-        return_type: Response(ResponseData(
  168       │-          committed: true,
  169       │-          data: Bool(true),
  170       │-        )),
  171       │-        cost: ExecutionCost(
  172       │-          write_length: 0,
  173       │-          write_count: 0,
  174       │-          read_length: 0,
  175       │-          read_count: 0,
  176       │-          runtime: 0,
  177       │-        ),
  178       │-        vm_error: None,
  179       │-      ),

The main issue is that with RON with have automatic field aligment (split per line, which should be good for better diffs), while with DEBUG we haven't (or better we should define custom Debug trait for each struct/enum involved).

Maybe for now, better to stick with RON/serde integration (even if I also liked the debug solution, being nicest for the consenus message)

@fdefelici fdefelici moved this from Status: 💻 In Progress to Status: In Review in Stacks Core Eng Oct 13, 2025
@fdefelici fdefelici marked this pull request as ready for review October 13, 2025 12:41
@fdefelici fdefelici requested review from a team as code owners October 13, 2025 12:41
Copy link
Contributor

@Jiloc Jiloc left a comment

Choose a reason for hiding this comment

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

LGTM! the only remark I can make, is that we could decide to skip serialization of None values, but I am fine as-is!

@fdefelici fdefelici requested a review from kantai October 13, 2025 14:25
@github-project-automation github-project-automation bot moved this from Status: In Review to Status: 💻 In Progress in Stacks Core Eng Oct 14, 2025
@fdefelici fdefelici added this pull request to the merge queue Oct 14, 2025
Merged via the queue into stacks-network:develop with commit dfc17c4 Oct 14, 2025
300 of 303 checks passed
@fdefelici fdefelici deleted the test/aac-vm-error branch October 14, 2025 14:26
@github-project-automation github-project-automation bot moved this from Status: 💻 In Progress to Status: ✅ Done in Stacks Core Eng Oct 14, 2025
Copy link

codecov bot commented Oct 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.86%. Comparing base (a7f4240) to head (6fc8788).
⚠️ Report is 51 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6587      +/-   ##
===========================================
+ Coverage    71.12%   79.86%   +8.74%     
===========================================
  Files          568      568              
  Lines       347674   347693      +19     
===========================================
+ Hits        247267   277670   +30403     
+ Misses      100407    70023   -30384     
Files with missing lines Coverage Δ
stackslib/src/chainstate/stacks/events.rs 96.55% <ø> (ø)
stackslib/src/chainstate/tests/consensus.rs 98.60% <100.00%> (+10.88%) ⬆️

... and 295 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 a7f4240...6fc8788. 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

aac Avoiding Accidental Consensus

Projects

Status: Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

AAC: Add tx_result and success or failure to the expected output in the test harness

3 participants