Block, Tx: Caching for hash method #1445

emersonmacro · 2021-09-01T02:46:32Z

Addresses a couple items from #775, specifically "Transaction#hash should be cached" and "BlockHeader#hash should be cached".

packages/block/src/header.ts

emersonmacro · 2021-09-01T03:12:27Z

packages/tx/src/legacyTransaction.ts

@@ -232,6 +235,9 @@ export default class Transaction extends BaseTransaction<Transaction> {
   * Use {@link Transaction.getMessageToSign} to get a tx hash for the purpose of signing.
   */
  hash(): Buffer {


@holgerd77 One thing I noticed while working on this... The comment for the hash method here says it should throw if not signed (https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/tx/src/legacyTransaction.ts#L231)... but it's not actually checking?

Contrast with the other transaction types, which both have an explicit check:
https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/tx/src/eip2930Transaction.ts#L299
https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/tx/src/eip1559Transaction.ts#L329

It's definitely possible I'm not understanding fully, but wanted to surface the question in case it is missing the isSigned check here.

good catch, seems like it should be added to legacy.

codecov · 2021-09-01T03:52:22Z

Codecov Report

Merging #1445 (2b8eceb) into master (2e8dff0) will decrease coverage by 0.03%.
The diff coverage is 77.77%.

Flag	Coverage Δ
block	`86.75% <100.00%> (-0.02%)`	⬇️
blockchain	`83.49% <ø> (-0.07%)`	⬇️
client	`83.00% <ø> (-0.02%)`	⬇️
common	`94.39% <ø> (+0.19%)`	⬆️
devp2p	`82.59% <ø> (ø)`
ethash	`86.56% <ø> (ø)`
tx	`87.34% <69.23%> (-0.90%)`	⬇️
vm	`79.32% <ø> (ø)`

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

holgerd77

Hi there, one bigger structural change request.

Additionally: can you please also add a comment to the freeze option comment for tx and blocks on the caching? Something like "...parameters. It also enables tx hash caching when the hash() function is called several times." (feel free to improve on this). 😄

holgerd77 · 2021-09-02T07:45:56Z

packages/tx/src/eip1559Transaction.ts

@@ -228,6 +230,9 @@ export default class FeeMarketEIP1559Transaction extends BaseTransaction<FeeMark

    const freeze = opts?.freeze ?? true
    if (freeze) {
+      if (this.isSigned()) {
+        this._cachedHash = this.hash()
+      }


Hi there, thanks for the PR! 😄

The cache creation for the hash shouldn't be done in the constructor but in the hash() function itself on the first execution (same for Block). Otherwise we are actually adding overhead for use cases where thehash() function is not needed.

If your problem is that the object actually will get frozen after the constructor is called:

I did some experiments on this and I guess it should work to wrap the _cachedHash property into some object, which will then still be modifiable. See the following results:

> const inner = { hash: '123' } 'use strict' > const outer = { inner, test: 5 } undefined > outer.test = 4 4 > outer { inner: { hash: '123' }, test: 4 } > Object.freeze(outer) { inner: { hash: '123' }, test: 4 } > outer.test = 3 3 > outer { inner: { hash: '123' }, test: 4 } > outer.inner.hash = '345' '345' > outer { inner: { hash: '345' }, test: 4 }

So as one can see the inner object hash property can still be modified on a frozen object (maybe someone else should nevertheless have an additional look here to see if this has no side effects whatsoever. Can't think of any thow atm).

Maybe choose some structure like:

private cache = { hash: undefined }

Then we can eventually expand this with other stuff. 😄

Sounds good - will give it a try

@holgerd77 I have an implementation in packages/block/src/header.ts based on your suggestion - tests are passing and I think it makes sense. Wanted to get your feedback on the approach, and if it looks good, I'll add it in tx and add the documentation comments. Thanks!

Yes, that looks good, ready to proceed! 😄

holgerd77 · 2021-09-02T07:46:29Z

packages/block/src/header.ts

@@ -295,6 +297,7 @@ export class BlockHeader {

    const freeze = options?.freeze ?? true
    if (freeze) {
+      this._cachedHash = this.hash()


-> to hash() function (see tx comment).

…onorepo into block-tx-cache-hash

packages/block/src/types.ts

holgerd77

Hi there, one small change needed.

Can you also make sure that the test coverage is above the threshold? I guess this mainly means that - if we decide to keep this new throw-clause for the legacy tx, you would need to add one test within the legacy.spec.ts file (there is an existing test on this in the typedTxsAndEIP2930.spec.ts test file for reference).

packages/tx/src/baseTransaction.ts

holgerd77 · 2021-09-07T08:37:07Z

packages/tx/src/legacyTransaction.ts

@@ -232,6 +232,17 @@ export default class Transaction extends BaseTransaction<Transaction> {
   * Use {@link Transaction.getMessageToSign} to get a tx hash for the purpose of signing.
   */
  hash(): Buffer {
+    if (!this.isSigned()) {
+      throw new Error('Cannot call hash method if transaction is not signed')
+    }


Maybe I am getting paranoid, but I am getting a bit shaky here and wondering if we did have some reason to not throw here yet and rather just add to the new typed txs for some weird backwards compatibility reason.

CC-ing @jochem-brouwer in here, was just seeing that you wrote a lot around the whole topic in your typed tx implementation #1048. Can you eventually remember something on this? (if not we should really do here and give this a final go)

I do remember that I wanted to implement that logic first, but that broke things for some reason which is why we did not edit it. (It is a breaking change also, I guess?). I'll take a look this afternoon to figure out what it was.

The thing that prompted me to add it to this PR was this comment - https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/tx/src/legacyTransaction.ts#L231. I noticed the typed txs had an explicit check but the legacy tx didn't, so I thought it might just have been missed. But maybe the comment was copied over and it wasn't meant to be implemented. Happy to take out that change if that's the case.

@jochem-brouwer Ok, thanks for the clarification. I guess this is to risky then to add on the side line here, I've reverted the change respectively replaced with a longer comment so that no future devs will stumble upon this again.

Will also add a note to the breaking changes TODO so that we will not forget on next breaking tx release.

emersonmacro · 2021-09-08T04:55:15Z

Hi there, one small change needed.

Can you also make sure that the test coverage is above the threshold? I guess this mainly means that - if we decide to keep this new throw-clause for the legacy tx, you would need to add one test within the legacy.spec.ts file (there is an existing test on this in the typedTxsAndEIP2930.spec.ts test file for reference).

Added a test case to cover the throw clause, but it's still saying the test coverage is below threshold? Did I miss something? I tried to check the Codecov report but was getting 403 API rate limit messages.

Nevermind, found the issue. Because we were always using the default tx options, the tx was always frozen, so we were always returning the cached value. Added some cases where we test unfrozen txs.

…to eventual compatibility issues, added longer explanatory comment

holgerd77

LGTM, great work Emmett! 😄

emersonmacro added 2 commits August 31, 2021 19:36

block: caching for hash method

3650857

tx: caching for hash method

8a274bd

emersonmacro added PR state: WIP package: block package: tx labels Sep 1, 2021

emersonmacro requested a review from holgerd77 September 1, 2021 02:46

ryanio reviewed Sep 1, 2021

View reviewed changes

packages/block/src/header.ts Outdated Show resolved Hide resolved

emersonmacro commented Sep 1, 2021

View reviewed changes

emersonmacro added 2 commits August 31, 2021 20:22

block, tx: updates to caching for hash method

4203861

block, tx: updates to caching for hash method

a1e33ac

emersonmacro added PR state: needs review and removed PR state: WIP labels Sep 1, 2021

Merge branch 'master' into block-tx-cache-hash

837830b

holgerd77 changed the title ~~Block, Tx: Caching for hash method~~ Tx: Caching for hash method Sep 2, 2021

holgerd77 changed the title ~~Tx: Caching for hash method~~ Block, Tx: Caching for hash method Sep 2, 2021

holgerd77 requested changes Sep 2, 2021

View reviewed changes

emersonmacro added PR state: WIP and removed PR state: needs review labels Sep 4, 2021

emersonmacro and others added 4 commits September 5, 2021 09:29

block: refactor caching mechanism in block header

37073bf

block: lint fixes

3c7912b

Merge branch 'block-tx-cache-hash' of github1:ethereumjs/ethereumjs-m…

e5b56cc

…onorepo into block-tx-cache-hash

Merge branch 'master' into block-tx-cache-hash

2039403

holgerd77 reviewed Sep 6, 2021

View reviewed changes

packages/block/src/types.ts Outdated Show resolved Hide resolved

emersonmacro and others added 3 commits September 6, 2021 21:03

block: hash caching refactor

b44210e

tx: hash caching refactor

ccda170

Merge branch 'master' into block-tx-cache-hash

f8bf704

emersonmacro added PR state: needs review and removed PR state: WIP labels Sep 7, 2021

holgerd77 requested changes Sep 7, 2021

View reviewed changes

emersonmacro and others added 2 commits September 7, 2021 21:18

tx: test for hash throwing on unsigned tx

bec1cff

Merge branch 'master' into block-tx-cache-hash

95ac4d7

emersonmacro and others added 2 commits September 7, 2021 22:50

tx: updated tests for hash caching

296c25c

tx: revert throwing when hash() is called on unsigned legacy txs due …

2b8eceb

…to eventual compatibility issues, added longer explanatory comment

holgerd77 mentioned this pull request Sep 8, 2021

Breaking Releases - Meta Issue (VM v6, others) (old) #1024

Closed

50 tasks

holgerd77 approved these changes Sep 8, 2021

View reviewed changes

holgerd77 merged commit 46797fe into master Sep 8, 2021

holgerd77 deleted the block-tx-cache-hash branch September 8, 2021 09:37

holgerd77 mentioned this pull request Feb 16, 2022

v6 Breaking Release Planning #1717

Closed

51 tasks

holgerd77 mentioned this pull request May 17, 2022

Tx: Throw when hash() is called on unsigned legacy txs #1890

Closed

ScottyPoi mentioned this pull request May 18, 2022

Tx: Throw when hash() is called on unsigned legacy transaction #1894

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block, Tx: Caching for hash method #1445

Block, Tx: Caching for hash method #1445

emersonmacro commented Sep 1, 2021

emersonmacro Sep 1, 2021

ryanio Sep 1, 2021

codecov bot commented Sep 1, 2021 •

edited

Loading

holgerd77 left a comment

holgerd77 Sep 2, 2021

emersonmacro Sep 4, 2021

emersonmacro Sep 5, 2021

holgerd77 Sep 6, 2021

holgerd77 Sep 2, 2021

holgerd77 left a comment

holgerd77 Sep 7, 2021

jochem-brouwer Sep 7, 2021

emersonmacro Sep 8, 2021

holgerd77 Sep 8, 2021

emersonmacro commented Sep 8, 2021 •

edited

Loading

holgerd77 left a comment

Block, Tx: Caching for hash method #1445

Block, Tx: Caching for hash method #1445

Conversation

emersonmacro commented Sep 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 1, 2021 • edited Loading

Codecov Report

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emersonmacro commented Sep 8, 2021 • edited Loading

holgerd77 left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 1, 2021 •

edited

Loading

emersonmacro commented Sep 8, 2021 •

edited

Loading