Code Review: Smart contract VM 01/30/19 #908

kantai · 2019-01-30T17:33:09Z

This is the first code review submission for the smart contract VM:

This code implements a basic LISP language, with support for ints, bools, tuples, and lists, for now. Recursion in this language is illegal. There is limited run-time type enforcement -- lists must all be single typed, tuples are strongly typed when used with data-map functions, native functions that expect ints, bools, etc. check their argument types.

At the moment, data-map functions like fetch-entry!, etc., all operate within a global context, set by the eval_all function. You can see test programs in the tests/ directory.

…epresentation of lisp s-exps.

… arithmetic.

…t. use a global context, which will work for our limited lisp dialect

… serializers for Secp256k1PublicKey. Also, update new location of BitcoinNetworkType

…)serializer methods for Secp256k1PublicKey

… trait to instantiate history rows from individual burn chain operations; implement helpers to insert and select rows of burn chain operations and convert them to and from database rows; implement basic DB tests

…tom) data type from a Sqlite3 Row

…o describe the order in which their column fields should be SELECT'ed

jcnelson · 2019-02-04T15:41:31Z

blockstack-vm/src/database.rs

+        Ok(())
+    }
+
+    fn insert_entry(&mut self, key: Value, value: Value) -> InterpreterResult {


Should we consider putting an upper bound on how many bytes an inserted entry can be?

I'd rather enforce it at the type level, as in, we should have a maximum byte limit for types, and the database should allow any entry that is a legal type.

Agreed re: making Value have a maximum byte size.

jcnelson · 2019-02-04T15:42:13Z

blockstack-vm/src/functions/arithmetic.rs

+    binary_comparison(args, &|x, y| x < y)
+}
+
+pub fn native_add(args: &[Value]) -> InterpreterResult {


What happens if args has length 0 or 1? Also, will args ever have length > 2? Same question for native_sub, native_mul, and native_div.

(+ 1 2 3) evaluates to 6.
(+) returns 0

For native_sub (-) errors, (- 1) returns the negation of it's single argument, (- 1 2 3) return -4.

Multiply and divide behave similarly ((*) returns 1, (/) is an error)

Got it 👍

jcnelson · 2019-02-04T15:45:16Z

blockstack-vm/src/functions/define.rs

+        }
+    }).collect();
+
+    let names = coerced_atoms?;


Why is the ? operator on its own line? Just curious.

The type inference didn't want to work otherwise -- if I move the ? operator up and eliminate the Result<> wrapper:

--> src/functions/define.rs:31:46 | 31 | arguments: arg_names.iter().map(|x| (*x).clone()).collect(), | ^ consider giving this closure parameter a type | = note: type must be known at this point

jcnelson · 2019-02-04T15:47:24Z

blockstack-vm/src/functions/define.rs

+                                "Illegal operation: attempted to re-define a value type.".to_string())),
+                            NamedParameter(ref _value) => Err(Error::InvalidArguments(
+                                "Illegal operation: attempted to re-define a named parameter.".to_string())),
+                            List(ref function_signature) => handle_define_function(&function_signature, &elements[2])


Is the user allowed to define functions that have reserved names?

Right now, it will allow it, though they'll never get called, because lookup_function tries the reserved names first, but it should be an error on define

jcnelson · 2019-02-04T15:47:44Z

blockstack-vm/src/functions/define.rs

+                        Err(Error::InvalidArguments("(define ...) requires 2 arguments".to_string()))
+                    } else {
+                        match elements[1] {
+                            Atom(ref variable) => handle_define_variable(variable, &elements[2], env),


Is the user allowed to define atoms that collide with reserved names?

Same as above -- we need to enforce name legality at define and let -- #914

jcnelson · 2019-02-04T15:48:27Z

blockstack-vm/src/functions/lists.rs

+use super::super::representations::SymbolicExpression::{AtomValue};
+use super::super::{Context,Environment,eval,apply,lookup_function};
+
+pub fn list_cons(args: &[Value]) -> InterpreterResult {


Is there an upper bound on how long a list can be?

Right now, no, but I think we should let the "maximum value size" enforce this (as in, if we have a maximum value size of 1000 bytes, then you cannot have a list of longer than 1000/16 integers, e.g.)

jcnelson · 2019-02-04T15:49:36Z

blockstack-vm/src/lib.rs

+fn lookup_variable(name: &str, context: &Context, env: &Environment) -> InterpreterResult {
+    // first off, are we talking about a constant?
+    if name.starts_with(char::is_numeric) {
+        match i128::from_str_radix(name, 10) {


Do we want to support alternative radixes? Like, say, base-16?

Maybe -- I was planning on lexing hexstrings into buffers, rather than ints. We could support both. Will create an issue for it.

jcnelson · 2019-02-04T15:50:35Z

blockstack-vm/src/lib.rs

+    }
+}
+
+pub fn lookup_function<'a> (name: &str, env: &Environment)-> Result<CallableType<'a>, Error> {


From this code, it looks like the user will not be able to change the runtime behavior of a reserved function by creating a function with the same name. However, I didn't see any code that prevents the user from doing so?

Right -- reserved functions are used first, before a user defined function would be. However, we would want to generate a runtime error at the point of the (define) (and also, when the static analyzer is implemented, a static error). I started an issue #914.

jcnelson · 2019-02-04T15:51:18Z

blockstack-vm/src/lib.rs

+ */
+pub fn eval_all(expressions: &[SymbolicExpression],
+                contract_db: Option<Box<database::ContractDatabase>>) -> InterpreterResult {
+    let db_instance = match contract_db {


This feels weird to me. I think the caller should always supply a contract DB.

Sure, I'll change that -- I'll move "blank db instantiation" to the execute function, the intent of which is just running a program in a single, transient smart contract context. That's not just for our testing purposes, but as a simple path for a developer wanting to test a script.

jcnelson · 2019-02-04T15:55:52Z

blockstack-vm/src/parser/mod.rs

+                    result.push(value);
+                }
+            },
+            _ => {


Do we want to accept non-printable characters with this match arm? Or should we just reject those as parse errors? I feel like our smart contract VM shouldn't accept non-printable characters (and should probably not use unicode) in order to ensure that the source code is unambiguous and doesn't have any homoglyph attacks or nefarious VT100 control codes embedded within it (which could manifest if you cat'ed the smart contract code to stdout in a terminal, for example).

Yeah, definitely should cause parse errors. The next code review will have a lexer that enforces that. It'll be a much more traditional munch-lexer.

jcnelson · 2019-02-04T15:57:20Z

blockstack-vm/src/types.rs

+            Value::Tuple(_a) => Err(Error::InvalidArguments("Cannot construct list of tuple types".to_string())),
+            _ => {
+                let mut base_type = TypeSignature::type_of(x);
+                base_type.dimension += 1;


Shouldn't this be a checked_add? What happens if the dimension overflows?

Yep, good catch, thanks

jcnelson · 2019-02-04T16:01:44Z

blockstack-vm/src/types.rs

+        Ok(TypeSignature::new(atom_type, dimension))
+    }
+
+    pub fn parse_type_str(x: &str) -> Result<TypeSignature, Error> {


How long can this string get? Asking because if we're going to split it, we might end up doing O(poly(n)) work for n occurrences of -. We might want to first count up the number of - occurrences first, and if it's greater than 4, error out.

Won't split only ever do O(n) work ?

Depending on the allocator implementation, it could do O(n) malloc()s, which each could take O(log m) time (for m blocks allocated). Not sure what the actual implementation does, but I'm of the school of thought that we should prepare for the worst -- especially since the attacker controls the input.

Okay, yes, avoiding non-constant object instantions is a good idea. Will just switch to using splitn here.

jcnelson · 2019-02-06T21:54:49Z

This looks really cool! This looks good to me to merge to develop, with one small change: could you put the code and tests under src/core/vm/, instead of blockstack-vm? This way it will be compiled into the blockstack-core binary. Then, I can add a CLI option to blockstack-core to load and run a script using state in an on-disk database (so we can get comfortable testing things out).

kantai · 2019-02-06T22:50:25Z

Yep, sounds good to me @jcnelson

…ng...

codecov-io · 2019-02-07T04:47:51Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@85500cc). Click here to learn what that means.
The diff coverage is 61.51%.

@@            Coverage Diff             @@
##             develop     #908   +/-   ##
==========================================
  Coverage           ?   64.78%           
==========================================
  Files              ?       44           
  Lines              ?     5637           
  Branches           ?        0           
==========================================
  Hits               ?     3652           
  Misses             ?     1985           
  Partials           ?        0

Impacted Files	Coverage Δ
src/util/macros.rs	`67% <ø> (ø)`
src/chainstate/burn/mod.rs	`0% <0%> (ø)`
src/burnchains/bitcoin/network.rs	`0% <0%> (ø)`
src/burnchains/bitcoin/indexer.rs	`0% <0%> (ø)`
src/core/mod.rs	`0% <0%> (ø)`
src/burnchains/bitcoin/spv.rs	`0% <0%> (ø)`
src/util/mod.rs	`0% <0%> (ø)`
src/main.rs	`2.63% <0%> (ø)`
src/burnchains/bitcoin/rpc.rs	`0% <0%> (ø)`
src/chainstate/burn/operations/mod.rs	`0% <0%> (ø)`
... and 33 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 85500cc...f5a019c. Read the comment docs.

kantai · 2019-02-07T04:57:03Z

Going forward, can we commit to staying on the stable rust compilers? It's hard for me to keep up with nightly, and I don't think it's a good idea to rely on unstable features in general.

I ripped checked_pow's implementation over from rust's code (https://github.com/milesand/rust/blob/master/src/libcore/num/mod.rs#L857), so we won't need to use nightly anymore. That's actually prepared for 1.34 (not 1.33), which means it's two releases away, which seems like too long for us to be using nightly/beta. Will mark an issue for us to remember to remove the code once 1.34 releases.

kantai · 2019-02-07T14:11:41Z

Cool -- I'm going to merge this. I think this is also going to merge PR #910.

blockstack-devops · 2024-11-25T00:22:43Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

kantai and others added 30 commits January 10, 2019 10:31

very rough initial apply/eval

cb03afb

some rough typing at the interpreter level

ec8f6e2

snake cases

eef1fe3

just build the vm

c456dca

rough contexts

206fa99

user defined functions

1be6197

start refactoring

8ea3955

refactor main into integration test

bfd5374

simple if statements and eq? implementation

9d81913

refactoring native/special function definitions out of lib.rs

c0287b2

change s-exp representation to enum of atom, list. this is a better r…

00e92f3

…epresentation of lisp s-exps.

implement let special function, add simple test

adcc746

add simple parsing.

e2f9558

add parser tests. add tests for native arithmetic. use checked native…

bdbd2fc

… arithmetic.

add call stack check to prevent recursion

5858cee

borrow checks will not like functions having pointers to their contex…

a0becda

…t. use a global context, which will work for our limited lisp dialect

support (defines) in global context

089ed92

add (define) and some tests for it

e017382

experiment with kcov

cd9ed34

change location of BitcoinNetworkType

c55c7e3

test BurnchainTxInput<BitcoinPublicKey> (de)serialization with custom…

4e7f7d7

… serializers for Secp256k1PublicKey. Also, update new location of BitcoinNetworkType

update location of BitcoinNetworkType

4044de0

BitcoinNetworkType is now mod-level

c659082

serde is broken in the rust-secp256k1 module, so implement custom (de…

4974ad4

…)serializer methods for Secp256k1PublicKey

BitcoinNetworkType is now a mod-level enum

f81d003

use new location of BitcoinNetworkType

565ab06

add FromRow trait to allow a uniform way of loading a particular (cus…

e2714cb

…tom) data type from a Sqlite3 Row

change location of BitcoinNetworkType

f162465

add a RowOrder trait that is used to force objects stored in the DB t…

733e078

…o describe the order in which their column fields should be SELECT'ed

jcnelson reviewed Feb 4, 2019

View reviewed changes

kantai mentioned this pull request Feb 6, 2019

Disallow arbitrary string characters in VM lexing #918

Closed

kantai added 6 commits February 6, 2019 17:00

Merge branch 'develop-jude' into review/smart-contract-013019

1595a2f

move to main src/ dir

6c13560

add author

965fb71

update src/vm files so that they will actually build

3099555

pin version on ed25519-dalek because the .1 version change was breaki…

78e9d6d

…ng...

unpin curve version.

f5a019c

kantai merged commit a74635a into develop Feb 7, 2019

kantai mentioned this pull request Feb 13, 2019

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

Merged

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

blockstack-devops added the locked label Nov 25, 2024

stacks-network locked as resolved and limited conversation to collaborators Nov 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Review: Smart contract VM 01/30/19 #908

Code Review: Smart contract VM 01/30/19 #908

kantai commented Jan 30, 2019

jcnelson Feb 4, 2019

kantai Feb 4, 2019

jcnelson Feb 4, 2019

jcnelson Feb 4, 2019 •

edited

Loading

kantai Feb 4, 2019

jcnelson Feb 4, 2019

jcnelson Feb 4, 2019

kantai Feb 4, 2019

jcnelson Feb 4, 2019

kantai Feb 4, 2019

jcnelson Feb 4, 2019

kantai Feb 6, 2019

jcnelson Feb 4, 2019

kantai Feb 6, 2019

jcnelson Feb 4, 2019

kantai Feb 6, 2019

jcnelson Feb 4, 2019

kantai Feb 6, 2019

jcnelson Feb 4, 2019

kantai Feb 6, 2019

jcnelson Feb 4, 2019 •

edited

Loading

kantai Feb 6, 2019

jcnelson Feb 4, 2019

kantai Feb 6, 2019

jcnelson Feb 4, 2019 •

edited

Loading

kantai Feb 6, 2019

jcnelson Feb 6, 2019 •

edited

Loading

kantai Feb 6, 2019

jcnelson commented Feb 6, 2019

kantai commented Feb 6, 2019

codecov-io commented Feb 7, 2019

kantai commented Feb 7, 2019

kantai commented Feb 7, 2019

blockstack-devops commented Nov 25, 2024

Code Review: Smart contract VM 01/30/19 #908

Code Review: Smart contract VM 01/30/19 #908

Conversation

kantai commented Jan 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcnelson Feb 4, 2019 • edited Loading

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

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 4, 2019 • edited Loading

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 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcnelson Feb 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcnelson commented Feb 6, 2019

kantai commented Feb 6, 2019

codecov-io commented Feb 7, 2019

Codecov Report

kantai commented Feb 7, 2019

kantai commented Feb 7, 2019

blockstack-devops commented Nov 25, 2024

jcnelson Feb 4, 2019 •

edited

Loading

jcnelson Feb 4, 2019 •

edited

Loading

jcnelson Feb 4, 2019 •

edited

Loading

jcnelson Feb 6, 2019 •

edited

Loading