Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

txscript/engine: add execution StepCallback #1980

Merged
merged 2 commits into from
Aug 15, 2023

Conversation

halseth
Copy link
Collaborator

@halseth halseth commented May 10, 2023

We add a new StepCallback as optional function closure on the Engine that will be called every time a step has been performed during script execution.

This is only meant to be used in debugging.

More specifically, this is used for Tapsim: https://github.com/halseth/tapsim

@coveralls
Copy link

coveralls commented May 10, 2023

Pull Request Test Coverage Report for Build 5785035301

  • 46 of 52 (88.46%) changed or added relevant lines in 1 file are covered.
  • 11 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.01%) to 55.321%

Changes Missing Coverage Covered Lines Changed/Added Lines %
txscript/engine.go 46 52 88.46%
Files with Coverage Reduction New Missed Lines %
connmgr/connmanager.go 1 86.97%
peer/peer.go 10 73.2%
Totals Coverage Status
Change from base Build 5747300520: 0.01%
Covered Lines: 26775
Relevant Lines: 48399

💛 - Coveralls

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Super useful! I do something like this at times locally to be able to print out debug information to introspect into a stack when tests fail.

txscript/engine.go Outdated Show resolved Hide resolved
txscript/engine.go Show resolved Hide resolved
@halseth
Copy link
Collaborator Author

halseth commented Jun 1, 2023

Rebased and addressed comments. PTAL @Roasbeef @kcalvinalvin

Copy link
Collaborator

@kcalvinalvin kcalvinalvin left a comment

Choose a reason for hiding this comment

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

ACK 3033ba3

@halseth
Copy link
Collaborator Author

halseth commented Jun 26, 2023

Rebased. PTAL @Roasbeef

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Looking pretty good, left some final comments. I think we should also add a basic test for txscript.NewDebugEngine as protection against regressions here. Can be a simple P2PKH spend or something like that.

txscript/engine.go Outdated Show resolved Hide resolved
txscript/engine.go Outdated Show resolved Hide resolved
@halseth
Copy link
Collaborator Author

halseth commented Aug 1, 2023

@Roasbeef Added a test, good idea 👍 552cb80

PTAL

@halseth halseth requested a review from Roasbeef August 1, 2023 12:57
We add a new stepCallback as optional function closure on the Engine
that will be called every time a step has been performed during script
execution.

It is accessed by calling the NewDebugEngine constructor.

This is only meant to be used in debugging.
Add a simple test that make sure the stack and alt stack is correctly
set in the callback from script execution.
@halseth
Copy link
Collaborator Author

halseth commented Aug 7, 2023

Rabased.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🎿


// TestDebugEngine checks that the StepCallbck called during debug script
// execution contains the expected data.
func TestDebugEngine(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@Roasbeef Roasbeef merged commit 201b608 into btcsuite:master Aug 15, 2023
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.

4 participants