fix: l1 handler message hash to txn hash mapping #3367
Conversation
EgeCaner
added
the
disable-deploy-test
EgeCaner
force-pushed
the
fix/L1-handler-message-hash-to-txn-hash-mapping
branch
from
9a9b02a to
c92ac35
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3367 +/- ##
==========================================
- Coverage 76.33% 76.32% -0.02%
==========================================
Files 351 351
Lines 33309 33349 +40
==========================================
+ Hits 25427 25452 +25
- Misses 6070 6080 +10
- Partials 1812 1817 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
EgeCaner
force-pushed
the
fix/L1-handler-message-hash-to-txn-hash-mapping
branch
from
2d955bb to
8c1faac
Compare
EgeCaner
force-pushed
the
fix/L1-handler-message-hash-to-txn-hash-mapping
branch
from
8c1faac to
535f9d3
Compare
thiagodeev
left a comment
thiagodeev
left a comment
There was a problem hiding this comment.
LGTM.
By generating a benchmark with cursor, it seems the approach with a buffered channel is at least 15% faster. It's a one-line change, WDYT about it?
| var writeMu sync.Mutex | ||
| numWorkers := runtime.GOMAXPROCS(0) | ||
| workerPool := pool.New().WithErrors().WithMaxGoroutines(numWorkers) | ||
| batch := database.NewBatch() |
There was a problem hiding this comment.
How long does the migration take. Is it super quick, no need, but if not, what do you think of adding some logging to inform users. I leave it completely at your discretion.
Also, is this operation light enough to work under the 8gb of ram and 4 core minimum requirements of Juno? I think yes, but asking just to be safe. An intuitive answer is good enough in this case in my opinion – let's not put more time into this PR than strictly required
There was a problem hiding this comment.
Migration was taking ~1 min 15 sec in my machine, let me run it in environment similar to minimum requirements
There was a problem hiding this comment.
I think how long it takes also matters because we're writing everything in a single batch. If user kills the process in the middle, all progress will be lost, and user has to suffer the same cost of migrating them again.
There was a problem hiding this comment.
Im not sure if we should allow to cancel. In case of cancel and re-run we would do the all operations again, migration wont start from where it was cancelled. To be able to remain from where we left, we need to track additional state, by taking a look at the table we are writing L1MessageHash to L2 txn hash we cannot tell were we left?
There was a problem hiding this comment.
Is it sepolia or mainnet?
It was Sepolia
There was a problem hiding this comment.
I will add some logs as well, on Mainnet it will take lot more than on sepolia
| blockNumbers := make(chan uint64) | ||
| go func() { | ||
| for blockNum := range chainHeight + 1 { | ||
| blockNumbers <- blockNum |
There was a problem hiding this comment.
I think we should select context here, in case user want to cancel.
| // recalculateL1HandlerMsgHashes recalculates L1Handler message hash to txn hash mappings. | ||
| // Needed because calculateL1MsgHashes2 ran with a buggy WriteL1HandlerMsgHashes. | ||
| // Functionally same as calculateL1MsgHashes2, but optimised for concurrent reads. | ||
| type recalculateL1HandlerMsgHashesToTxnHashes struct{} |
There was a problem hiding this comment.
nit: Is it possible to move this into a separate file or package? I think this file is getting too long now.
I tried the buffered channel but it didnt provided meaningful or any visible difference, time spent on channel is dwarfed by the other heavy operations DB read/write |
Due to a bug, only the first L1Handler transaction in a block was written to MsgHash to L2TxnHash mapping, resulting in consecutive ones to be missing, leading to to error in
starknet_getMessageStatus.Includes migration