[evm] remove corresponding logs when tx is reverted #3014
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3014 +/- ##
==========================================
+ Coverage 64.96% 65.07% +0.11%
==========================================
Files 191 191
Lines 20603 20628 +25
==========================================
+ Hits 13384 13424 +40
+ Misses 5528 5511 -17
- Partials 1691 1693 +2
Continue to review full report at Codecov.
|
| require.Equal(test.logAddr, stateDB.logs[test.logSize-1].Address) | ||
| } else { | ||
| require.Equal(6, len(stateDB.logs)) | ||
| require.Equal("io1x3cv7c4w922k6wx5s8p6d8sjrcqlcfrxhkn5xe", stateDB.logs[5].Address) |
There was a problem hiding this comment.
if revertLog is not enabled, the number of logs stays at 6
| @@ -892,10 +919,16 @@ func (stateDB *StateDBAdapter) clear() { | |||
| stateDB.suicideSnapshot = nil | |||
| stateDB.preimages = nil | |||
| stateDB.preimageSnapshot = nil | |||
| stateDB.logsSnapshot = nil | |||
There was a problem hiding this comment.
Once the memory is unreachable, it will be freed.
No need to set it to nil
There was a problem hiding this comment.
yes, reason for this:
- this is for code clarity (clearly tells reader the variable is not needed anymore), follow the existing code
- in a sense help the runtime/GC to know immediately this memory is no longer needed and can be GC'ed, similar to 90badd6#diff-aa7bcf8c732d6ec1b1b2d3a3324071563c4c913f007f3298bf8f7e1f7c78aaa5R49
There was a problem hiding this comment.
Unlike C++, Go is automatic memory management. The GC won't invoked immediately when the reference is removed.
There was a problem hiding this comment.
right ... again the reasons explained above
| // restore logs | ||
| if stateDB.revertLog { | ||
| stateDB.logs = stateDB.logs[:stateDB.logsSnapshot[snapshot]] | ||
| delete(stateDB.logsSnapshot, snapshot) |
There was a problem hiding this comment.
why do we need map here? Is the distribution of sn sparse?
I think slice is enough
| receipt.AddLogs(stateDB.Logs()...).AddTransactionLogs(depositLog, burnLog) | ||
| if receipt.Status == uint64(iotextypes.ReceiptStatus_Success) || | ||
| featureCtx.AddOutOfGasToTransactionLog && receipt.Status == uint64(iotextypes.ReceiptStatus_ErrCodeStoreOutOfGas) { | ||
| receipt.AddTransactionLogs(stateDB.TransactionLogs()...) | ||
| } | ||
| stateDB.clear() |
There was a problem hiding this comment.
The position change of this line may be a hard fork.
There was a problem hiding this comment.
from my understanding it's not a hard-fork, clear() does not clear Logs and TransactionLogs, which are used in line 246~249, now moving clear() to after line 249 and as a result, these 2 can be cleared as well
it is not a necessary change, just to make clear() a little better, that's why I put it in a 2nd commit. I am fine we don't check this in. Let me know your thoughts
There was a problem hiding this comment.
then, it should be fine.
| receipt.AddLogs(stateDB.Logs()...).AddTransactionLogs(depositLog, burnLog) | ||
| if receipt.Status == uint64(iotextypes.ReceiptStatus_Success) || | ||
| featureCtx.AddOutOfGasToTransactionLog && receipt.Status == uint64(iotextypes.ReceiptStatus_ErrCodeStoreOutOfGas) { | ||
| receipt.AddTransactionLogs(stateDB.TransactionLogs()...) | ||
| } | ||
| stateDB.clear() |
There was a problem hiding this comment.
then, it should be fine.
| @@ -62,11 +62,13 @@ type ( | |||
| suicideSnapshot map[int]deleteAccount // snapshots of suicide accounts | |||
| preimages preimageMap | |||
| preimageSnapshot map[int]preimageMap | |||
| logsSnapshot []int // logs is an array, save len(logs) at time of snapshot suffices | |||
There was a problem hiding this comment.
shall we revert transaction logs as well? if so, it is better to have it fixed in this PR as well.
No description provided.