[BFT] Requester Engine update to follow guidelines #8205
Conversation
…/8063-requester-engine-update
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…om/onflow/flow-go into yurii/8063-requester-engine-update
jordanschalm
left a comment
jordanschalm
left a comment
There was a problem hiding this comment.
Nice work.
My main suggestion is this one regarding the ability to disable identity validation: I think we should remove that option.
I also created a follow-up PR #8254 including a few documentation and naming updates, as well as moving a test over to synctest to avoid sleeping. Feel free to adjust that PR as you see fit directly.
| // DefaultEntityRequestCacheSize is the default max message queue size for the provider engine. | ||
| // This equates to ~5GB of memory usage with a full queue (10M*500) |
There was a problem hiding this comment.
| // DefaultEntityRequestCacheSize is the default max message queue size for the provider engine. | |
| // This equates to ~5GB of memory usage with a full queue (10M*500) | |
| // DefaultEntityRequestCacheSize is the default max message queue size for the requester engine. | |
| // This equates to ~5GB of memory usage with a full queue (10M*500) |
Where are you getting the 10M/EntityResponse estimate from?
There was a problem hiding this comment.
I took the value from the provider engine: https://github.com/onflow/flow-go/blob/master/engine/common/provider/engine.go#L32
engine/common/requester/engine.go
Outdated
| selector = filter.And( | ||
| selector, | ||
| filter.Not(filter.HasNodeID[flow.Identity](me.NodeID())), | ||
| filter.Not(filter.HasParticipationStatus(flow.EpochParticipationStatusEjected)), | ||
| ) | ||
|
|
||
| // make sure we only send requests to nodes that are active in the current epoch and have positive weight | ||
| if cfg.ValidateStaking { | ||
| selector = filter.And( | ||
| selector, | ||
| filter.HasInitialWeight[flow.Identity](true), | ||
| filter.HasParticipationStatus(flow.EpochParticipationStatusActive), | ||
| ) | ||
| } |
There was a problem hiding this comment.
| } | |
| selector = filter.And( | |
| selector, | |
| filter.Not(filter.HasNodeID[flow.Identity](me.NodeID())), | |
| filter.Not(filter.HasParticipationStatus(flow.EpochParticipationStatusEjected)), | |
| filter.HasInitialWeight[flow.Identity](true), | |
| ) | |
I think we should remove the ValidateStaking option entirely and always at least check that the sender is not ejected. I don't think there is a good reason to have an option that disables identity validation.
This option is only used in the Execution Node. I am guessing this was added because ENs may request collections from LNs who are joining or leaving the network near epoch boundaries. Now that we have explicit epoch statuses in the identity, filter.Not(filter.HasParticipationStatus(flow.EpochParticipationStatusEjected)) will include both joining and leaving LNs that are otherwise in good standing.
Suggestions for Requester Engine (PR #8205)
Co-authored-by: Jordan Schalm <[email protected]>
Co-authored-by: Jordan Schalm <[email protected]>
Context
This PR updates Requester Engine to the latest guidelines we are using for BFT Engines.
What was done:
MessageProcessorinterface.ComponentManagerto implement a restartable component.I have decided to keep logic as it is to prevent changes to the actual business logic to reduce surface of changes and to keep the existing working code as it is.