Common: Remove Parameters #3537
Conversation
…override the default parameter set
…cloning/serialization problems
|
This is now ready for review, added a description in the initial post section. |
| if (!(eip in this._params)) { | ||
| this._params[eip] = { ...paramsConfig } // copy | ||
| } else { | ||
| this._params[eip] = { ...this._params[eip], ...params[eip] } |
There was a problem hiding this comment.
Does / should this deep copy?
There was a problem hiding this comment.
Ah, yes, likely, thanks for catching. I actually thought this would (and I still think it does to some extend, but maybe not on the second level dicts? 🤔) But then I re-stumbled upon this in the test writing, where I tried the same and it actually didn’t.
I will keep this here to save CI but do in subsequent PR today (also about Common).
There was a problem hiding this comment.
That's fine! I have addressed three other places where the shallow copy seems to happen with comments.
This was a long thought process, and not a pop-up idea out of nowhere, by the way! 😂 |
| @@ -75,6 +73,8 @@ export class Common { | |||
| (this._chainParams.customHardforks && this._chainParams.customHardforks[hf.name]), | |||
| ]) | |||
| this._hardfork = this.DEFAULT_HARDFORK | |||
| this._params = { ...(opts.params ?? {}) } // copy | |||
There was a problem hiding this comment.
If the shallow copy should be removed, then this one should likely also be addressed 😄
There was a problem hiding this comment.
Oh yes, good point! Thanks!
| * @param params | ||
| */ | ||
| resetParams(params: ParamsDict) { | ||
| this._params = { ...params } // copy |
| this._paramsCache = { | ||
| ...this._paramsCache, | ||
| ...params['params'], | ||
| ...params, |
There was a problem hiding this comment.
Here also a possible shallow copy.
There was a problem hiding this comment.
Guess you mean "deep copy" but: yes!
Will all do in subsequent PR (not the state manager one)
THIS is actually the reason I did all this Common refactoring during the last days. 😂
This PR fully removes all parameters from Common and moves them over to the packages into dedicated
params.tsfile. Nevertheless the full Common hardfork/EIP switching logic is preserved with Common still providing the central binding ground for parameter selection, so there is are no API changes for the users whatsoever.Initially this work was mainly motivated by our current tree shaking work. I now find other benefits rather prevailing.
This new structure makes it significantly easier to fully adjust and customize parameter sets for libraries in a robust way along all hardforks, which was previously extremely complicated. I added new
paramsoptions for the affected libraries along (tx, block, evm, vm), see associated test case and/or code docs for usage.There are somewhat significant tree shaking benefits too though.
So this is the
londonTx.tsexample before:This is with the PR:
Benefits are mainly due to the large EVM parameter set not being unnecessarily pulled over to the tx library. This now goes for all libraries though with the new structure, so evm is free-d up from tx, block and vm parameters.
Some additional note:
I had some problems with side effects along plainly using the param dicts as a base for customization (manipulation of the imported dict directly affected the original dictionary itself). I added explicit notes to the docs. Also - to make deep cloning easier - I removed all BigInt usages from params and replaced it with strings. I think this is a general improvement, I had some problems with this already earlier, can't fully remember the context though. Here it is for easier deep cloning, which does not work for BigInts (depending what option you choose). Change has no drawbacks, since there is a
BigInt(param)call in theCommon.param()method anyhow and so there was no logic code change necessary and this rather eliminated this double-BigInt instantiation (which worked).Ready for review! 🙂