Refactor provider broadcast #4744
Conversation
zhangchiqing
requested review from
koko1123,
janezpodhostnik,
sideninja and
peterargue
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #4744 +/- ##
==========================================
- Coverage 55.62% 55.57% -0.05%
==========================================
Files 931 932 +1
Lines 86292 86370 +78
==========================================
+ Hits 47996 47997 +1
- Misses 34665 34742 +77
Partials 3631 3631
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| broadcasted = true | ||
| } | ||
| } | ||
| broadcasted, err := e.providerEngine.BroadcastExecutionReceipt( |
There was a problem hiding this comment.
this looks like the only use of the returned boolean, and that is only for logging. Does it still make sense to return this boolean? If we still want this in the log, can we not move the logging inside of BroadcastExecutionReceipt
There was a problem hiding this comment.
Yes, it's only used for logging for now. And yes, we could move the logging to the provider engine, but it's not hurt to still return the boolean. I'd like to keep as is so that we don't have to introduce additional logging just to know whether it's broadcasted. It's simple to just merge the broadcaster along with other property of the receipts, so that it's easy to search for the receipt.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
| @@ -471,29 +464,11 @@ func (e *Engine) executeBlock( | |||
| return | |||
| } | |||
|
|
|||
| // if the receipt is for a sealed block, then no need to broadcast it. | |||
| lastSealed, err := e.state.Sealed().Head() | |||
There was a problem hiding this comment.
This is the only place where protocol state is being used, which is to check if a result can be skipped for broadcasting if it's already sealed.
After moving this check to provider engine, the ingestion engine no long depend on protocol state, therefore the protocol state can be removed from the engine
| @@ -785,6 +744,7 @@ func TestBlocksArentExecutedMultipleTimes_multipleBlockEnqueue(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestBlocksArentExecutedMultipleTimes_collectionArrival(t *testing.T) { | |||
| unittest.SkipUnless(t, unittest.TEST_FLAKY, "To be fixed later") | |||
There was a problem hiding this comment.
this test will be refactored in the follow up PR
| broadcasted = true | ||
| } | ||
| } | ||
| broadcasted, err := e.providerEngine.BroadcastExecutionReceipt( |
There was a problem hiding this comment.
Yes, it's only used for logging for now. And yes, we could move the logging to the provider engine, but it's not hurt to still return the boolean. I'd like to keep as is so that we don't have to introduce additional logging just to know whether it's broadcasted. It's simple to just merge the broadcaster along with other property of the receipts, so that it's easy to search for the receipt.
This PR move the condition of "whether EN should broadcast execution result" from ingestion engine to provider engine, which reduce the ingestion engine complexity.