Skip to content

Add support for recording SHA3 preimages#3543

Merged
fjl merged 7 commits into
ethereum:masterfrom
Arachnid:preimage
Jan 17, 2017
Merged

Add support for recording SHA3 preimages#3543
fjl merged 7 commits into
ethereum:masterfrom
Arachnid:preimage

Conversation

@Arachnid
Copy link
Copy Markdown
Contributor

No description provided.

@mention-bot
Copy link
Copy Markdown

@Arachnid, thanks for your PR! By analyzing the history of the files in this pull request, we identified @obscuren, @jimenezrick and @karalabe to be potential reviewers.

Comment thread core/database_util.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

naming suggestion: "PreimageTable" (without Get prefix)

Comment thread internal/web3ext/web3ext.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new operation should be called debug_preimage

Comment thread core/blockchain.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please write preimages as part of the state commit.

Copy link
Copy Markdown
Contributor Author

@Arachnid Arachnid Jan 11, 2017

Choose a reason for hiding this comment

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

Doing this introduces an import cycle - state imports core (for database_util), which imports state. This could be solved by removing all the preimage stuff from database_util, but reducing duplication in the number of places where we specify table prefixes etc seems like a Good Thing. Thoughts?

Comment thread core/state/statedb.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You don't need this method if you write them during Commit here.

Comment thread core/state/statedb.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please document this. It should say that the method is used for recording preimages seen by the VM.

@Arachnid Arachnid force-pushed the preimage branch 3 times, most recently from 6d122cf to 08c1213 Compare January 11, 2017 09:46
Comment thread eth/api.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This needs to be renamed to Preimage

Comment thread internal/web3ext/web3ext.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please call it preimage in JS to reduce confusion.

@GitCop
Copy link
Copy Markdown

GitCop commented Jan 11, 2017

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

  • Commit: 82fb946a1835c35da3fae4d627e19825bfab1107
  • Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.


This message was auto-generated by https://gitcop.com

@Arachnid
Copy link
Copy Markdown
Contributor Author

Arachnid commented Jan 11, 2017

After running some stats on a full-synced node, there are about 2M EVM sha3 preimages, with ~370MB of values (plus ~40 bytes per key, plus overhead). Given the low-ish volume, I'm thinking it probably makes sense to dump the flag and just record these unconditionally like we do for trie preimages.

For context, there are 21M trie preimages, totalling ~446M of values.

Thoughts?

Comment thread eth/backend.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This one and ForceJit may be deleted.

@GitCop
Copy link
Copy Markdown

GitCop commented Jan 11, 2017

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

  • Commit: 82fb946a1835c35da3fae4d627e19825bfab1107
  • Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.


This message was auto-generated by https://gitcop.com

Comment thread core/database_util.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please document this function

Comment thread core/database_util.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doc

Comment thread eth/api.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doc

@GitCop
Copy link
Copy Markdown

GitCop commented Jan 12, 2017

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

  • Commit: 82fb946a1835c35da3fae4d627e19825bfab1107
  • Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.


This message was auto-generated by https://gitcop.com

@fjl
Copy link
Copy Markdown
Contributor

fjl commented Jan 12, 2017

Please rebase on master to remove the ethdb commit.

@GitCop
Copy link
Copy Markdown

GitCop commented Jan 12, 2017

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

  • Commit: 303da33
  • Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.


This message was auto-generated by https://gitcop.com

@fjl
Copy link
Copy Markdown
Contributor

fjl commented Jan 13, 2017

@Arachnid tests are broken

@GitCop
Copy link
Copy Markdown

GitCop commented Jan 13, 2017

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

  • Commit: 303da33
  • Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.


This message was auto-generated by https://gitcop.com

@karalabe karalabe modified the milestones: 1.5.8, 1.5.7 Jan 16, 2017
@fjl fjl merged commit 17d9223 into ethereum:master Jan 17, 2017
@fjl fjl removed the review label Jan 17, 2017
farazdagi pushed a commit to status-im/go-ethereum that referenced this pull request Feb 23, 2017
UlysseCorbeil pushed a commit to streamingfast/go-ethereum that referenced this pull request Mar 13, 2026
* fix: fix nodereal builder address

* fix: fix puissant builder address

* fix: fix comment
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.

6 participants