Skip to content

Conversation

@gerau
Copy link

@gerau gerau commented Dec 6, 2025

Add prune_with_tracker method to node::redeem::RedeemNode to allow passing custom execution trackers during node pruning. The existing prune method is now a wrapper around this new method; it passes SetTracker to preserve the original behavior.

Also modify the bit_machine::ExecTracker trait to include two new methods: contains_left and contains_right. These check if the left or right branch of a case node contains a given Ihr, allowing us to replicate the SetTracker logic inside old prune method.

Removed bit_machine::BitMachine::exec_prune, because after the change, it appears that this function is no longer used anywhere. Since it is visible only within the crate, I assume it is safe to remove.

Add `prune_with_tracker` method to `node::redeem::RedeemNode` to allow
passing custom execution trackers during node pruning. The existing
`prune` method is now a wrapper around this new method; it passes
`SetTracker` to preserve the original behavior.

Also modify the `bit_machine::ExecTracker` trait to include two new
methods: `contains_left` and `contains_right`. These check if the
left or right branch of a case node contains a given `Ihr`, allowing
us to replicate the `SetTracker` logic inside old `prune` method.

Removed `bit_machine::BitMachine::exec_prune`, because after the change, it appears that this function is no longer used
anywhere.
Since it is visible only within the crate, I assume it is safe to
remove.
@gerau gerau marked this pull request as draft December 6, 2025 10:51
@KyrylR
Copy link
Contributor

KyrylR commented Dec 6, 2025

By adding the ability to pass a tracker here in prune_with_tracker, you also delegate responsibility to the user to know the library’s internals; in particular, the user is now responsible for making these work:

    /// Check if left branch of the case node contains given `ihr`.
    fn contains_left(&self, ihr: &Ihr) -> bool;

    /// Check if right branch of the case node contains given `ihr`.
    fn contains_right(&self, ihr: &Ihr) -> bool;

    /// Track the execution of the left branch of the case node with the given `ihr`.
    fn track_left(&mut self, ihr: Ihr);

    /// Track the execution of the right branch of the case node with the given `ihr`.
    fn track_right(&mut self, ihr: Ihr);

Which, in my opinion, is not the right way to go.

By looking more closely at the prune function, I see why the tracker was not passed before, though that does not mean we should sacrifice developer experience.

Instead, I would propose adding more capabilities to the SetTracker.

You can add optional Sink functionality and have a function like prune_with_debug, where you would pass this new structure.

Here is a rough idea of how sink logic can be handled:

fn default_debug_sink(label: &str, value: &dyn core::fmt::Display) {
    println!("DBG: {label} = {value}");
}

fn default_jet_trace_sink(jet: Elements, args: &[Value], result: &Value) {
    print!("{jet:?}(");
    for (i, a) in args.iter().enumerate() {
        if i > 0 {
            print!(", ");
        }
        print!("{a}");
    }
    println!(") = {result}");
}

    /// Create a new tracker bound to the provided debug symbol table.
    #[must_use]
    pub fn new(debug_symbols: &'a DebugSymbols) -> Self {
        Self {
            debug_symbols,
            debug_sink: None,
            jet_trace_sink: None,
        }
    }

    /// Enable forwarding of debug!() calls to the provided sink.
    #[must_use]
    pub fn with_debug_sink<F>(mut self, sink: F) -> Self
    where
        F: FnMut(&str, &dyn core::fmt::Display) + 'a,
    {
        self.debug_sink = Some(Box::new(sink));
        Self { ..self }
    }

    /// Enable the default debug!() sink that prints to stdout.
    #[must_use]
    pub fn with_default_debug_sink(self) -> Self {
        self.with_debug_sink(default_debug_sink)
    }

    /// Enable forwarding of jet call traces to the provided sink.
    #[must_use]
    pub fn with_jet_trace_sink<F>(mut self, sink: F) -> Self
    where
        F: FnMut(Elements, &[Value], &Value) + 'a,
    {
        self.jet_trace_sink = Some(Box::new(sink));
        Self { ..self }
    }

    /// Enable the default jet trace sink that prints to stdout.
    #[must_use]
    pub fn with_default_jet_trace_sink(self) -> Self {
        self.with_jet_trace_sink(default_jet_trace_sink)
    }
    
    // ... 

    fn track_jet_call(
        &mut self,
        jet: &Elements,
        input_buffer: &[UWORD],
        output_buffer: &[UWORD],
        _: bool,
    ) {
        if let Some(sink) = self.jet_trace_sink.as_mut()
            && let (Ok(args), Ok(result)) = (
                parse_args(*jet, input_buffer),
                parse_result(*jet, output_buffer),
            )
        {
            sink(*jet, &args, &result);
        }
    }

    fn track_dbg_call(&mut self, cmr: &Cmr, value: simplicityhl::simplicity::Value) {
        if let Some(sink) = self.debug_sink.as_mut()
            && let Some(tracked_call) = self.debug_symbols.get(cmr)
            && let Some(Either::Right(debug_value)) =
                tracked_call.map_value(&StructuralValue::from(value))
        {
            sink(debug_value.text(), &debug_value.value());
        }
    }

    fn is_track_debug_enabled(&self) -> bool {
        self.debug_sink.is_some()
    }

So if the user wants, they will be able to construct a structure for getting logs.

We should also make enough effort to write good Rust docs for the new function to make it as easy as possible for the user to see the logs of prune execution.

@apoelstra
Copy link
Collaborator

This PR does multiple things:

  • inlines the exec_prune method which I agree is not really necessary and misleadingly name
  • expands the ExecTracker trait with extra methods that allow arbitrary trackers to guide pruning
  • provides a prune_with_tracker method

Can you split these into multiple commits?

By adding the ability to pass a tracker here in prune_with_tracker, you also delegate responsibility to the user to know the library’s internals;

We already expose exec_with_tracker, which is very useful (I use it inhttps://github.com/BlockstreamResearch/hal-simplicity/pull/32 for example). I agree with both of you that it should be expanded and that it "requires the user to understand the library internals".

I think we should back up and come up with a principled API for ExecTracker, explicitly document the conditions under which all the methods are called, and then revisit this. I will open an issue.

@apoelstra apoelstra mentioned this pull request Dec 6, 2025
@apoelstra
Copy link
Collaborator

I propose we do this #324

@apoelstra
Copy link
Collaborator

See #325 for an alternate approach.

@gerau
Copy link
Author

gerau commented Dec 9, 2025

Closing this in favor of #325

@gerau gerau closed this Dec 9, 2025
apoelstra added a commit that referenced this pull request Dec 10, 2025
…ion trait

3bf7cef bit_machine: add example StderrTracker which outputs stuff to stderr (Andrew Poelstra)
6013bd5 bit_machine: replace `ExecTracker` API with a much more general one (Andrew Poelstra)
4134976 bit_machine: add PruneTracker trait and RedeemNode::prune_with_tracker (Andrew Poelstra)
ca4e497 bit_machine: move tracker stuff to its own module (Andrew Poelstra)
eaa02d0 bit_machine: rename private new/move/drop methods and variants (Andrew Poelstra)
228a85f bit_machine: put pub(super) on all public methods of Frame (Andrew Poelstra)
8622a8f bit_machine: return concrete type from `as_bit_iter` (Andrew Poelstra)
7ecf394 BitIter: add ExactSizeIterator bound, size_hint and tests (Andrew Poelstra)

Pull request description:

  Adds the ability for the execution tracker to track every visited node, to view the "input" (top read frame) of every node and the "output" (top write frame after execution) of every terminal node. Allows it to read this data in the form of a bit iterator, which is much easier to work with than the `&[UWORD]` that the old jet interface provided. Allows tracking "debug" nodes without special-purpose methods that enable/disable costly conversions.
  
  Also adds a `PruneTracker` extension trait which can be used to introspect or manipulate the pruning process.
  
  See #323 and #324 for motivation.
  
  Fixes #324.


ACKs for top commit:
  KyrylR:
    ACK 3bf7cef


Tree-SHA512: 81969779ca6f45d0ae3556475f10de6e661a8169c65409e15d697277e6806689b104644f5e5f25ce22d806f51fbfd892ebd0649ae8d24f2cd58bd551f4b476e7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants