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.

Sign up for GitHub

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

Jump to bottom

Merkle Patricia Trees #280

Merged
merged 3 commits into from Nov 2, 2019
Merged

Merkle Patricia Trees #280

merged 3 commits into from Nov 2, 2019

Conversation

MrChico
Copy link
Member

@MrChico MrChico commented Sep 25, 2019

Includes #278
Initial work on Merkle Patricia Trie encoding. Correctly hashes

["test", "test"],
["te", "testy"]

to
0x8452568af70d8d140f58d941338542f645fcca50094b20f3c3d8c3df49337928

@MrChico MrChico changed the title Mpt Merkle Patricia Trees Sep 25, 2019
@MrChico MrChico requested a review from livnev September 25, 2019 21:11
@MrChico MrChico marked this pull request as ready for review October 29, 2019 20:02
@MrChico
Copy link
Member Author

MrChico commented Oct 29, 2019

This PR is now ready for merge. @livnev please review:
Includes:

  • RLP encoding and decoding, including checks ensuring that the encoding is optimal in order to exhibit a partial isomorphism between ByteString and RLP as probabilistically proven by QuickCheck istances (see appendix A). RLP decoding can be run using the cli hevm rlp --decode 0xc1c0
  • Merkle Patricia Trie implementation compliant with the tests in https://github.com/ethereum/tests/blob/develop/TrieTests/trietest.json (which is the only test with a sane format). This test can be run with the cli hevm merkle-test --file PATH_TO_TRIETEST.JSON. If we want we can add this to the ci process.

Appendix A:

We exhibit the partial isomorphism witnessed by rlp.encode and rlp.decode by the quickcheck instances:

testProperty "rlp encode is a partial inverse (bytes)" $ \(Bytes bs) ->
      case rlpdecode bs of
        Just r -> rlpencode r == bs
        Nothing -> True

and

testProperty "rlp decode is a retraction (RLP)" $ \r ->
       rlpdecode (rlpencode r) == Just r

Cranking the withMaxSuccess 100000 $ demonstrates the flawed "suboptimal encodings" that are admitted by the naïve implementation (see previous commits).

livnev
livnev reviewed Nov 2, 2019
View reviewed changes
Copy link
Member

@livnev livnev left a comment

Choose a reason for hiding this comment

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

Agree wholeheartedly with this commit.

src/hevm/src/EVM/Transaction.hs Outdated
k <- choose (0,10)
ls <- vectorOf k arbitrary
return $ List ls)
]
Copy link
Member

Choose a reason for hiding this comment

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

Unless you strongly disagree, I would prefer to move these quickcheck instances to test.hs to avoid cluttering the core code.

src/hevm/src/EVM/Transaction.hs Outdated
rlpWord256 n = rlpencode $ BS $ word256Bytes n

rlpWord160 :: Addr -> ByteString
rlpWord160 n = rlpencode $ BS $ word160Bytes n
Copy link
Member

Choose a reason for hiding this comment

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

I would also prefer to move all of the technical RLP stuff to a new file RLP.hs

src/hevm/src/EVM/Transaction.hs Outdated

newContractAddressCREATE2 :: Addr -> W256 -> ByteString -> Addr
newContractAddressCREATE2 a s b = num $ keccak $ mconcat $
[BS.singleton 0xff, word160Bytes a, word256Bytes $ num s, word256Bytes $ keccak b]
Copy link
Member

Choose a reason for hiding this comment

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

Not sure these two methods had to be moved here, I might prefer to put them back.

src/hevm/src/EVM/Types.hs
word256Bytes x = BS.pack [byteAt x (31 - i) | i <- [0..31]]

word160Bytes :: Addr -> ByteString
word160Bytes x = BS.pack [byteAt (addressWord160 x) (19 - i) | i <- [0..19]]
Copy link
Member

Choose a reason for hiding this comment

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

Agree about moving these guys in here though.

src/hevm/src/EVM/Types.hs
unpackNibbles bs = BS.unpack bs >>= unpackByte
where unpackByte b = [hi b, lo b]

--Well-defined for even length lists only (plz dependent types)
Copy link
Member

Choose a reason for hiding this comment

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

Let's avoid political controversy by using only neutral language in our comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea that political controversy is something that needs to be avoided is a belief that maintains injustice like keeping DTT out of GHC. I refuse to be silenced

Copy link
Member

Choose a reason for hiding this comment

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

Please keep politics out of Haskell.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's political

src/hevm/test/test.hs
@@ -84,7 +87,6 @@ main = defaultMain $ testGroup "hevm"
Nothing
(execute 1 (h <> v <> r <> s) 32)
]
]
Copy link
Member

Choose a reason for hiding this comment

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

oops

src/hevm/test/test.hs

instance Arbitrary Bytes where
arbitrary = fmap (Bytes . BS.pack) arbitrary

Copy link
Member

Choose a reason for hiding this comment

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

moving this back unless you disagree

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good

src/hevm/hevm.cabal Outdated
tasty-hunit >= 0.10,
tasty-quickcheck >= 0.9,
text >= 1.2,
vector >= 0.12
Copy link
Member

Choose a reason for hiding this comment

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

reformatting to avoid unnecessary diff noise

@MrChico
hevm: RLP encoding and decoding support
ba1c140 
RLP: add encoding functionality

hevm: refactor Keccak to utilize rlpencode

hevm: cli cleanup and minor polish

Fix RLP stuff: a quickcheck story

hevm: move rlp stuff to RLP, put create address stuff in Concrete,
move quickcheck instances to test.hs

More property based testing

hevm/EVM.hs: use padLeft instead of frontPad
@MrChico
hevm: merkle patrcia tree support
08d0864 
hevm: Simplify normalizing function

hevm: everything in it's right place

hevm: Split normalizing, insertPath to update, delete and other simplifications

hevm: fix merkle tries and code cleanup

hevm: Fix Patricia Merkle Trie (Quickcheck discoveries!)

test.hs: correct list grouping typo

hevm: reörder imports

hevm: clarify some comments

hevm.cabal: reformat to remove diff noise

hevm: fix some imports

hevm: Patricia.hs: format with more linebreaks

hevm: more cleanup
@MrChico
hevm: version 0.35
0083ccc
@livnev livnev merged commit a9490ca into master Nov 2, 2019
@livnev livnev deleted the mpt branch November 2, 2019 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@livnev livnev livnev left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@MrChico @livnev