core, eth, les, light: avoid storing computable receipt metadata

[Matthalp-zz]

These changes omit storing the transaction hash (TxHash), gas
cost (GasCost), and contract address (ContractAddress) fields
within its corresponding the transaction receipt. The big wins come
from not storing the transaction hash and contract address since it is
not amenable to the snappy compression used by LevelDB, as
@karalabe and @rjl493456442 have pointed out.

The storage savings comes at the expense of additional processing
time to load receipts from storage. Specifically this comes from
additional reads needed to load the receipts' corresponding block
body to populate some fields as well as the chain configuration
needed to compute the contract address for contract creation
transactions.

However, this performance overhead should not affect the majority
of go-ethereum users. The two main places this read is needed is
for (1) subscribers watching block data during reorganizations and
(2) users querying JSON-RPC for transaction receipts. Substantial
block reorganizations do not happen often enough for (1) to be
significantly affected and while there are some use cases that
heavily use (2), the overheads should not be too horrible even when
traversing the entire historical chain.

Note that there here are some situations where all of the receipt
metadata is not populated, such as in light clients. The new code
supports this behavior by not failing when all of the metadata
cannot be constructed.

[Matthalp-zz]

@karalabe @holiman I did the bare minimum to exclude these fields. It appears to be able to sync just fine through block 1M, so figured we could run it on the benchmarking systems and see what happens. If things look favorable, I'll move on to the complete integration (and making all the tests that now fail actually pass).

[fjl]

I think this won't do much for storage size because the TxHash will still be stored. The diff so far just makes it all-zero. Would be better to delete those fields from receiptStorageRLP.

[Matthalp-zz]

@fjl This actually still reduces the storage size because a singe byte (the RLP empty string representation) is stored instead of the 32 additional bytes for the actual hash (and variable number of bytes for the gas cost). This was done as a "quick an dirty" test to see what the savings will be. If the savings is seen to be considerable then I'll put forth the effort to do this the right way.

[holiman]

@fjl nope, @matthalp is correct! It's not quite finished, but 138G vs 149G.

[Matthalp-zz]

I want to compare eth_getTransactionReceipt for every transaction on the canonical chain against an unmodified client before giving the go ahead to review.

[Matthalp-zz]

@zsfelfoldi It looks like the light protocol complicates this change a bit. I plan to debug this last test case tomorrow, but would appreciate any feedback you have.

[karalabe]

FYI

[Matthalp-zz]

@karable Interpolating the y-axis of the graph looks like a savings of about 12 GB. Is that right?

[karalabe]

Ah, sorry, didn't leave the markers in. The saving it ~14GB.

[karalabe]

Hey all, so I've pushed a followup PR (figured it was easier than to go back and forth for a number of days for nitpicks cleanups). In essence, I've made the following fixes:

• Originally in this PR, reading the receipts via rawdb and deriving the computed fields also entailed accessing the canonical hash of the genesis block and the chain configs from leveldb. These were pretty pointless data to access, since almost all our code is aware of the chain config, so I modified the code to pass this as an argument, not pull it from disk.
• Deriving the computed fields (originally SetReceiptsData, renamed to Receipts.DeriveFields) was moved from rawdb to types at Gary's suggestion, since really it's not a data access functionality, rather tightly coupled to the receipts. I've also changed the signature to use only types.Transactions, not a full types.Body + I attached it to the types.Receipts instead of a dangling method.
• There was an accidental bug introduced in the PR: when an empty block's receipts were read, the original code on master returned an empty slice, the new code returned nil. This is wrong, because nil represents the concept of missing receipts, whereas the empty one obviously represents receipts being available, but no transactions in the block. This cause les to fail some test. Fixed.
• The fakepeer in downloader used the old ReadReceipts to deliver full receipts, but AFAIK we should only deliver consensus receipts there. Fixed to ReadRawReceipts, but we should test that it works correctly.

[karalabe]

This PR seems kind of good to me now. Would be nice to have some more eyes on it before merging.

[rjl493456442]

LGTM

[Matthalp-zz]

Thanks @karalabe and @rjl493456442 ! The follow up commits make sense to me.

I just noticed DeriveFields was not setting receipt bloom filters, so I added a follow up commit on that.

There was an accidental bug introduced in the PR: when an empty block's receipts were read, the original code on master returned an empty slice, the new code returned nil. This is wrong, because nil represents the concept of missing receipts, whereas the empty one obviously represents receipts being available, but no transactions in the block.

@karalabe Thank you for catching this. As an aside, it's more idiomatic go to use nil slices instead of empty slices, but I understand the signaling problem here. I feel like this will not be the last time this kind of issue/bug comes up on the code base, so I would recommend modifying these methods to return two return values: ReadFoo() (<Foo>, bool) and then doing a check on the bool to know if this exists. I'm more than happy to do this.

[karalabe]

Aren't the blooms already handled during decoding?

[karalabe]

As far as I see it those are already covered.

[Matthalp-zz]

@karalabe You are right -- my bad. I hadn't had my morning ☕️ yet.

This does raise the question: where should we be computing the bloom? It's kind of weird to be computing these fields in different places, but at the same time the bloom filter is immediately computable when decoding and the overhead is negligible. What would you prefer?

[karalabe]

Bloom seems fit in decoding because it's consensus. Safer not to screw up.

…

On Mon, Apr 15, 2019, 19:16 Matthew Halpern ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In core/types/receipt.go <#19345 (comment)> : > @@ -310,6 +310,8 @@ func (r Receipts) DeriveFields(config *params.ChainConfig, hash common.Hash, num r[i].BlockNumber = new(big.Int).SetUint64(number) r[i].TransactionIndex = uint(i) + r[i].Bloom = CreateBloom(Receipts{r[i]}) @karalabe <https://github.com/karalabe> You are right -- my bad. I hadn't had my morning ☕️ yet. This does raise the question: where should we be computing the bloom? It's kind of weird to be computing these fields in different places, but at the same time the bloom filter is immediately computable when decoding and the overhead is negligible. What would you prefer? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19345 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAH6GeBkozRg9P2daXF5300c-tsa3K8Kks5vhKX0gaJpZM4cONof> .

[Matthalp-zz]

@karalabe Just pushed a commit to revert the bloom population in Receipt.DeriveFields and its associated test logic.

[karalabe]

🎉