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

Implement OnExecHook VM API #3460

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Conversation

Furetur
Copy link
Contributor

@Furetur Furetur commented May 23, 2024

Problem

This MR implements the proposed Hooks API from #3415 that can be used for coverage collection, tracing, breakpoints, and etc

Solution

This is a very small change. The main points are as follows:

  1. A new field is added to the VM
+	// All registered hooks.
+	// Each hook should never be nil.
+	hooks hooks
  1. hooks is a structure meant to store all of the VM hooks:
// OnExecHook is a type for a callback that is invoked
// for each executed instruction
type OnExecHook = func(scriptHash util.Uint160, offset int, opcode opcode.Opcode)

// A struct that contains all VM hooks
type hooks struct {
	onExec OnExecHook
}
  1. The default value of the OnExec hooks is a NOP function
  2. A VM.SetOnExecHook method is added to register hooks
// SetOnExecHook sets the value of OnExecHook which
// will be invoked for each executed instruction.
// This function panics if the VM has been started.
func (v *VM) SetOnExecHook(hook OnExecHook) {
	if v.state != vmstate.None {
		panic("Cannot set onExec hook of a started VM")
	}
	v.hooks.onExec = hook
}

@Furetur Furetur changed the title vm: implement OnExecHook Implement OnExecHook May 23, 2024
pkg/vm/vm.go Outdated Show resolved Hide resolved
pkg/vm/vm.go Outdated Show resolved Hide resolved
pkg/vm/vm.go Outdated Show resolved Hide resolved
pkg/vm/vm.go Outdated Show resolved Hide resolved
pkg/vm/vm.go Outdated Show resolved Hide resolved
@AnnaShaleva AnnaShaleva changed the title Implement OnExecHook Implement OnExecHook VM API Jun 5, 2024
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.10%. Comparing base (2280523) to head (a7883c7).
Report is 43 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3460      +/-   ##
==========================================
- Coverage   86.13%   86.10%   -0.04%     
==========================================
  Files         331      331              
  Lines       38470    38564      +94     
==========================================
+ Hits        33138    33207      +69     
- Misses       3804     3827      +23     
- Partials     1528     1530       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AnnaShaleva
Copy link
Member

@Furetur, any update on this? See also the related comments (#3462 (comment), #3462 (comment)).

@AnnaShaleva
Copy link
Member

Also please, pay attention to the PR workflows. Linter job is failing and commit sign-off is required for every commit.

@Furetur
Copy link
Contributor Author

Furetur commented Jun 17, 2024

Hello, @AnnaShaleva!

Thanks for your comments, especially the ones about workflows.

I am currently overloaded with work but I'll do my best to address your comments over the next few days.

@Furetur
Copy link
Contributor Author

Furetur commented Jun 21, 2024

@AnnaShaleva could you please approve the workflow?

@Furetur Furetur force-pushed the add-onexec-hook branch 2 times, most recently from afd3ac3 to 243ebc4 Compare June 22, 2024 13:57
@Furetur Furetur requested a review from AnnaShaleva June 22, 2024 13:58
@Furetur
Copy link
Contributor Author

Furetur commented Jun 22, 2024

Fixed the 'lint' job (locally). @AnnaShaleva could you please rerun the workflows?

Also, previously the 'coverage' job failed because some token was null. I believe that it was not because of my code

@AnnaShaleva
Copy link
Member

Also, previously the 'coverage' job failed because some token was null.

It's just the problem with our Coverage job itself, it doesn't work with outer contributions. So you're right, it's not your fault.

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

pkg/vm/vm.go Outdated Show resolved Hide resolved
@roman-khimov
Copy link
Member

It's just the problem with our Coverage job itself

It's a feature!

pkg/vm/vm.go Outdated Show resolved Hide resolved
@Furetur Furetur force-pushed the add-onexec-hook branch 2 times, most recently from 87f9317 to a192f77 Compare June 24, 2024 13:05
@Furetur
Copy link
Contributor Author

Furetur commented Jun 24, 2024

Seems like I've fixed all the comments, or are there any that I've missed?

@AnnaShaleva
Copy link
Member

Also please note that unit-tests are failing:

2024-06-24T16:05:21.9161891Z === RUN   TestCannotSetOnExecHookOfStartedVm
2024-06-24T16:05:21.9162035Z     vm_test.go:135: 
2024-06-24T16:05:21.9162483Z         	Error Trace:	/home/runner/work/neo-go/neo-go/pkg/vm/vm_test.go:135
2024-06-24T16:05:21.9163197Z         	            				/home/runner/work/neo-go/neo-go/pkg/vm/vm_test.go:2782
2024-06-24T16:05:21.9163485Z         	Error:      	Received unexpected error:
2024-06-24T16:05:21.9163733Z         	            	no program loaded

@AnnaShaleva
Copy link
Member

AnnaShaleva commented Jul 1, 2024

@Furetur, could you please update the #3460 (comment) and fix failing tests? This PR is ready to go otherwise, and then we'll continue with #3462.

@AnnaShaleva
Copy link
Member

Also, please, add "Close #3415" entry to the PR/commit message for proper GH linking.

@Furetur Furetur force-pushed the add-onexec-hook branch 2 times, most recently from 543e2db to 122cd59 Compare July 2, 2024 15:10
@Furetur
Copy link
Contributor Author

Furetur commented Jul 2, 2024

@AnnaShaleva

could you please update the #3460 (comment) and fix failing tests?

Done

@Furetur
Copy link
Contributor Author

Furetur commented Jul 2, 2024

Sorry for the failing unit tests. I've probably "refactored" them a little bit before committing and forgot to rerun them

@Furetur
Copy link
Contributor Author

Furetur commented Jul 2, 2024

Previously the CodeCov job failed because my commit decreased the overall coverage of the entire project. I don't think that my change could have caused those changes

But I've added a test to mptdata_test.go that covers uncovered lines of code

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

pkg/network/payload/mptdata_test.go Outdated Show resolved Hide resolved
pkg/vm/vm.go Outdated Show resolved Hide resolved
Refs nspcc-dev#3415

This commit introduces a small new change
that implements the Hooks API and more
specifically the OnExecHook. This feature
can be used to implement test coverage
collection, tracing, breakpoints, and etc.

To be more specific, this commit:

1. adds a new `hooks` field to the `VM`
   (this field contains the OnExecHook
    function)

2. sets the default value of this hook
   to be a NOP function

3. adds the `VM.SetOnExecHook` method

Signed-off-by: Furetur <[email protected]>
@AnnaShaleva AnnaShaleva merged commit 6f77195 into nspcc-dev:master Jul 3, 2024
18 of 19 checks passed
@Furetur
Copy link
Contributor Author

Furetur commented Jul 3, 2024

Extracted the additional test into a separate PR
#3501

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