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

core: remove redundant storage of transactions and receipts #14801

Merged
merged 6 commits into from
Jul 14, 2017

Conversation

karalabe
Copy link
Member

@karalabe karalabe commented Jul 13, 2017

This PR reduces fast sync time by about 30 minutes on my machine from 2:15 to 1:45, and also reduces the overall database size from 26.3GB to 14.9GB.

The change is that currently we're storing each transaction twice, once in the block body, once individually to allow hash lookups. In addition, we're also storing the block metadata (hash, number, index) of each such transaction with its hash. (Similarly, we store each receipt twice, once as a collection for the block, once individually). But:

  • If we have the positional metadata, we have everything to look up the tx (and receipt) already, there's no use storing the data content twice apart from an irrelevant speed boost.
  • We index all this by tx hash, thus scattering it all over the db. Prefixing the lookup entries makes transaction/receipt importing a whole lot faster during sync as it would localizes database changes to a well defined part of disk, not all over.

if blockHash != (common.Hash{}) {
body := GetBody(db, blockHash, blockNumber)
if body == nil || len(body.Transactions) <= int(txIndex) {
log.Error("Transaction refereced missing", "number", blockNumber, "hash", blockHash, "index", txIndex)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/refereced/referenced/

if err = batch.Put(tx.Hash().Bytes(), data); err != nil {
return err
}
// Encode and queue up the transaction metadata for storage
meta := struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you break this out into a top-level struct?

Copy link
Member Author

@karalabe karalabe Jul 13, 2017

Choose a reason for hiding this comment

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

Sure, makes sense if we have a global accessor for it.

if list != nil {
// Retrieve all the receipts belonging to this block and write the loopup table
if _, err := GetBlockReceipts(ctx, pool.odr, hash, number); err != nil { // ODR caches, ignore results
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a quick note: in the long run we'll need ODR versions of GetTransaction and GetReceipt, and when we have those, we can remove this block receipts request here since they will be downloaded on demand if the app needs them. Until we have and ODR request for lookup entries, we can leave it like this.

@karalabe karalabe added this to the 1.7.0 milestone Jul 14, 2017
Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

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

This looks good, but LookupEntry is a very generic name for a transaction index.

Name suggestions:

  • TxIndex
  • TxInclusion
  • InclusionInfo

@karalabe
Copy link
Member Author

It's for both transactions and receipts.

@fjl
Copy link
Contributor

fjl commented Jul 14, 2017

Yes, but the receipt index is always equal to the transaction index.

@zsfelfoldi
Copy link
Contributor

LookupEntry sounds fine for me too but we could also call it TxHashLookup.

@karalabe karalabe merged commit 0ff35e1 into ethereum:master Jul 14, 2017
@sandakersmann
Copy link
Contributor

WOW! That's some major optimizations :)

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.

5 participants