change: improve TX tracking #188
Conversation
Codecov Report
@@ Coverage Diff @@
## main #188 +/- ##
==========================================
+ Coverage 56.86% 58.66% +1.80%
==========================================
Files 17 17
Lines 1224 1200 -24
==========================================
+ Hits 696 704 +8
+ Misses 483 460 -23
+ Partials 45 36 -9
Continue to review full report at Codecov.
|
x/gastracker/keeper/keeper.go
Outdated
| for ; iter.Valid(); iter.Next() { | ||
| v := gastracker.TransactionTracking{} | ||
| k.cdc.MustUnmarshal(iter.Value(), &v) | ||
| log.Printf("%s", &v) |
x/gastracker/ante/ante_test.go
Outdated
| @@ -116,7 +107,7 @@ func TestGasTrackingAnteHandler(t *testing.T) { | |||
| "Gastracking ante handler should not return an error", | |||
| ) | |||
|
|
|||
| currentBlockTrackingInfo, err := keeper.GetCurrentBlockTracking(ctx) | |||
| currentBlockTrackingInfo := keeper.GetCurrentBlockTracking(ctx) | |||
| assert.NoError(t, err, "Current block tracking info should exists") | |||
There was a problem hiding this comment.
Should probably remove this assert
|
|
||
| bytes := store.Get(key) | ||
| if bytes == nil { | ||
| panic(fmt.Errorf("no gas tracking found for tx")) |
There was a problem hiding this comment.
Panics are very dangerous to runtime and health of the process, are we sure we shouldn't just return an error?
There was a problem hiding this comment.
I will comment this, but it must yield a panic
There was a problem hiding this comment.
While tracking new block, you will be iterating through all tx objects and removing them. This iteration as well deletion costs more disk I/O since the data may be at different sectors of disk compared to if entire data is stored in one place.
Better approach here would be to cache the entire block tracking object in memory if possible.
This can be looked and considered as an upgrade, It's hard to predict Disk I/O performance since there are multiple factors at play from DB being used to OS optimizations and Filesystems. If need be we could look into a cache solution but this in itself is likely to yield it's own cost in both technical debt and performance given the behavior of the Golang GC. |
Performance should improve in this PR, before adding one contract operation meant fully decoding the block, its transactions, and each transaction's operations, appending info and then encoding everything back into state. As TX number grows, alongside contract interactions, this would have been extremely more expensive than what I have implemented here. Also, with regards to |
Initially TX tracking plus contracts operations were all saved into a single object.
This brings huge performance limitations and issues.
As to add 1 contract operation, you need to unmarshal one single object containing all the txs (and their respective operations) and then append to it, and then marshal it back again.
In blocks with lots of TXs this gets crazy expensive.
So this PR puts every tx tracking information into its own object.
This could be better improved by putting every single operation into its own object and doing all the connections when the block tracking info is fetched. Leaving this for another PR to reduce the scope.
On top of that it also removes copied test utilities.