tx: add getDataFee to tx cache #1532
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
| /** | ||
| * The amount of gas paid for the data in this tx | ||
| */ | ||
| getDataFee(): BN { |
There was a problem hiding this comment.
do we need to add this? would it not work falling back to the default baseTransaction method?
There was a problem hiding this comment.
It's a little weird because the cache is created in baseTransaction, but caching doesn't actually happen in baseTransaction because it doesn't have the option to be frozen. The freeze option only exists on the derived classes. That's my reading of the code but I might be misinterpreting it
There was a problem hiding this comment.
hm, I think Object.isFrozen(this) would still work in the baseTransaction, but would have to try it out
There was a problem hiding this comment.
Ah right, just remembered, my first attempt was to implement the caching in baseTransaction, but the issue I ran into was that eip1559Transaction and eip2930Transaction both call super.getDataFee() but then modify the value in their own implementations of getDataFee. But the value of getDataFee was getting calculated and cached at the baseTransaction level, so the incorrect value was being returned in the derived classes.
There was a problem hiding this comment.
Ok, guess that makes sense.
Can you structurally align this method with the other two methods? So with this cost constant and generally the same order of calling? That would be easier to read and compare.
|
|
||
| if (Object.isFrozen(this) && !this.cache.dataFee) { | ||
| this.cache.dataFee = cost | ||
| } |
There was a problem hiding this comment.
On the first if condition you can remove the Object.isFrozen(this) check and on the second the this.cache.dataFee check. Guess this reads a bit better and is less redundant, also for the other tx types.
There was a problem hiding this comment.
Good call - updated and it does read better
|
This invalidates the tx object in case that Common changes (i.e. tx is now in a different hardfork, for instance changing the fee to pay per 0-byte in the calldata or nonzero-byte in the calldata). Doesn't this lead to undesired side-effects? This can be solved by also caching the current common setting. In case that common has changed (network, hardfork) then the cache is invalid and we should thus re-calculate. |
|
@jochem-brouwer hmm, is this practically relevant, a frozen transaction where the Common changes after instantiation? 🤔 |
|
The (Note: this race condition does most likely not happen in practice since the created blocks will re-import the transactions. But it is to illustrate that this could report wrong values of the data fee and thus cause race conditions, especially at fork transitions) |
|
@jochem-brouwer Ok, guess you are right and this might have side effect. I think we can just listend to the hardforkChanged event here from Common and then invalidate (delete) the data fee cache? 🤔 |
|
@jochem-brouwer @holgerd77 Thanks for the feedback - added the listener for Also concerned with the performance alert that popped up from the latest change. Did the addition of the event listener cause that much performance degradation? |
|
|
||
| this.common.on('hardforkChanged', () => { | ||
| delete this.cache.dataFee | ||
| }) |
There was a problem hiding this comment.
Ah, this should be not only for the legacy tx but for all tx types. So I guess the BaseTransaction constructor is a better place here.
There was a problem hiding this comment.
I tried moving it to the BaseTransaction constructor, but this.common wasn't defined yet so it threw an error. I just added it to the other transaction types - not very DRY but it seems to be working. If there's a way to get it working in BaseTransaction, I can update it
|
@emersonmacro ah, and I guess you can ignore these performance alerts for now, this doesn't seem to work atm. |
| @@ -168,6 +171,17 @@ tape('[Transaction]', function (t) { | |||
| st.end() | |||
| }) | |||
|
|
|||
| t.test('getDataFee() -> should invalidate cached value on hardfork change', function (st) { | |||
| const common = new Common({ chain: Chain.Mainnet, hardfork: Hardfork.Byzantium }) | |||
| const tx = Transaction.fromValuesArray(txFixtures[0].raw.map(toBuffer), { | |||
There was a problem hiding this comment.
For completeness, this test should use all Tx types, so this test should also run with EIP1559/EIP2930 transactions (which will probably fail right now as pointed out by @holgerd77 in another comment)
Partially addresses #775 - adds caching to
Transaction#getDataFeemethod