Refactor EvalScript into a CScriptExecution class, so single-stepping can be done #6178

Rebased #3901, and exposed to libbitcoinconsensus. This should be sufficient for implementing a basic Script debugger outside of Bitcoin Core.

ut ACK. Very clean commits, kudos.

Do we have any specific users for this change yet? I proposed earlier that we just add a very simple callback option to EvalScript() to optionally execute a function at each step in a way guaranteed to not be able to change the EvalScript() loop state unless desired.

Given how limited the scripting system is, I just don't see there being much demand for a full-on, fully consensus accurate, debugger; if you do need a debugger for your script it's almost certainly too complex. Simply providing a mechanism to get a good instruction trace if desired should be fine and it's a lot less risky than this pull-req.

Scripts get "too complex" fast...

Well again, do we have a concrete use-case/example?

The obvious use case is a debugger, as already mentioned.

I mean, an example of a script where you'd actually need a debugger vs. just getting an execution transcript. Remember that script execution is deterministic, and must execute quickly, so you can always implement an apparent debugger by just replaying the script from the start and stopping at the right point.

I think the debugger can also be interesting for testing simple scripts. For example, one could write some unittests to make sure an individual operand/instruction affects the stack in expected ways. I think an example of the debugger being used automatically for testing purposes should be enough to justify this. It also makes libconsensus more interesting. The consensus risks of non-trivially changing consensus code will have to be compensated with testing, but it's not like https://github.com/luke-jr/bitcoin/commit/978d000c6201951b4611f7374c72de9988516c3c is hard to read or review. The rest are just enhancements to libconsensus' API, so I don't really see where the huge risks are. Maybe luke should reduce the scope of the PR to that single commit and leave the changes on libconsensus for a later PR, would you consider that less risky? Because https://github.com/luke-jr/bitcoin/commit/978d000c6201951b4611f7374c72de9988516c3c looks really safe and elegant to me.

This-cannot-possibly-be-wrong ACK https://github.com/luke-jr/bitcoin/commit/978d000c6201951b4611f7374c72de9988516c3c ut ACK https://github.com/luke-jr/bitcoin/commit/beabd6350e49c3660f45d3d5c85633f2cd21e26a

@jtimon Well, remember my simpler proposal of a callback works just fine for the unittest case - what I want to get some time to implement - as you only need an execution trace rather than the ability to dynamically modify state. Basically, at each step the callback fires and you check it against an expected execution log and/or append to one.

But anyway, I agree that I can't see how the first commit would fail... So meh, utACK on code, NACK on overall approach. Churn in the consensus critical part is dangerous and adds to cognitive load for everyone to finding and spotting problems.

Oh, and @luke-jr if you could add a few lines of comments to the CScriptExecution explaining more about the rational and design that'd be good for future maintenance, esp. ones describing what each state variable does.

Seems to risky to me to change the consensus code for this. Don't get me wrong, there are scenarios in which single-stepping is useful - but never in consensus usage, so it would be better to do it in an external utility class which just 'emulates' consensus.

As said before, maybe the scope should be reduced to the first commit at first to make it harder to complain about "risks", because that change is really simple. I would be fine with an external utility class as long as it remains useful for unittests and it doesn't reimplement consensus, but I'm not really sure about what @laanwj has in mind here.

@jtimon @laanwj Like I said above, a callback function can be used with an external method to simulate single-stepping with sufficient performance for debugging; unittests just need execution transcripts, not single-stepping.

Well, It would also be nice if the external class can be used as a debugger, even if that's not strictly necessary for unittests.

@laanwj A debugger that isn't using consensus code is just as bad/dangerous as an alternate implementation. @petertodd 's approach is somewhat less debugger-friendly, but probably covers all practical use cases.

@luke-jr You can always compare the final output of the debug executed with a consensus execution.

If non-trivial changes are needed for a particular use case, I think it's better to use a reimplementation than to risk affecting consensus code. This goes against traditional software design principles, but given the high risk, I think that is a reasonable exception. This is a very abstract statement of course; I'm not saying that the step-by-step debugger is too much of a change to integrate it - just thay we shouldn't shy away from having a reimplementation of our fear of consensus changes is blocking useful features.

+1 @laanwj

Too much risk for too little (and +speculative) value.

What is the use case for exposing this to libbitcoinconsensus?

@sipa I would like to write a GUI Script IDE/debugger.

I'm not sure we should overload libbitcoinconsensus with such functionality. Its purpose is to implement consensus-critical verification.

@sipa I'm not sure what you're saying there. A debugger's correct execution of Scripts is fairly consensus-critical. Perhaps not when it's in the debugger, but it needs to have consensus-critical behaviour to the debugger when put on the network.

@luke-jr I think he means you could expose the debugger functions within bitcoin core but not expose it in the libconsensus API. Let's imagine a future where libconsensus is a separated repo like libsecp256k1. Then if you want to use the debugger you need to use libconsensus as a sub-tree of your repository (like bitcoin core would do) or duplicate the code, but you cannot access the debugger from the API of the compiled libbitcoinconsensus binary. Am I right, @sipa?

@jtimon subtrees are bad practice and should never* be used for shared code, especially in the debugger scenario. The only reason we "need" then in Core is because the library upstreams do not guarantee consensus compatibility.

We're very far away from having core using libconsensus only through its API if we even want that. Anyway, to be clear, I was just trying to explain what I thought @sipa meant, that doesn't mean I am against exposing the debugger calls in libconsensus myself. I think that's within it's purpose of "implement consensus-critical verification", it's just step-by-step consensus verification.

Closing as too risky. Please let's not burden the consensus code with auxiliary concerns such as debugging. This belongs, IMO, in other tooling (which could eg. simulate the consensus code).

FWIW, Hashmal is a use-case for this. It's a development tool as well as an educational one. Essentially a transaction script IDE (coincidentally the very thing @luke-jr had intended to make).

It's certainly a valid use-case for single-stepping. My point is, why not implement it in the IDE instead of relying on bitcoin core's consensus code to be adapted?

@laanwj IMO there's no definitive right-or-wrong answer here, though I am in favor in merging the PR. My logic is that it would just be easier (i.e. less error-prone) for any client code to step through scripts with this, and ensure that any script debuggers using this behave in the same manner.

@laanwj Because it's important for debuggers to be consensus-correct?

If you copy this code into your IDE, it will be consensus-correct, right? (if not, it contains a bug that changes the consensus, and we'd certainly not want to merge it either...)

Necessity of doing so defeats the point of having a libbitcoinconsensus in the first place...

I agree that libconsensus becomes more useful with this. Yes, we risk changing consensus behaviour but the "risky" commit is very easy to review and we can always reduce the risks further with more testing.

There dont appear to be any unit tests. Are there any specific unit tests that could be written to help prove consensus is met. I'm thinking some specific tests added before this PR so after this PR they dont break. This PR itself probably needs some tests also.

Im seriously intrigued about what people think that could go wrong with https://github.com/luke-jr/bitcoin/commit/978d000c6201951b4611f7374c72de9988516c3c Would it help to reduce the scope of this PR to that single commit?

You can't prove consensus is met, except through static analysis techniques which are not applicable here.

Also, consensus is not necessary for the observed execution steps, only for its (boolean) outcome.

I've been thinking about making a copy of the consensus code and its dependencies (CScript for example) inside the consensus directory, allowing for stripped, simple, and non-changing implementations there. There could be an alternative, more featureful copy outside, which is used for signing, debugging, ... for whatever process that requires consensus compatibility, you would compare the result of both.

@laanwj +1 beer @changetip

Clever!

Hi @laanwj, I've delivered a tip worth 1 beer (15,369 bits/$3.50) from @petertodd to your ChangeTip pocket.

Learn about ChangeTip

@sipa consensus doesn't need to be able to observe step, just like it doesn't need a BaseSignatureChecker (and still we made that "risky" refactor). But it would be good for libconsensus to expose the stepping and would also facilitate the work for writing some tests, don't you think?

Is it really worth it to duplicate the consensus code just to avoid the "risks" in https://github.com/luke-jr/bitcoin/commit/978d000c6201951b4611f7374c72de9988516c3c (seriously, I can't see them, can someone please point out the risky parts of this commit)?

@jtimon I think stepping would be useful for a "libbitcoinscript", but not for consensus. I think consensus code should be focussed on one thing only: exact reproducibility of validity checking.

And yes, I think that duplication is useful. Right now, it is nonobvious what parts of the script code are consensus critical, which means we have to assume all of it is, making changes like signature checking, callbacks, stepping, ... harder to introduce. Such a "consensus copy" would possibly remove CSignatureChecker, and revert to the older, more straightforward implementation.

I think consensus code should be focussed on one thing only: exact reproducibility of validity checking.

Why did we add BaseSignatureChecker then ?

Right now, it is nonobvious what parts of the script code are consensus critical, which means we have to assume all of it is

This can be trivially solved by moving the consensus critical files to the consensus folder (which we will eventually have to do if we want to separate libconsensus into its own repository/subtree).

making changes like signature checking, callbacks, stepping, ... harder to introduce

We already have signature checking, don't we? Regarding callbacks and stepping we just need one of them for testing. Why not chose the later whose implementation is also extremely simple (still waiting for someone to point to something risky in https://github.com/luke-jr/bitcoin/commit/978d000c6201951b4611f7374c72de9988516c3c ) and can be exposed in libconsensus with almost no maintainance complexity costs? Anything else that you think that "we want to have but we don't want in libconsensus"?

Such a "consensus copy" would possibly remove CSignatureChecker, and revert to the older, more straightforward implementation.

So libconsensus would be less useful than it could be but we have a more powerful duplication in Bitcoin Core that is not exposed as an independent library? Why not the other way around? Why not let the powerful version be the exposed library and the simplified duplication be a Bitcoin Core-only thing? Well, I guess that at that point the duplication makes no sense at all...To be honest, the duplication still doesn't make any sense at all to me. You are failing to explain what is the purpose of the simplified duplicated version.

@petrodd Thanks! @sipa @jtimon

I think stepping would be useful for a "libbitcoinscript", but not for consensus. I think consensus code should be focussed on one thing only: exact reproducibility of validity checking.

Exactly. This is one area we certainly don't want scope creep in. Consensus should only be involved with validation. Making changes to it for anything else is too risky.

If that requires some code duplication to support auxiliary features (such as debugging), that's acceptable. This more powerful library could also be exposed to the outside world, but not necessarily as part of libbitcoin_consensus.

If Bitcoin Core is using a simplified consensus-only copy of the code, I don't we'd want external code to use the more complicated and potentially wrong code instead. The purpose of libbitcoinconsensus is to avoid needing reimplementation in order to write reasonably-sure consensus-compliant code.

Why did we add abstracted signature checking? I think it was a very useful thing to do, but I'm increasingly of the opinion that the flexibility needed for some features is in conflict with the requirement that consensus code not be touched unless consensus changes are intended. That's why I'm arguing that they should be separated entirely.

Exactly. This is one area we certainly don't want scope creep in. Consensus should only be involved with validation.

The steps are part of the consensus validation, otherwise this wouldn't be helpful for writing tests about consensus validation. It's just that it's not strictly necessary to separate or expose them, but it's clearly not comparable to other examples like signing.

Making changes to it for anything else is too risky.

Can you please tell me which part of https://github.com/luke-jr/bitcoin/commit/978d000c6201951b4611f7374c72de9988516c3c you consider more risky?

If that requires some code duplication to support auxiliary features (such as debugging), that's acceptable. This more powerful library could also be exposed to the outside world, but not necessarily as part of libbitcoin_consensus.

What if alternative implementations prefer the version with stepping and somehow that implementation is the one most widely used by the network? "The implementation is the specification". Which one of the 2? If it's the most widely used, wouldn't it make sense to remove the duplicated step-less one?

As it is currently, the consensus library is very easy to use and I was able to include it in NBitcoin in just a few minutes. This is because .NET provides a mechanism for invoking unmanaged functions (C++ functions) that just works. However, there is no easy way to use it after this PR because it is implemented as a method.

see: using a class defined in a c++ dll in c# code and, How to Marshal a C++ Class

Provably there are other libraries that could be affected too, I don't really know. So, this PR can be useful in some scenarios (debuggers, for example) but also can bring some inconvenients (nothing unsolvable) in others.

Huh? This adds only standard C APIs to libbitcoinconsensus, which is the most common standard for such things, and also was already the only APIs exported by the library.

@luke-jr I'm sorry, you're right. I didn't review it carefully, just gave it a glance.

Fwiw, a class is not really required: it could be a parameter struct to the all the methods (which would become functions). Maybe not using the class makes ghe supposedly risky commit look even less risky?