protocol/vm: remove the dependency on protocol/bc #797

bobg · 2017-03-22T21:52:56Z

This resolves a long-standing TODO item, and will also allow protocol/bc to invoke VM validation (inverting the dependency) in the upcoming "entries"-based implementation of validation.

The dependency is broken by introducing a new object, vm.Context, that contains the execution context for the virtual machine.

The entrypoints to the VM are now consolidated into the single function vm.Verify, whose sole parameter is an appropriate Context object.

The unrelated VMContexts object in the TxHashes structure is no longer needed; it has been reduced to a list of txsighash values.

Some tests in protocol/vm depend on protocol/bc and have been moved into the new vm_test package to break the dependency cycle. Future work: remove even those files' dependence on protocol/bc by avoiding construction of full transaction and block objects.

Another carved-off piece of #788

bobg · 2017-03-22T21:53:08Z

PTAL

kr · 2017-03-22T22:41:41Z

👀

kr · 2017-03-22T22:47:57Z

protocol/vm/vmcontext.go

@@ -0,0 +1,27 @@
+package vm
+
+type VMContext interface {


The name vm.VMContext stutters, it should prob be vm.Context IMO. (Yeah, I know there is also context.Context, and within this package the unqualified type name might potentially be confusing, but the external view — what it looks like in other packages — is more important. Also I think in practice that confusion won't really be a problem because this package doesn't use context.Context at all.)

Good Go interfaces don't have 19 methods. This type might work better as a struct with optional fields. Most of these methods are just accessors anyway.

// Context defines blah blah ... // Pointer and function fields are optional. If the corresponding opcode // executes on a nil field, Verify will return ErrContext. type Context struct { Version uint64 Code []byte TxVersion *uint64 CheckOutput func(index uint64, data []byte, amount uint64, assetID []byte, vmVersion uint64, code []byte) bool etc }

The name vm.VMContext stutters, it should prob be vm.Context IMO.

Ha, I had it that way first but thought better of it when I considered the potential for confusion. Your point about vm.Context is well-taken, though.

This type might work better as a struct with optional fields

I tried it and expected to hate it, but it's a definite improvement. Thanks!

kr · 2017-03-22T23:14:06Z

protocol/vm/vm_test.go

@@ -1,4 +1,4 @@
-package vm
+package vm_test


I see this is mentioned in the commit message

Some tests in protocol/vm depend on protocol/bc and have been moved into the new vm_test package to break the dependency cycle.

The ideal way to handle this would be to make the tests not depend on protocol/bc at all, and have them set up context objects directly. Do you consider that future work? Should it be mentioned in a TODO or in the commit message? Or would it be easy enough to do right here in this PR?

Basically I think using actual blocks and transactions here is more of an integration test, and we can probably exercise this package more efficiently (more coverage with less test code) if we focus on unit tests.

Yes, I consider this future work, and have updated the commit message.

…ations to a struct with a lot of optional fields.

bobg · 2017-03-23T06:11:17Z

Updated, PTAL

kr · 2017-03-23T21:56:11Z

protocol/bc/vmcontext.go

+
+	checkOutput := func(index uint64, refdatahash []byte, amount uint64, assetID []byte, vmVersion uint64, code []byte) (bool, error) {
+		if index >= uint64(len(tx.Outputs)) {
+			return false, vm.ErrBadValue


Out of scope for this PR (because it would be a spec change), but it seems strange to me that this one check should abort the whole vm while all the other checks just return false. If my program asks whether there's an output at position 5, and there are only two outputs, the answer is "no, there isn't".

So yeah, it would be great (IMO) if this function could be a simple predicate. But since it can't, it might look a little cleaner to have it return error that's one of vm.ErrBadValue or vm.NotFound or nil.

Actually, now that you mention it, the spec is silent on what to when index is out of range. (And the updated spec on the txgraph-spec branch is similarly silent but includes outdated language about "the number of outputs.")

I agree that just returning false would be preferable. @danrobinson @oleganza would this be acceptable, and if so will you clarify the spec?

The spec says

Fails if the number of outputs is less or equal to index.

which I interpret to explicitly require the current behavior.

I think this line says that CHECKOUTPUT should fail execution if asked to find out-of-bounds output:

Fails if the number of outputs is less or equal to index.

Same as in txgraph spec.

@bobg i guess "outputs" should be "destinations" and moved into appropriate part (under "if we are in the Mux").

the spec is silent

Fails if the number of outputs is less or equal to index.

😊

Carry on...

Did you have thoughts on my actual suggestion above?

But since it can't [be a simple predicate], it might look a little cleaner to have it return error that's one of vm.ErrBadValue or vm.NotFound or nil.

kr · 2017-03-23T22:07:56Z

protocol/vm/introspection.go

 	err := vm.applyCost(1)
 	if err != nil {
 		return err
 	}

-	return vm.push(ii.Nonce, true)
+	if vm.context.Nonce == nil {


Doing this check after applying the cost (rather than before) looks like a behavior change from what we had before. Is that intentional? Should it be mentioned in the commit message?

My assumption going in was that this PR would be pretty much a pure behavior-preserving refactoring.

It's a pretty inconsequential behavior change that only manifests when both error conditions are true (it's the wrong context and the runlimit is exhausted). The old code would detect one of the errors first, the new code detects the other. (It still works the same way in the case of zero errors or one.) I don't think it's worth mentioning or changing.

Cool. Just something to keep in mind, stuff like this makes it harder to review. Because I'm asking myself (and then debating whether I should bring it up in review feedback) "does this look like it was on purpose? will it matter?" And anyone who reads this commit in the future will be left with the same questions (unless they find their way to this discussion thread).

If a PR is really a pure refactoring, then review is more straightforward, because there won't be any behavior changes. And if there are, the reviewer can just flag them and then move on.

kr · 2017-03-23T22:12:34Z

protocol/vm/vm.go

-				return err
+		if r := recover(); r != nil {
+			if rErr, ok := r.(error); ok {
+				err = errors.Sub(ErrUnexpected, rErr)


This looks like it's doing some new work to preserve error information if the panic value is an error. Is this really related to switching the dependency direction? I don't understand how. I think it's a good change, but maybe in its own PR?

kr · 2017-03-23T22:15:46Z

protocol/vm/context.go

+	BlockTimeMS          *uint64
+	NextConsensusProgram *[]byte
+
+	TxSigHash        *[]byte


A slice is capable of being nil on its own, so if you want, the slice fields here don't have to be pointers. (Same as for the func field).

The slices that are hashes could be nil to mean "absent," because we know they should be non-empty if present. But other slices in this struct could be "empty and present," so those do need to be pointers to slices. My (weak) aesthetic preference was to handle all the slices that way.

There's a difference between a nil slice (which would mean absent) and a zero-length but non-nil slice (which would mean empty and present). So they don't need to be pointers to slices. But I don't feel strongly about it, I just wanted to raise the possibility.

Oh good point. But, I don't know, I don't much like the idea of relying on the difference between []byte(nil) and []byte{} because I've seen one turn into the other too easily. Maybe just pilot error, but still.

kr · 2017-03-23T22:17:23Z

protocol/vm/context.go

+
+	TxVersion *uint64
+
+	BlockHash            *[]byte


It would be nice to document how the optional fields work, maybe in the godoc for type Context or func Verify. Something brief would be fine, e.g. something like what I wrote in an earlier comment:

// Fields below this point are optional. If the corresponding opcode // executes on a nil field, Verify will return ErrContext.

kr · 2017-03-23T23:02:06Z

LGTM

Remove vm->bc dependency.

93fd5e5

bobg added the PTAL label Mar 22, 2017

kr reviewed Mar 22, 2017

View reviewed changes

s/VMContext/Context/, and change from an interface with two implement…

df6946c

…ations to a struct with a lot of optional fields.

kr reviewed Mar 23, 2017

View reviewed changes

bobg added 2 commits March 23, 2017 15:45

Document the optional fields of a vm.Context.

4b58b74

Remove an unrelated change; it will go into its own PR.

a0f1323

kr approved these changes Mar 23, 2017

View reviewed changes

auto rev id

7bfd324

iampogo merged commit 21df39c into main Mar 23, 2017

iampogo deleted the vmcontext branch March 23, 2017 23:05

kr mentioned this pull request Mar 24, 2017

protocol/bc: add an Entry pointer to ValueSource, exclude it from hashing #802

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

protocol/vm: remove the dependency on protocol/bc #797

protocol/vm: remove the dependency on protocol/bc #797

bobg commented Mar 22, 2017 •

edited

bobg commented Mar 22, 2017

kr commented Mar 22, 2017

kr Mar 22, 2017

bobg Mar 23, 2017

kr Mar 22, 2017

kr Mar 22, 2017

bobg Mar 23, 2017

bobg commented Mar 23, 2017

kr Mar 23, 2017

bobg Mar 23, 2017

kr Mar 23, 2017

oleganza Mar 23, 2017

oleganza Mar 23, 2017

bobg Mar 23, 2017

kr Mar 23, 2017 •

edited

kr Mar 23, 2017

bobg Mar 23, 2017

kr Mar 23, 2017

kr Mar 23, 2017

kr Mar 23, 2017

bobg Mar 23, 2017

kr Mar 23, 2017

bobg Mar 23, 2017

kr Mar 23, 2017

kr commented Mar 23, 2017

protocol/vm: remove the dependency on protocol/bc #797

protocol/vm: remove the dependency on protocol/bc #797

Conversation

bobg commented Mar 22, 2017 • edited

bobg commented Mar 22, 2017

kr commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobg commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kr Mar 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kr commented Mar 23, 2017

bobg commented Mar 22, 2017 •

edited

kr Mar 23, 2017 •

edited