[Merged by Bors] - Improve eth1 block sync #2008
Conversation
paulhauner
force-pushed
the
eth1-sync
branch
from
2146954
to
0155e1d
Compare
paulhauner
added
ready-for-review
beacon_node/client/src/notifier.rs
Outdated
| head_info | ||
| .genesis_time | ||
| .saturating_sub(voting_periods_past * voting_period_duration) |
There was a problem hiding this comment.
I think this should subtract the follow distance as well, otherwise we expect our cache to contain the block from the start of the (virtual) voting period rather than the latest block ETH1_FOLLOW_DISTANCE behind the start of the virtual voting period
beacon_node/eth1/src/service.rs
Outdated
| @@ -27,7 +27,7 @@ pub const DEFAULT_NETWORK_ID: Eth1Id = Eth1Id::Goerli; | |||
| /// Indicates the default eth1 chain id we use for the deposit contract. | |||
| pub const DEFAULT_CHAIN_ID: Eth1Id = Eth1Id::Goerli; | |||
|
|
|||
| const STANDARD_TIMEOUT_MILLIS: u64 = 15_000; | |||
| const STANDARD_TIMEOUT_MILLIS: u64 = 60_000; | |||
There was a problem hiding this comment.
I think the higher timeout could be applied to just the GET_DEPOSIT_LOG_TIMEOUT_MILLIS.
Other calls are lightweight and them taking > 15 secs could imply bad health of the eth1 nodes.
beacon_node/eth1/src/service.rs
Outdated
| // | ||
| // This prevents the block downloading routine for running for a very long time and | ||
| // starving the deposit cache updater. | ||
| if Instant::now().duration_since(start_instant) > max_runtime { |
There was a problem hiding this comment.
I'm not sure I understand why we can't complete the blocks downloads before going back to deposits.
Even if we go back to updating deposits every 14secs, we usually won't get more than 1-2 new logs every update and this would slow down the block download.
There was a problem hiding this comment.
I'm not sure I understand why we can't complete the blocks downloads before going back to deposits.
I suspect we could do this and I had a look at it, but it was sufficiently complicated that I wasn't confident to do it before mainnet :)
There was a problem hiding this comment.
I meant we could just remove the
if Instant::now().duration_since(start_instant) > max_runtime
check and let all the block downloads complete. If we get an error somewhere in between, we start from that point in the next update.
There was a problem hiding this comment.
I had some reasoning behind this, but it was loosely held. I think I'll just follow your suggestion and leave it out :)
|
All comments addressed! |
|
Thanks reviewers! bors r+ |
## Issue Addressed NA ## Proposed Changes - Log about eth1 whilst waiting for genesis. - For the block and deposit caches, update them after each download instead of when *all* downloads are complete. - This prevents the case where a single timeout error can cause us to drop *all* previously download blocks/deposits. - Set `max_log_requests_per_update` to avoid timeouts due to very large log counts in a response. - Set `max_blocks_per_update` to prevent a single update of the block cache to download an unreasonable number of blocks. - This shouldn't have any affect in normal use, it's just a safe-guard against bugs. - Increase the timeout for eth1 calls from 15s to 60s, as per @pawanjay176's experience with Infura. ## Additional Info NA
|
Pull request successfully merged into unstable. Build succeeded: |
bors
bot
changed the title
## Issue Addressed NA ## Proposed Changes - Set version to `v1.0.3` - Run cargo update ## Additional Info - ~~Blocked on #2008~~
Issue Addressed
NA
Proposed Changes
max_log_requests_per_updateto avoid timeouts due to very large log counts in a response.max_blocks_per_updateto prevent a single update of the block cache to download an unreasonable number of blocks.Additional Info
NA