Code Review: Smart Contract VM 02/13/19 #921

kantai · 2019-02-13T20:27:04Z

This PR adds the following:

Contract interface for executing a transaction within the context of a transaction, with an in-memory implementation of that interface.
(define-public ...) function for specifying the public functions in a contract.
Support for buffer literals via hex strings or ascii string literals.
Type definitions that use the structural representation described in SIP-002 (i.e., (list 3 (buffer 5)))
Max value size enforcement
Max stack and context depth enforcement

This PR also reimplements the lexer from #908 so that it is a much more standard lexer (and the lexer itself handles literals).

This PR addresses:

#911
#913 -- though this will need to be revisited during the course of the implementation
#914
#915
#917
#918

…reterResult

…uffer

…ct-vm

…of use warnings)

…clones

…paration for creating contract contexts-- a transaction should _never_ be able to modify a contract's global context.

kantai · 2019-02-15T23:32:48Z

Pushed some more work into this PR:

Adding type parameters to (define-public ...): #922

Use loose type admission for maximum buffer/tuple size: basically, we want it so that we enforce a maximum buffer size, but will accept smaller buffers (the containing type is just the maximum of its composite types)

Adding initial implementation of a "Principal" type, parsed as described in SIP-002. I got a little carried away on this and implemented C32 address decoding -- see the implementation in src/address/c32.rs

jcnelson · 2019-02-18T02:46:44Z

src/vm/callables.rs

+                }
+            }
+        } else {
+            let types = self.types.as_ref().unwrap(); // if types is None, and is_public = true, we should panic.


Totally fine that this panics, but could you insert an error message here as well?

Alternatively, you could make this type of function unrepresentable by removing is_public from DefinedFunction and instead create a type enumeration like this:

pub enum FunctionTypes { PublicFunction(DefinedFunction), PrivateFunction(DefinedFunction) }

Then, you'd simply handle this as a match statement.

Yep, I'll create a new type for this

jcnelson · 2019-02-18T02:55:49Z

src/vm/contexts.rs

@@ -21,26 +28,38 @@ impl <'a> Environment <'a> {
    }
 }

+// Aaron: note -- only the global context will ever have DefinedFunctions
+//        so it is probably worthwhile to separate into 2 types.
+


Probably a good idea -- goes along with the general Rust-ism of avoiding non-sensical instantiations by making them unrepresentable in the first place.

jcnelson · 2019-02-18T02:58:44Z

src/vm/errors.rs

+    BadSymbolicRepresentation(String),
+    ReservedName(String),
+    InterpreterError(String),
+    MultiplyDefined(String)


"MultiplyDefined"? As in, defined multiple times, or something to do with the multiplication operator?

Ah, it's an error when you use the same variable name multiple times in a variable declaration like, the function definition:

(define (foo a a) (+ a a))

I can rename it to VariableDefinedMultipleTimes

jcnelson · 2019-02-18T03:09:27Z

src/vm/types.rs

    List(Vec<Value>, TypeSignature),
+    Principal(u8, Vec<u8>), // a principal is a version byte + hash160 (20 bytes)


You can enforce this in the type system by using [u8; 20] instead of Vec<u8>

jcnelson · 2019-02-18T03:10:44Z

src/vm/types.rs

+        let mut value_size: i128 = 0;
+        for (name, type_signature) in self.type_map.iter() {
+            // we only accept ascii names, so 1 char = 1 byte.
+            name_size = name_size.checked_add(name.len() as i128).unwrap();


For these unwrap()s, can you add an error message?

Also, how certain are we that these unwrap()s never occur?

Sure -- will add an error message.

These panics should never occur, since the composite values of the tuple should be smaller than MAX_VALUE_SIZE (much smaller than i128::max bytes) and combining them until the tuple is too large would require an extraordinarily large program. However, since the max value size check for tuple (and list) types occurs using the size() method, it's theoretically possible. So I'll change these to raise a ValueTooLarge error.

src/vm/types.rs

jcnelson · 2019-02-18T03:22:13Z

src/vm/types.rs

+    //     e.g.: (list "abcd" "abc") will currently error because one etry is
+    //           if type (buffer 4) and the other is of type (buffer 3)
+    //       my feeling is that this should probably be allowed, and the resulting
+    //       type should be (list 2 (buffer 4)) 


I agree, just as long as we can make it clear somehow that (list "abcdefghijk" "abc") costs as much to process as (list "abcdefghijk" "\x00\x00\x00\x00\x00\x00\x00\x00\x00abc"). We'd also want to be careful about how we "expand" shorter buffer items -- do we left-pad them with 0?

yep -- we'd need to make this clear. I think the behavior we want is that for any functions which accepts two buffers, to treat the smaller buffer as left padded with zeros. currently we don't have any buffer functions (except eq?).

How do you want to handle eq? for buffers that get implicitly left-padded with zeros? I don't think \x00abc and abc are necessarily equal. Maybe the buffer type should include a length field internally to avoid this problem?

Yeah, I was just going to comment on this. Those shouldn't be equal. Buffers should have a length, which they do currently (via their vec.len()), but the Buffer type has a maximum length, which is a little silly for a buffer's runtime type (because its length is always equal to its maximum length), but for declared types like public function inputs and datamap keys, values, it makes sense, because the maximum length determines whether or not a given runtime value would be admissable.

(this contradicts my previous response up there --- I don't think we should be automatically extending buffers. if we decide to include buffer functions like strncat, they should specify their padding behavior, if they have any)

jcnelson · 2019-02-18T03:25:44Z

src/vm/parser.rs

+    //    lazy_static (or just hand implementing that), and I'm not convinced
+    //    it's worth either (1) an extern macro, or (2) the complexity of hand implementing.
+    let lex_matchers: &[LexMatcher] = &[
+        LexMatcher::new(r##""(?P<value>((\\")|([[:ascii:]&&[^"\n\r\t]]))*)""##, TokenType::StringLiteral),


What is :ascii: here? Is it printable ASCII characters? Also, can we have a lexer test below for ensuring that a code snippit with non-printable characters does not parse?

Also, the str type is a unicode string slice. We'll want the lexer to reject non-ASCII strings.

Yeah -- that :ascii: class should actually be :print: for printable ascii characters.

But this lexer will reject non-ASCII strings. None of these matchers will match a non-ascii character, which will cause the lexer to fail (if the lexer cannot process a given character, it exhaust all the matches and returns a ParseError)

… Value to derive. remove now uneccessary Hash traits.

…21319

kantai · 2019-02-19T17:34:07Z

Okay -- I pushed changes that addressed all of those issues.

Use two types for Global vs. Local contexts.
Use two types for public vs. private defined functions.
Raise a ValueTooLarge error in the event of an i128 overflow in type size.
Use printable ascii character class instead of ascii character class (and add unicode rejection test)
Use [u8;20] for the principal data
Rename MultiplyDefined error
Implement value equality check which checks for data equality not type equality. Will create a new issue for speccing out buffer functions.

jcnelson · 2019-02-19T18:39:36Z

Thanks! As soon as CircleCI finishes, please go ahead and merge to develop. Thanks also for implementing c32.rs 😀

kantai · 2019-02-19T19:00:25Z

Thanks for the review! The Circle CI tests look like they've finished successfully (https://circleci.com/gh/blockstack/blockstack-core/1716), so I'll go ahead and merge (Circle github notifications/commit check updates appear to currently be broken).

kantai added 30 commits January 30, 2019 15:52

extending list type information with max_len

b664716

use type _admission_ for database checks, rather than type equality

fccd156

cleanup "use" statements, use a generic result type instead of Interp…

3d46937

…reterResult

more test coverage, test list type admission in datamaps

db254b4

more test coverage.

4c7f7e5

eliminate unneccessary boxing

0ef94c3

enforce max stack depth (currently 128). use u8 instead of char for b…

11b1a31

…uffer

do literal lexing in the actual lexer. use something like a munch lexer.

0add35c

get_data_map / get_mut_data_map for the contract db interface

2ba047a

catch potential dimension overflow in list construction

47ddc2c

move environment initialization out of eval_all

fe8e394

issue #914 -- enforce name legality at (define...)

02f15cb

missing source file

898c1ac

handle define legality in functions/define.rs

b53e515

check for reserved names in let-expressions

a7391fd

tests for maximum stack depth

7852400

use splitn for a bounded number of splits

9d88516

enforce maximum context depth

393428e

Merge branch 'review/smart-contract-013019' into feature/smart-contra…

046ec96

…ct-vm

merge reorg of src/vm

594d0cc

Merge branch 'review/smart-contract-013019' into feature/smart-contra…

09f31e3

…ct-vm

Merge branch 'review/smart-contract-013019' into feature/smart-contra…

e929979

…ct-vm

add fmt::Display impl for Values. add CLI command (mostly to get rid …

4ed2a7d

…of use warnings)

use 256 for max stack, context depth. use drain() in parser to avoid …

06e1f04

…clones

Merge branch 'develop' into feature/smart-contract-vm

ec2af76

u8 is too small for 256!

ba2d703

refactor value construction to allow for value size enforcement.

53f6e3f

environments now have immutable ref to global context. this is in pre…

6c48a5b

…paration for creating contract contexts-- a transaction should _never_ be able to modify a contract's global context.

update tests to work with env constructions

388743c

add execute_transaction impl for skeletal contract interface

c981aca

jcnelson reviewed Feb 18, 2019

View reviewed changes

src/vm/types.rs Show resolved Hide resolved

jcnelson reviewed Feb 18, 2019

View reviewed changes

src/vm/types.rs Show resolved Hide resolved

jcnelson reviewed Feb 18, 2019

View reviewed changes

kantai added 10 commits February 18, 2019 09:15

return error from size() instead of panics on overflow.

d062cd9

handle the sender argument to contract execution

c62297d

use [u8;20] array instead of vec for principal type

476cd7f

use two types for public and private functions

8791851

separate two kinds of contexts (local and global) into two types

241b3cc

implement equality for Value enum

f3177ee

implement partialeq, hash for ListData and TupleData manually: allows…

74e45cb

… Value to derive. remove now uneccessary Hash traits.

rename MultiplyDefined error to VariableDefinedMultipleTimes

bb20395

light cleaning up of test functions

29a0225

Merge branch 'feature/smart-contract-vm' into review/smart-contract-0…

0634a09

…21319

kantai merged commit 062c4da into develop Feb 19, 2019

kantai deleted the review/smart-contract-021319 branch January 27, 2021 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Review: Smart Contract VM 02/13/19 #921

Code Review: Smart Contract VM 02/13/19 #921

kantai commented Feb 13, 2019

kantai commented Feb 15, 2019

jcnelson Feb 18, 2019

jcnelson Feb 18, 2019

kantai Feb 18, 2019

jcnelson Feb 18, 2019

kantai Feb 18, 2019

jcnelson Feb 18, 2019

kantai Feb 18, 2019

jcnelson Feb 18, 2019

jcnelson Feb 18, 2019

jcnelson Feb 18, 2019

kantai Feb 18, 2019

jcnelson Feb 18, 2019 •

edited

kantai Feb 18, 2019

jcnelson Feb 19, 2019

kantai Feb 19, 2019

kantai Feb 19, 2019

jcnelson Feb 18, 2019

jcnelson Feb 18, 2019

kantai Feb 18, 2019

kantai commented Feb 19, 2019

jcnelson commented Feb 19, 2019

kantai commented Feb 19, 2019

		List(Vec<Value>, TypeSignature),
		Principal(u8, Vec<u8>), // a principal is a version byte + hash160 (20 bytes)

Code Review: Smart Contract VM 02/13/19 #921

Code Review: Smart Contract VM 02/13/19 #921

Conversation

kantai commented Feb 13, 2019

kantai commented Feb 15, 2019

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

Choose a reason for hiding this comment

jcnelson Feb 18, 2019 • 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

kantai commented Feb 19, 2019

jcnelson commented Feb 19, 2019

kantai commented Feb 19, 2019

jcnelson Feb 18, 2019 •

edited