Limit inbound download and verify queue #1622
Conversation
Uses the "load shed directly" design pattern from ZcashFoundation#1618.
teor2345
added
C-bug
| #[instrument(skip(self, hash), fields(hash = %hash))] | ||
| pub fn download_and_verify(&mut self, hash: block::Hash) -> bool { | ||
| pub fn download_and_verify(&mut self, hash: block::Hash) -> DownloadAction { |
There was a problem hiding this comment.
Where does this return value actually get used? I'm surprised to see that changing the return type here didn't end up causing compiler errors in any other part of our code.
There was a problem hiding this comment.
The return value is not being used https://github.com/ZcashFoundation/zebra/blob/main/zebrad/src/components/inbound.rs#L243
The other side of the connection is not expecting anything in response to the AdvertiseBlock they sent to us.
I was a bit surprised about this as well but i assumed it was fine.
There was a problem hiding this comment.
@yaahc currently the Inbound service doesn't act on the DownloadAction.
But in future, we might want to return an error if a peer sends us lots of queued or cached blocks, because it means that they are a long way behind us. (Or for zcashd, it means their idea of our height is wrong.)
|
I think we've dealt with all the concerns here, so I'm going to merge this PR so we can focus on other work. If there's anything we need to fix, let's open another ticket. |
Motivation
The
Inboundservice'sDownloadsqueue can grow without limit.See #1618 for some general background.
Solution
Drop new requests when the Inbound downloads queue reaches a fixed limit.
The code in this pull request has:
Review
This change is a lower priority than #1620, so I'm not going to tag anyone for review.
Follow Up Work
Do the rest of #1618.