Common deep copy refactor #3608
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
| Benchmark suite | Current: 4010916 | Previous: 9856f66 | Ratio |
|---|---|---|---|
Block 9422905 |
2207 ops/sec (±1.09%) |
37877 ops/sec (±1.73%) |
17.16 |
Block 9422906 |
2223 ops/sec (±0.59%) |
35937 ops/sec (±3.35%) |
16.17 |
Block 9422907 |
2203 ops/sec (±0.78%) |
36656 ops/sec (±1.46%) |
16.64 |
Block 9422908 |
2170 ops/sec (±0.74%) |
35733 ops/sec (±1.60%) |
16.47 |
Block 9422910 |
2201 ops/sec (±0.47%) |
34971 ops/sec (±1.78%) |
15.89 |
This comment was automatically generated by workflow using github-action-benchmark.
| customCrypto: this.customCrypto, | ||
| }) | ||
| copy.updateParams(this._params) | ||
| copy.setHardfork(this.hardfork()) |
There was a problem hiding this comment.
This code has never been touched by any refactor, I am really not seeing why we should change this now.
Also, side note, the spread operator ... has issues with nested copying, see e.g. this example:
That's why spread is generally not so suitable.
Also to note: side adding setHardfork() can have massive side effects here, I remember similar experiments in the past.
packages/common/test/chains.spec.ts
Outdated
| await setCommon() | ||
| await resetCommon() | ||
| }) | ||
|
|
There was a problem hiding this comment.
I am honestly very confused by this whole setup, so these tests trigger the issue but were later removed?? 😂
I will copy (cherry-pick) them over in a new PR and add the changes I initially intended.
There was a problem hiding this comment.
(and see if tests are passing)
packages/common/src/common.ts
Outdated
| @@ -69,7 +71,7 @@ export class Common { | |||
| (this._chainParams.customHardforks && this._chainParams.customHardforks[hf.name]), | |||
| ]) | |||
| this._hardfork = this.DEFAULT_HARDFORK | |||
| this._params = { ...(opts.params ?? {}) } // copy | |||
| this._params = opts.params ? JSON.parse(JSON.stringify(opts.params)) : {} // copy | |||
There was a problem hiding this comment.
Ok, so this is actually the single line change I initially intended, likely with the ones from updateParams() and resetParams() (good catch!), no changes intended for copy() whatsoever.
Will tests these lines in a new PR (maybe again, eventually you've been to this point already), then this should already be enough for this (or a replacing) PR!
(we also should not mix this with #3593 , it's absolutely not clear that these are related)
packages/common/src/common.ts
Outdated
| @@ -58,6 +59,7 @@ export class Common { | |||
| public events: EventEmitter | |||
|
|
|||
| constructor(opts: CommonOpts) { | |||
| this._opts = opts | |||
| this.events = new EventEmitter() | |||
|
|
|||
| this._chainParams = opts.chain | |||
There was a problem hiding this comment.
Ah, but you are right, we need additional treatment for chain. But we should do here, then we can avoid doing any copy() changes.
Also (more important): if we do not here, then the initial Mainnet (or whatever chain) structure gets modified in the first place. That's bad and we should very much avoid (same for the params structs, I stumbled upon this kind of failure).
packages/common/test/chains.spec.ts
Outdated
| let chainConfig: ChainConfig | ||
| let c: Common | ||
| const setCommon = async () => { | ||
| chainConfig = Mainnet |
There was a problem hiding this comment.
Haha, so funny, I am pretty sure (or: could very much imagine) that this is where you turned wrong: so, by associating chainConfig = Mainnet here and then manipulating this further down the line with chainConfig.chainId = 2 this then also manipulates Mainnet itself.
And that causes another test below in line 55 to fail. 😂 And the error looks pretty much like a "still failing" message if one is not looking pretty closely. 🙂
|
Have replaced with #3613, will close here. Ok, but this work was definitely not "wasted", since this discovered various additional necessary deep-copy cases + could re-use the test setup! 🙏 |
This change refactors the
Commonclass to deep copy parameters on calls tocopy. It aims to address issue #3593 and #3572.