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

Add RLP encoding implementation to evmone::state library #463

Merged
merged 3 commits into from
Jun 16, 2022
Merged

Conversation

chfast
Copy link
Member

@chfast chfast commented May 6, 2022

No description provided.

@codecov
Copy link

codecov bot commented May 6, 2022

Codecov Report

Merging #463 (1f6005b) into master (4a5dada) will not change coverage.
The diff coverage is n/a.

❗ Current head 1f6005b differs from pull request most recent head a5dde60. Consider uploading reports for the commit a5dde60 to get more accurate results

@@           Coverage Diff           @@
##           master     #463   +/-   ##
=======================================
  Coverage   99.60%   99.60%           
=======================================
  Files          39       39           
  Lines        4593     4593           
=======================================
  Hits         4575     4575           
  Misses         18       18           
Flag Coverage Δ
consensus 79.12% <0.00%> (ø)
unittests 99.65% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@chfast chfast marked this pull request as ready for review May 6, 2022 20:51
@chfast chfast requested review from axic, gumb0 and yperbasis May 6, 2022 20:51
Copy link
Member

@axic axic left a comment

Choose a reason for hiding this comment

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

Should consider adding/reviewing test cases from

I know projects which found bugs after adding all these, as each of them are not comprehensive. (Should try to merge all these tests.)

@@ -0,0 +1,117 @@
// evmone: Fast Ethereum Virtual Machine implementation
// Copyright 2021 The evmone Authors.
Copy link
Member

Choose a reason for hiding this comment

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

👀

Copy link
Member Author

Choose a reason for hiding this comment

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

I started this last year :(

namespace evmone::rlp
{
using bytes = std::basic_string<uint8_t>;
using bytes_view = std::basic_string_view<uint8_t>;
Copy link
Member

Choose a reason for hiding this comment

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

Weren't these defined somewhere? Or only in Fizzy?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is already defined in multiple places (2x in EVMC). But as long as the definition is the same this is fine. Notice this RLP implementation does not include any evmone/evmc headers. Also intx include can be easily dropped.

@chfast
Copy link
Member Author

chfast commented May 25, 2022

Should consider adding/reviewing test cases from

I think I can easily manually port "official" and geth tests.

@chfast chfast force-pushed the state_rlp branch 3 times, most recently from 2dc2339 to a5dde60 Compare June 15, 2022 09:02
@chfast chfast changed the base branch from master to evmc_hex_upgrade June 15, 2022 09:31
@chfast chfast force-pushed the evmc_hex_upgrade branch from f3d2c49 to 9a34f3f Compare June 15, 2022 10:22
Base automatically changed from evmc_hex_upgrade to master June 15, 2022 11:21
test/state/rlp.hpp Outdated Show resolved Hide resolved
test/state/rlp.hpp Outdated Show resolved Hide resolved
@chfast chfast force-pushed the state_rlp branch 2 times, most recently from 7b1099c to db90da9 Compare June 15, 2022 15:02
test/state/rlp.hpp Outdated Show resolved Hide resolved
TEST(state_rlp, encode_mpt_node)
{
const bytes path{0x20, 0x41};
const bytes value{'v', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_', '_',
Copy link
Member

Choose a reason for hiding this comment

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

what are these chars? maybe better represent them in hex?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a test case evolved...

test/state/rlp.hpp Outdated Show resolved Hide resolved
Copy link
Member

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

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

just one more place to use decimal value

@chfast chfast merged commit 5b982ac into master Jun 16, 2022
@chfast chfast deleted the state_rlp branch June 16, 2022 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants