[stringhandler]

So after a few different approaches to fixing this bug, I settled on this one for now. Basically, run the program on a Bitmachine with a tracker which stores which IHRs were called. Then go top down along the DAG and store kept IHRs, and for cases, mark their children according to how we will hide them.

Then, rebuild the tree from the kept nodes, using the hiding information to replace case nodes, if necessary. Finally, run finalize on the new tree to determine the correct types.

There is an extra step added in the second last commit, which runs the code again like consensus would, and fails if there are any un-run nodes. This can be removed if deemed unnecessary.

Also, because pruning is done by IHR, a step is added to the beginning to remove that share an IHR but are different in memory (reduces to Max Sharing instead of Internal Sharing). For decoded programs, this is always the case anyway, but for programs edited in memory, this may not be a valid assumption.

[stringhandler]

I made a few other attempts at this, and created some good fuzzing programs for pruning, which we don't really have. I'll add them in a separate PR

[apoelstra]

In 43bde49:

This is a neat idiom. It pushes node.left_child() if it is Some.

[apoelstra]

In 23ae4bf:

This approach (just fail) is certainly better than producing a broken program. But we should revisit and try to make pruning work unconditionally.

[stringhandler]

This approach (just fail) is certainly better than producing a broken program. But we should revisit and try to make pruning work unconditionally.

Agree

[apoelstra]

ACK 21878a5; successfully ran local tests

[ikripaka]

Guys, would you mind adding #[non_exhaustive] property to the ExecutionError?
When I tried to use this commit as a patch, Simplex failed to compile.
In this case, I assume new errors can be added later without breaking users' code, or is the current approach better?

[stringhandler]

I don't mind but could you provide the actual error?

[ikripaka]

error[E0004]: non-exhaustive patterns: `&ExecutionError::PruningFailed` not covered
   --> crates/test/src/fuzz/outcome.rs:57:92
    |
 57 |             ProgramError::Pruning(execution) | ProgramError::Execution(execution) => match execution {
    |                                                                                            ^^^^^^^^^ pattern `&ExecutionError::PruningFailed` not covered

<shorten it a little bit>

550 | pub enum ExecutionError {
    | ^^^^^^^^^^^^^^^^^^^^^^^
...
564 |     PruningFailed,
    |     ------------- not covered
    = note: the matched value is of type `&ExecutionError`

[ikripaka]

This error isn't complex; it's easy to fix, but this library was published on Friday.
In the non_exhaustive case, we can use a wildcard pattern to handle it gracefully from our side; any additional errors would be handled like that.

[apoelstra]

Yeah, agreed we should add #[non_exhaustive] to this error (and probably all our error enums). But can wait for a followup.

[stringhandler]

In the case that you're using the error, I would say that it is better to not add the attribute. It looks like you are fuzz testing something and genuinely need to manually update the case. (I'm guessing, since the path says crates/test/src/fuzz/outcome.rs:57:92)

[apoelstra]

Interesting point.

In rust-bitcoin we have started adding the attribute to almost all our error enums so that post-1.0 we will still have room to extend the errors without semver-breaking changes.

But maybe because this crate is so young and nowhere near 1.0, we want to take the opposite tack, and try to alert people via compiler breakage when we've extended error cases.

I'm unsure.

[stringhandler]

I don't mind adding it at some point, but I think it may be too early for ExecutionError in particular. If there are new ways an execution can fail, it would most likely be better for wallets and downstream to handle it explicitly, and know about it.

[apoelstra]

Okay, agreed.