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

Clarity <-> MARF #1084

Merged
merged 33 commits into from Aug 16, 2019

Conversation

@kantai
Copy link
Member

commented Aug 8, 2019

This PR integrates the MARF into Clarity (#1055)

The big items:

  1. Clarity no longer depends on a bunch of SQLite savepoint behavior for handling rollbacks / commits on transaction errors and response types. This is no implemented via the RollbackWrapper class. The most notable change here is that the borrow checker is no longer used to enforce a "one savepoint -> one commit" mapping (as it is in rusqlite).
  2. There's a new trait KeyValueStorage which is used to wrap different backends for ClarityDB and AnalysisDB classes.
  3. The in-code naming convention of Assets/Tokens is changed to NFTs / FTs.
  4. Addressing #968, #989 -- I did this by extending the enum macro to create get_name and lookup_by_name for a new DefineFunction enum. This reduces the duplication of function naming pretty significantly (especially in docs gen).
  5. Address #1085 by adding a test (the lexer already forbade variable names with .), and prepare the lexer for "qualified contract names".

Things left unaddressed:

  1. Improved serialization of Clarity values.
  2. Block info simulation -- the local dev environment still simulates blocks by "mining them into the db". This won't reflect the block structure of the MARF. This is left unaddressed because I think the block information ultimately needs to be handled by a process that is a higher level than Clarity, and I don't want to spend effort on implementing a new block simulation structure when it would likely need to be replaced again.
kantai added 13 commits Aug 5, 2019
…ctor of vm to use ClarityDatabase rather than prior SQLite connections
…interface for callers of globalcontext (rather than needing to use begin/rollback/commit
…ment' into feature/clarity-marfs
… marf. add test structure in tests/contracts.rs for marfed kv
…hMap K/V store and Marf K/V
… the k/v wrapper code
@codecov

This comment has been minimized.

Copy link

commented Aug 8, 2019

Codecov Report

Merging #1084 into develop will decrease coverage by <.01%.
The diff coverage is 92.63%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1084      +/-   ##
===========================================
- Coverage    80.34%   80.33%   -0.01%     
===========================================
  Files           98      105       +7     
  Lines        23786    23648     -138     
===========================================
- Hits         19110    18998     -112     
+ Misses        4676     4650      -26
Impacted Files Coverage Δ
src/deps/bitcoin/blockdata/opcodes.rs 70.27% <ø> (ø) ⬆️
src/vm/tests/contracts.rs 80.9% <100%> (-1.07%) ⬇️
src/vm/checker/mod.rs 100% <100%> (ø) ⬆️
src/chainstate/stacks/index/storage.rs 67.75% <100%> (+0.42%) ⬆️
src/vm/database/mod.rs 100% <100%> (ø)
src/vm/tests/forking.rs 100% <100%> (ø)
src/vm/functions/mod.rs 96.64% <100%> (-0.43%) ⬇️
src/vm/tests/simple_apply_eval.rs 96.47% <100%> (-0.05%) ⬇️
src/vm/tests/datamaps.rs 98.37% <100%> (-0.02%) ⬇️
src/vm/database/structures.rs 100% <100%> (ø)
... and 39 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 cebeaa0...40d98e1. Read the comment docs.

src/vm/database/marf.rs Outdated Show resolved Hide resolved
@kantai kantai changed the title [Work in Progress] Clarity <-> MARF Clarity <-> MARF Aug 13, 2019
@kantai kantai requested a review from jcnelson Aug 13, 2019
src/vm/types.rs Show resolved Hide resolved
src/clarity.rs Show resolved Hide resolved
@jcnelson jcnelson self-requested a review Aug 14, 2019
Copy link
Member

left a comment

The code overall looks great. Thanks for integrating the MARF into Clarity so quickly! 👍

However, there's something that was overlooked in the tests (and to a lesser extent, the API): dealing with forks. As far as I could see, there are no tests as to what happens if the chain forks. Are clarity objects created in one fork absent in the other, and vice versa? Are clarity objects created prior to the fork present in both? The MARF was designed to address this situation, and it looks like the code uses the MARF to do so, but there needs to be tests. This also means that the MARF API should expose to Clarity a way to get the longest fork (if one is not given), and the Clarity datastore get method needs to be able to take a particular chain tip (or default to the tip of the longest fork if not given).

Related, I think the Clarity CLI should support the notion of simulated-mining of forks at some point -- I should be able to build multiple children off of a single parent, and I should be able to launch different contracts and tokens in different forks.

@kantai

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

However, there's something that was overlooked in the tests (and to a lesser extent, the API): dealing with forks. As far as I could see, there are no tests as to what happens if the chain forks.

Yep -- I can add some tests that perform forks. The expected outcome is that the side store will store the mapping of MARFValue (hashed values) to values for all forks processed, but that the clarity db's "view" of the data will be the one presented by the open MARF block.

This also means that the MARF API should expose to Clarity a way to get the longest fork (if one is not given)

Yeah, I think there should be an API exposed, but Clarity's interface with the MARF ultimately wouldn't be affected by it -- rather the caller of the Clarity VM would handle it outside of the context of Clarity -- e.g., the way that the routines in the clarity local environment (clarity.rs) handle beginning and committing the marf blocks.

and the Clarity datastore get method needs to be able to take a particular chain tip (or default to the tip of the longest fork if not given).

I don't think that's necessarily the case. Any given Clarity context is only ever the execution context for a single transaction, so a context doesn't really need to be aware of whether or not it is in a fork (what's more, there's only ever one block that a context calls get with). The 'current block' will be set by the Clarity caller (i.e., the block handler loop).

src/vm/checker/check_db.rs Outdated Show resolved Hide resolved
src/vm/checker/read_only/mod.rs Show resolved Hide resolved
src/vm/checker/typecheck/mod.rs Outdated Show resolved Hide resolved
src/vm/checker/typecheck/mod.rs Show resolved Hide resolved
src/vm/database/key_value_wrapper.rs Outdated Show resolved Hide resolved
src/vm/database/marf.rs Show resolved Hide resolved
src/vm/contexts.rs Outdated Show resolved Hide resolved
@kantai

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

Added test of forking behavior in vm::tests::forking in c5f3d9f

kantai added 2 commits Aug 16, 2019
Copy link
Member

left a comment

Looks good to me, thank you @kantai!

@jcnelson jcnelson self-requested a review Aug 16, 2019
Copy link
Member

left a comment

Looks good to me!

@kantai kantai merged commit 77961d1 into develop Aug 16, 2019
6 checks passed
6 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: test_demo Your tests passed on CircleCI!
Details
codecov/patch 92.63% of diff hit (target 80.34%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +12.29% compared to cebeaa0
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.