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.

Sign up for GitHub

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

Block, Tx: Caching for hash method #1445

Merged
merged 16 commits into from
Sep 8, 2021
Merged

Block, Tx: Caching for hash method #1445

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3650857
block: caching for hash method
emersonmacro Sep 1, 2021
8a274bd
tx: caching for hash method
emersonmacro Sep 1, 2021
4203861
block, tx: updates to caching for hash method
emersonmacro Sep 1, 2021
a1e33ac
block, tx: updates to caching for hash method
emersonmacro Sep 1, 2021
837830b
Merge branch 'master' into block-tx-cache-hash
emersonmacro Sep 1, 2021
37073bf
block: refactor caching mechanism in block header
emersonmacro Sep 5, 2021
3c7912b
block: lint fixes
emersonmacro Sep 5, 2021
e5b56cc
Merge branch 'block-tx-cache-hash' of github1:ethereumjs/ethereumjs-m…
emersonmacro Sep 5, 2021
2039403
Merge branch 'master' into block-tx-cache-hash
emersonmacro Sep 5, 2021
b44210e
block: hash caching refactor
emersonmacro Sep 7, 2021
ccda170
tx: hash caching refactor
emersonmacro Sep 7, 2021
f8bf704
Merge branch 'master' into block-tx-cache-hash
emersonmacro Sep 7, 2021
bec1cff
tx: test for hash throwing on unsigned tx
emersonmacro Sep 8, 2021
95ac4d7
Merge branch 'master' into block-tx-cache-hash
emersonmacro Sep 8, 2021
296c25c
tx: updated tests for hash caching
emersonmacro Sep 8, 2021
2b8eceb
tx: revert throwing when hash() is called on unsigned legacy txs due …
holgerd77 Sep 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/block/src/header.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ export class BlockHeader {
public readonly _common: Common
public _errorPostfix = ''

private readonly _cachedHash?: Buffer

/**
* Static constructor to create a block header from a header data dictionary
*
Expand Down Expand Up @@ -295,6 +297,7 @@ export class BlockHeader {

const freeze = options?.freeze ?? true
if (freeze) {
this._cachedHash = this.hash()
Copy link
Member

Choose a reason for hiding this comment

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

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

Object.freeze(this)
}
}
Expand Down Expand Up @@ -725,6 +728,10 @@ export class BlockHeader {
* Returns the hash of the block header.
*/
hash(): Buffer {
if (this._cachedHash) {
return this._cachedHash
}

return rlphash(this.raw())
}

Expand Down
9 changes: 9 additions & 0 deletions packages/tx/src/eip1559Transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ export default class FeeMarketEIP1559Transaction extends BaseTransaction<FeeMark

public readonly common: Common

private readonly _cachedHash?: Buffer

/**
* The default HF if the tx type is active on that HF
* or the first greater HF where the tx is active.
Expand Down Expand Up @@ -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()
}
Copy link
Member

Choose a reason for hiding this comment

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

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. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good - will give it a try

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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!

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that looks good, ready to proceed! 😄

Object.freeze(this)
}
}
Expand Down Expand Up @@ -330,6 +335,10 @@ export default class FeeMarketEIP1559Transaction extends BaseTransaction<FeeMark
throw new Error('Cannot call hash method if transaction is not signed')
}

if (this._cachedHash) {
return this._cachedHash
}

return keccak256(this.serialize())
}

Expand Down
9 changes: 9 additions & 0 deletions packages/tx/src/eip2930Transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ export default class AccessListEIP2930Transaction extends BaseTransaction<Access

public readonly common: Common

private readonly _cachedHash?: Buffer

/**
* The default HF if the tx type is active on that HF
* or the first greater HF where the tx is active.
Expand Down Expand Up @@ -202,6 +204,9 @@ export default class AccessListEIP2930Transaction extends BaseTransaction<Access

const freeze = opts?.freeze ?? true
if (freeze) {
if (this.isSigned()) {
this._cachedHash = this.hash()
}
Object.freeze(this)
}
}
Expand Down Expand Up @@ -300,6 +305,10 @@ export default class AccessListEIP2930Transaction extends BaseTransaction<Access
throw new Error('Cannot call hash method if transaction is not signed')
}

if (this._cachedHash) {
return this._cachedHash
}

return keccak256(this.serialize())
}

Expand Down
13 changes: 13 additions & 0 deletions packages/tx/src/legacyTransaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ export default class Transaction extends BaseTransaction<Transaction> {

public readonly common: Common

private readonly _cachedHash?: Buffer

/**
* Instantiate a transaction from a data dictionary.
*
Expand Down Expand Up @@ -129,6 +131,9 @@ export default class Transaction extends BaseTransaction<Transaction> {

const freeze = opts?.freeze ?? true
if (freeze) {
if (this.isSigned()) {
this._cachedHash = this.hash()
}
Object.freeze(this)
}
}
Expand Down Expand Up @@ -232,6 +237,14 @@ export default class Transaction extends BaseTransaction<Transaction> {
* Use {@link Transaction.getMessageToSign} to get a tx hash for the purpose of signing.
*/
hash(): Buffer {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

if (!this.isSigned()) {
throw new Error('Cannot call hash method if transaction is not signed')
}
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.


if (this._cachedHash) {
return this._cachedHash
}

return rlphash(this.raw())
}

Expand Down