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

Various verification fixes 2 #417

Merged
merged 12 commits into from
Oct 4, 2019
Merged

Conversation

roman-khimov
Copy link
Member

This focuses on VM improvements necessary to make tx verification work. It's mostly related to interop functionality, but doesn't add interops at the moment (I want to add more of them before the first push). Fixes #295 along the way.

@roman-khimov roman-khimov added this to the v0.51.0 milestone Oct 3, 2019
@roman-khimov roman-khimov force-pushed the various-verification-fixes2 branch 2 times, most recently from 8303426 to 97bce52 Compare October 3, 2019 14:10
@codecov
Copy link

codecov bot commented Oct 3, 2019

Codecov Report

Merging #417 into master will decrease coverage by 0.23%.
The diff coverage is 54.7%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #417      +/-   ##
==========================================
- Coverage   51.27%   51.04%   -0.24%     
==========================================
  Files          96       97       +1     
  Lines        6487     6585      +98     
==========================================
+ Hits         3326     3361      +35     
- Misses       2922     2982      +60     
- Partials      239      242       +3
Impacted Files Coverage Δ
pkg/smartcontract/param_context.go 0% <ø> (ø) ⬆️
pkg/vm/interop.go 0% <0%> (ø) ⬆️
pkg/vm/compiler/compiler.go 23.52% <0%> (+4.48%) ⬆️
pkg/vm/stack_item.go 54.03% <0%> (-5.79%) ⬇️
pkg/core/blockchain.go 27.86% <3.03%> (-1.09%) ⬇️
pkg/vm/vm.go 70.4% <44.3%> (-3.35%) ⬇️
pkg/vm/stack.go 87.65% <68.75%> (-1.89%) ⬇️
pkg/core/contract_state.go 84.78% <84.78%> (ø)
pkg/vm/context.go 77.27% <97.67%> (+1.86%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e83dc94...dca332f. Read the comment docs.

pkg/core/blockchain.go Outdated Show resolved Hide resolved
pkg/vm/stack_item.go Outdated Show resolved Hide resolved
pkg/vm/vm.go Outdated Show resolved Hide resolved
pkg/core/blockchain.go Outdated Show resolved Hide resolved
pkg/core/contract_state.go Outdated Show resolved Hide resolved
pkg/core/contract_state.go Outdated Show resolved Hide resolved
pkg/vm/vm.go Show resolved Hide resolved
Copy link
Contributor

@volekerb volekerb left a comment

Choose a reason for hiding this comment

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

LGTM only some nit questions/comments

Fixes script invocations via the APPCALL instruction. Adjust contract state
field types accordingly.
The same way C# node does.
This is an opaque data item that is to be used by the interop functions.
This is both for #373 and for interop functions that have to check some
inputs.
This function is intended to be ran outside of the execute's panic recovery
mechanism, so it shouldn't panic if there is no result.
Switch cases are evaluated sequentially and the fault case is top-priority to
handle.
It gives access to the internal value's Value() which is essential for interop
functions that need to get something from InteropItems. And it also simplifies
some already existing code along the way.
Interop services routinely push such things (block index, blockchain height)
onto the stack.
Script can return non-bool results that can still be converted to bool
according to the usual VM rules. Unfortunately Bool() panics if this
conversion fails which is OK for things done in vm.execute(), but certainly
not for VerifyWitnesses(), thus there is a need for TryBool() that will just
return an error in this case.
Make Context.Next() return both opcode and instruction parameter if any. This
simplifies some code and needed to deal with #295.
Fix #295, deduplicate code and add `inspect` parameter to the vm command to
dump AVMs (`contract inspect` works with Go code).
@roman-khimov roman-khimov merged commit aab2f9a into master Oct 4, 2019
@roman-khimov roman-khimov deleted the various-verification-fixes2 branch October 4, 2019 13:17
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.

Proper instruction handling in CompileAndInspect()
2 participants