fix splitting of transaction nonce groups in state #11103
Conversation
brad-decker
force-pushed
the
fix-splitting-of-transaction-groups
branch
from
8d885f5 to
531a804
Compare
Builds ready [531a804]
Page Load Metrics (587 ± 47 ms)
|
| this._deleteTransaction(transactions[index].id); | ||
| } | ||
| } | ||
| const nonceNetworkSet = new Set(); |
There was a problem hiding this comment.
I want to make sure I understand this.
So the goal is to ensure that if there are two or more transactions with the same nonce in the transaction list, that they don't get deleted.
So we iterate through the transactions. If a transaction is either counted as below txHistoryLimit - 1 or is not in a final state, then it is NOT deleted. If a transaction is counted higher than txHistoryLimit - 1 or is in a final state BUT has the same nonce as a transaction already flagged as one that shouldn't be deleted, then it is NOT deleted. If a transaction is above the txHistoryLimit count or is in a final state and does not match the nonce of a transaction already identified as one that shouldn't be deleted, then it is deleted...
So something about this isn't adding up for me... probably me misunderstanding, but I want to check.
Suppose that there are only 10 transactions and they are all "final states". Wouldn't they all be deleted according to this logic?
Should the if (txCount > txHistoryLimit - 1) { that was removed above wrap this tx deletion code?
And this is in a function that only adds a single tx. Before it only removed a single tx. Why now attempt to remove multiple within this function?
There was a problem hiding this comment.
maybe I forget how filter works...
There was a problem hiding this comment.
Based on my reading of the tests I think I did misunderstand the goal though
There was a problem hiding this comment.
Can the purpose of this code change be stated as:
If the addition of a transaction brings the transaction count above the txHistoryLimit then all transactions that share a nonce with the oldest "finalized" transaction should be deleted.
?
There was a problem hiding this comment.
So the goal is to ensure that if there are two or more transactions with the same nonce in the transaction list, that they don't get deleted.
It's to ensure that one transaction in a group isn't deleted without deleting the entire group.
We can't let all groups of more than 1 transaction persist indefinitely - then we'd have no limit on storage. But we can't break groups apart either without breaking the UI.
So the solution we went with here was to interpret the transaction history limit of 50 as meaning 50 groups of transactions. If there are over 50 groups, we delete any groups beyond that 50 (which should always be a single group if this is the only means of adding transactions, but might be more than 1 group if transactions are added elsewhere).
There was a problem hiding this comment.
when we return false from this method we are Not including it, when we return true we are including it. So when return true we are saying "this is an item we want to delete". if there are only ten transactions non will be deleted because they will never hit the limit and thus will all be added to the nonceSet
There was a problem hiding this comment.
basically yes as stated that will work
| // following cancel transaction with the same nonce. these two transactions should be dropped | ||
| // together as soon as the 11th unique nonce is attempted to be added, mocked here with limit + 1 | ||
| const txs = generateTransactions(limit + 1, { | ||
| chainId: currentChainId, |
There was a problem hiding this comment.
should the nonce method param be defined here, to ensure more than once transaction of the same nonce is in the test?
There was a problem hiding this comment.
Yes! must have dropped during code cleanup
brad-decker
force-pushed
the
fix-splitting-of-transaction-groups
branch
from
531a804 to
d6c9039
Compare
Builds ready [d6c9039]
Page Load Metrics (588 ± 51 ms)
|
|
This looks good to me. One other non-blocking improvement I could suggest: add another test case for when there are transactions with the same nonce, but the number of generated transactions is less than the limit, and verify that entire groups of transactions are not cut off. |
|
@danjm added and also updated the test above it to be more specific because the last checks could have applied to either the first group of transactions or the second and the test was specifically checking that the first group was cut off and the second was not. |
Builds ready [f6f222d]
Page Load Metrics (549 ± 46 ms)
|
Co-authored-by: Mark Stacey <[email protected]>
Builds ready [8f60118]
Page Load Metrics (667 ± 48 ms)
|
brad-decker
force-pushed
the
fix-splitting-of-transaction-groups
branch
from
01ffeb0 to
284c1b5
Compare
Builds ready [284c1b5]
Page Load Metrics (601 ± 51 ms)
|
Fixes: #11069
Explanation:
The tx-state-manager has always deleted the oldest "finalized" transaction from the state after the limit (default is 50), this is erroneous and can lead to UI issues when you split a cancel transaction away from the transaction that it was canceling and other various edge cases.
This change makes sure we remove entire groups of transactions after we hit a limit. This will result in a number greater than 50 transactions being possible, if large groups of transactions occur. Grouping is done by nonce by network, but only the
${limit}most recent unique nonces are considered.