[Merged by Bors] - Return eth1-related data via the API #1797
Conversation
paulhauner
force-pushed
the
eth1-sync-status
branch
from
1c95aa6 to
2d9bbdf
Compare
paulhauner
added
ready-for-review
|
Hey @pawanjay176 would you be interested in reviewing this? :) |
pawanjay176
left a comment
pawanjay176
left a comment
There was a problem hiding this comment.
Looks good. Just a few minor suggestions and questions :)
Apart from this, I had one minor peeve with the update_deposit_cache for a long time. Currently, the max_log_requests_per_update config param in eth1 service is always set to u64::max(). Because of this, when we start building the eth1 cache from scratch, the call to update_deposit_cache only returns after getting all logs upto current head. If even a single query to the eth1 node fails, we have to start over building the deposit cache.
I think it'd be good if we set max_log_requests_per_update to some smaller value so that we don't try to get all the logs with a single update_deposit_cache call.
| type Eth1DataVoteCount = HashMap<(Eth1Data, BlockNumber), u64>; | ||
|
|
||
| /// We will declare ourself synced with the Eth1 chain, even if we are this many blocks behind. | ||
| const ETH1_SYNC_TOLERANCE: u64 = 8; |
There was a problem hiding this comment.
Any specific reason for 8? Just curious
There was a problem hiding this comment.
Nope, no reason. I added a note to that measure :)
There was a problem hiding this comment.
Well, 8 just felt like a decent number to me. I can't back it up, though 😅
book/src/api-lighthouse.md
Outdated
| beacon node are ready for block production. | ||
| - This value might be set to | ||
| `false` whilst `eth1_node_sync_status_percentage == 100.0` if the beacon | ||
| node is still build its internal cache. |
There was a problem hiding this comment.
| node is still build its internal cache. | |
| node is still building its internal cache. |
book/src/api-lighthouse.md
Outdated
| } | ||
| ``` | ||
|
|
||
| ### `/lighthouse/eth1/block_cache` |
There was a problem hiding this comment.
/lighthouse/eth1/block_cache is already present above. This is a repetition
| }; | ||
|
|
||
| self.eth1_service = None; | ||
| self.eth1_service = Some(backend.core.clone()); |
There was a problem hiding this comment.
Any idea why we set this to None earlier? curious again :)
beacon_node/eth1/src/service.rs
Outdated
| &self, | ||
| ) -> Result<(DepositCacheUpdateOutcome, BlockCacheUpdateOutcome), String> { | ||
| let endpoint = &self.config().endpoint.clone(); | ||
| let remote_head_block_number = |
There was a problem hiding this comment.
We could save a call to the eth1 node by using eth_getBlockByNumber with 'latest' in the params to get the block corresponding to remote head.
There was a problem hiding this comment.
Nice one, I'll do this!
Co-authored-by: Pawan Dhananjay <pawandhananjay@gmail.com>
|
All comments addressed! |
|
bors r+ |
## Issue Addressed - Related to #1691 ## Proposed Changes Adds the following API endpoints: - `GET lighthouse/eth1/syncing`: status about how synced we are with Eth1. - `GET lighthouse/eth1/block_cache`: all locally cached eth1 blocks. - `GET lighthouse/eth1/deposit_cache`: all locally cached eth1 deposits. Additionally: - Moves some types from the `beacon_node/eth1` to the `common/eth2` crate, so they can be used in the API without duplication. - Allow `update_deposit_cache` and `update_block_cache` to take an optional head block number to avoid duplicate requests. ## Additional Info TBC
|
Pull request successfully merged into master. Build succeeded: |
bors
bot
changed the title
Issue Addressed
Proposed Changes
Adds the following API endpoints:
GET lighthouse/eth1/syncing: status about how synced we are with Eth1.GET lighthouse/eth1/block_cache: all locally cached eth1 blocks.GET lighthouse/eth1/deposit_cache: all locally cached eth1 deposits.Additionally:
beacon_node/eth1to thecommon/eth2crate, so they can be used in the API without duplication.update_deposit_cacheandupdate_block_cacheto take an optional head block number to avoid duplicate requests.Additional Info
TBC