[MEL] - Pt.2 Integration of Message Extraction Across Arbnode Behind Feature Flag#4402
[MEL] - Pt.2 Integration of Message Extraction Across Arbnode Behind Feature Flag#4402rauljordan wants to merge 6 commits intomasterfrom
Conversation
❌ 11 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
rauljordan
requested review from
diegoximenes,
eljobe and
tsahee
and removed request for
tsahee
rauljordan
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4402 +/- ##
==========================================
- Coverage 32.76% 32.73% -0.04%
==========================================
Files 493 493
Lines 58360 58569 +209
==========================================
+ Hits 19123 19170 +47
- Misses 35893 36038 +145
- Partials 3344 3361 +17 |
| if inboxBatchCount > 0 { | ||
| var err error | ||
| prevBatchMeta, err = b.inbox.GetBatchMetadata(inboxBatchCount - 1) | ||
| if b.msgExtractor != nil { |
There was a problem hiding this comment.
I don't love this pattern of deciding whether or not to use MEL based on whether or not there is a MsgExtractor or if it is nil. When I look at this code, it just smells like the perfect place for the batch_poster to define an interface it needs, and then for the code in NewBatchPoster to use either InboxTracker or melrunner.MessageExtractor based on the opts.
I'm thinking, in this file, the interface could be:
type l1BatchInformer interface {
GetBatchMetadata(batchCount uint64)
GetBatchCount()
}Oh! I see one problem is that the mel implementation actually needs a context.Context in GetBatchcount. Maybe we could add an unused context.Context to InboxTracker's implementation?
I'm not 100% convinced of this. But, it seems like it could be a lot nicer if we can find a common interface to make the batch poster code only care about the actual type during construction.
There was a problem hiding this comment.
Agreed! Actually the MEL implementation has a GetContext() method and it has its own context, so we can easily remove context from the GetBatchCount method. Will check
| StopWaiter: stopwaiter.StopWaiter{}, | ||
| config: config, | ||
| inboxReader: inboxReader, | ||
| msgExtractor: msgExtractor, |
There was a problem hiding this comment.
Similarly, the ConsensusExecutionSyncer could just depend on a:
type msgCounter interface {
GetSafeMsgCount(ctx context.Context)
GetFinalizedMsgCount(ctx context.Context)
}There was a problem hiding this comment.
In this file, I can see that it's much harder to find a common interface for the melrunner.MessageExtractor and the InboxTracker. So, I'm not going to suggest that we actually try to do that. What I would like though is to pull out some of the mel-specific handling and the inboxTracker-specific handling handling into some helper functions. Mainly, this just helps with readability because, for example, the sequenceWithoutLockout function is getting hard to follow because of the multiple branch points.
| coordinator *SeqCoordinator, | ||
| ) (*DelayedSequencer, error) { | ||
| if inboxReader == nil && msgExtractor == nil { | ||
| return nil, nil |
There was a problem hiding this comment.
Shouldn't this be some sort of programming error (as opposed to just returning nil?)
| log.Error("Error getting head state from mel", "err", err) | ||
| res["batchMetadataError"] = err.Error() | ||
| } else { | ||
| batchSeen, err := s.sequencerInbox.GetBatchCount(s.GetContext(), new(big.Int).SetUint64(headMelState.ParentChainBlockNumber+1)) |
There was a problem hiding this comment.
The +1 at the end of this call just feels so much like a "magic number."
I always wonder, "Why?"
| log.Error("Error getting head state from mel", "err", err) | ||
| res["batchMetadataError"] = err.Error() | ||
| } else { | ||
| batchSeen, err := s.sequencerInbox.GetBatchCount(s.GetContext(), new(big.Int).SetUint64(headMelState.ParentChainBlockNumber+1)) |
There was a problem hiding this comment.
I also feel like there is an opportunity for some good code-sharing given that batchSeen is also calculated below on line 293. I think it's the same logic. Can we "refactor: extract function" on this?
| s.delayedBridge = delayedBridge | ||
| } | ||
|
|
||
| func (s *TransactionStreamer) SetMsgExtractor(msgExtractor *melrunner.MessageExtractor) { |
There was a problem hiding this comment.
Setter injection makes me nervous.
- Why can't we have the TransactionStreamer be correct by construction?
- It "seems" like there is an assumption later in the code that if inboxTracker is not nil, then it will be used. So, if a TransactionStreamer is constructed with an inboxTracker, and then, later
SetMsgExtractoris set, the non-nil inboxTracker would mask the msgExtractor on line 1188. (Maybe this setter should also set the inboxTracker to nil, or error out if it's not nil? - I really hate panics. Isn't there some other way to handle this?
|
Amazing feedback and helpful comments @eljobe ! Thank you. I'll take care of this soon |
2 participants
This PR integrates MessageExtraction across the Nitro node gated behind the MEL feature flag configuration, allowing us to run a MEL-enabled node safely. Some of the changes are refactoring of certain variables to other packages to support MEL, but most changes are straightforward.
Depends on #4398