Unknown block for envelope#8992
Conversation
…known-block-for-envelope
pawanjay176
left a comment
pawanjay176
left a comment
There was a problem hiding this comment.
Looks good as a whole.
Just a suggestion, curious what you think. I'm also not fully sure what's a good way to avoid DoS
| } | ||
|
|
||
| // Check to ensure this won't over-fill the queue. | ||
| if self.awaiting_envelopes_per_root.len() >= MAXIMUM_QUEUED_ENVELOPES { |
There was a problem hiding this comment.
I'm a bit unsure of this mechanism.
When we are adding stuff to this queue, the envelope hasn't gone through any kind of verification, so we could potentially get DoS'd with garbage envelopes even before the block comes in and fill up this queue.
The awaiting_envelopes_per_root container currently is behaving in a FIFO manner. I wonder if the better mechanism is to do LIFO where we evict older entries to make space for newer entries. I don't think that completely protects us from dos but its harder to dos if it becomes timing dependent.
For a later TODO, We should probably do some lightweight pre-validation when we receive an envelope (check builder signature for e.g)
There was a problem hiding this comment.
ive switched the queue to LIFO and added a TODO about performing early sig verification checks
| work_queues.delayed_block_queue.push(work, work_id) | ||
| } | ||
| Work::DelayedImportEnvelope { .. } => { | ||
| work_queues.delayed_block_queue.push(work, work_id) |
There was a problem hiding this comment.
Thoughts on having a different queue here for delayed envelopes?
There was a problem hiding this comment.
i introduced a new delayed_envelope_queue
michaelsproul
left a comment
michaelsproul
left a comment
There was a problem hiding this comment.
Looks good, nice clean impl. Just had one small question
|
We need something like this for blocks where their parent envelope is unknown too, right? |
…known-block-for-envelope
|
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
mergify
bot
added
waiting-on-author
eserilev
added
ready-for-review
michaelsproul
added
ready-for-merge
Merge Queue Status
This pull request spent 28 minutes 20 seconds in the queue, including 26 minutes 5 seconds running CI. Required conditions to merge
|
3 participants
Issue Addressed
Add a queue that allows us to reprocess an envelope when it arrives over gossip references a unknown block root. When the block is finally imported, we immediately reprocess the queued envelope.
Note that we don't trigger a block lookup sync. Incoming attestations for this block root will already trigger a lookup for us. I think thats good enough