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

vm, compiler: support ASSERTMSG and ABORTMSG #3066

Merged
merged 6 commits into from
Aug 10, 2023
Merged

vm, compiler: support ASSERTMSG and ABORTMSG #3066

merged 6 commits into from
Aug 10, 2023

Conversation

AnnaShaleva
Copy link
Member

@AnnaShaleva AnnaShaleva commented Jul 20, 2023

Port neo-project/neo-vm#491, ready for review.

TODO:

  • Wait for the neo-project core update to set the opcodes price.
  • Update interop deps after PR is settle.

@AnnaShaleva AnnaShaleva added the vm VM tasks/bugs/issues label Jul 20, 2023
@AnnaShaleva AnnaShaleva added this to the v0.102.0 milestone Jul 20, 2023
@AnnaShaleva AnnaShaleva requested a review from roman-khimov July 20, 2023 08:54
@AnnaShaleva AnnaShaleva force-pushed the abort-msg branch 2 times, most recently from 24949e5 to 99e81b4 Compare August 9, 2023 15:41
@AnnaShaleva AnnaShaleva marked this pull request as ready for review August 9, 2023 15:41
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

I don't see an update to pkg/vm/testdata/neo-vm, but it should be made since neo-project/neo-vm#491 has introduced some new tests.

@AnnaShaleva
Copy link
Member Author

I don't see an update to pkg/vm/testdata/neo-vm

Shouldn't the update be done locally? Each time I'd like to update submodules I perform

git submodule update --init

or just

git submodule update

and new commits are being fetched.

Our testing job perform the same thing:

      - name: Sync VM submodule
        run: |
          git submodule sync
          git submodule update --init

Where should I add an update?

And I've fixed the JSON testdata path, it was wrong since neo-project/neo-vm@5a11f4b.

@roman-khimov
Copy link
Member

You need to go down into pkg/vm/testdata/neo-vm, do a regular git remote update && git rebase origin/master and then you'll see a change in the index. Commit it and only then others will see this update on git submodule update.

@AnnaShaleva
Copy link
Member Author

AnnaShaleva commented Aug 10, 2023

git rebase origin/master

I performed it in the clean repo, and I don't have anything to commit after all these steps:

anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go$ git checkout abort-msg 
Switched to branch 'abort-msg'
Your branch is up to date with 'origin/abort-msg'.
anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go$ cd pkg/vm/testdata/neo-vm/
anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go/pkg/vm/testdata/neo-vm$ git remote update
Fetching origin
anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go/pkg/vm/testdata/neo-vm$ git rebase origin/master
HEAD is up to date.
anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go/pkg/vm/testdata/neo-vm$ git status
HEAD detached at 7e59968
nothing to commit, working tree clean
anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go/pkg/vm/testdata/neo-vm$ cdw
anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go$ git status
On branch abort-msg
Your branch is up to date with 'origin/abort-msg'.

nothing to commit, working tree clean

The 7e59968 commit is the top of the NeoVM.

@AnnaShaleva
Copy link
Member Author

AnnaShaleva commented Aug 10, 2023

And our testing job fetches the proper commit without any updates from my side:
image

@AnnaShaleva AnnaShaleva marked this pull request as draft August 10, 2023 09:37
@AnnaShaleva
Copy link
Member Author

And moreover, with the old NeoVM JSON test path our testing job is failing with the following error:

2023-08-10T09:45:07.9164810Z === RUN   TestUT
2023-08-10T09:45:07.9165000Z     json_test.go:110: 
2023-08-10T09:45:07.9165641Z         	Error Trace:	/home/runner/work/neo-go/neo-go/pkg/vm/json_test.go:110
2023-08-10T09:45:07.9165856Z         	Error:      	Not equal: 
2023-08-10T09:45:07.9166152Z         	            	expected: true
2023-08-10T09:45:07.9166442Z         	            	actual  : false
2023-08-10T09:45:07.9166619Z         	Test:       	TestUT
2023-08-10T09:45:07.9167571Z         	Messages:   	neo-vm tests should be available (check submodules)
2023-08-10T09:45:07.9167715Z --- FAIL: TestUT (0.00s)

which means that the job can't find tests by the specified path. It is expected, because the path was updated in the NeoVM. See the job report: https://github.com/nspcc-dev/neo-go/actions/runs/5819454595/job/15777873680?pr=3066.

@roman-khimov
Copy link
Member

We have and we should always have a particular commit used for pkg/vm/testdata/neo-vm. Then we update it as needed. Like in e66e822 or 3e6ce3c.

@roman-khimov
Copy link
Member

roman-khimov commented Aug 10, 2023

The problem is 0d17273 in fact, it accidentally touched the submodule and no one noticed.

So now there is nothing to update.

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #3066 (3608314) into master (7a7d0a0) will increase coverage by 0.00%.
Report is 53 commits behind head on master.
The diff coverage is 85.25%.

@@           Coverage Diff           @@
##           master    #3066   +/-   ##
=======================================
  Coverage   84.70%   84.70%           
=======================================
  Files         329      329           
  Lines       43827    43919   +92     
=======================================
+ Hits        37124    37203   +79     
- Misses       5196     5200    +4     
- Partials     1507     1516    +9     
Files Changed Coverage Δ
pkg/config/hardfork_string.go 50.00% <ø> (ø)
pkg/core/fee/opcode.go 100.00% <ø> (ø)
pkg/vm/opcode/opcode.go 100.00% <ø> (ø)
pkg/vm/opcode/opcode_string.go 100.00% <ø> (ø)
pkg/services/rpcsrv/server.go 77.91% <60.00%> (+0.14%) ⬆️
pkg/core/dao/dao.go 79.10% <73.33%> (-1.17%) ⬇️
pkg/vm/stackitem/json.go 96.41% <88.46%> (+0.15%) ⬆️
cli/server/server.go 68.91% <100.00%> (ø)
pkg/config/hardfork.go 100.00% <100.00%> (ø)
pkg/core/blockchain.go 78.57% <100.00%> (+0.20%) ⬆️
... and 5 more

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@AnnaShaleva
Copy link
Member Author

The problem is 0d17273 in fact

😱

@AnnaShaleva
Copy link
Member Author

OK, this 0d17273 has updated the submodule up to the latest 7e59968 NeoVM commit, let this record be here, and thus we don't have to update the submodule in this PR.

@AnnaShaleva AnnaShaleva marked this pull request as ready for review August 10, 2023 10:01
@roman-khimov roman-khimov merged commit 562293c into master Aug 10, 2023
@roman-khimov roman-khimov deleted the abort-msg branch August 10, 2023 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm VM tasks/bugs/issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants