Support for obtaining deposit snapshots during trustedNodeSync #4303

zah · 2022-11-08T22:07:42Z

Other changes:

More optimal search for TTD block.
Add timeouts to all REST requests during trusted node sync. Fixes trustedNodeSync may hang forever due to lack of timeout handling in all REST requests #4037
Removed support for storing a deposit snapshot in the network metadata.

arnetheduck · 2022-11-09T06:19:51Z

beacon_chain/eth1/eth1_monitor.nim

+    fatal "Failed to initialize web3 provider",
+          depositContract = depositContractAddress,
+          web3Url, err = providerRes.error
+    quit 1


do we close the db when quit runs? also, this prevents starting eth1 and eth2 in "random" order which is not nice - ie I should be able to start the two services in any order and they should be "prone" to resume as soon as the "other" is up

I agree in general with the suggestions, but this requires more invasive changes to the Eth1Monitor that will be delivered in another PR.

arnetheduck · 2022-11-09T06:22:22Z

beacon_chain/eth1/eth1_monitor.nim

+            depositContract = depositContractAddress,
+            deploymentBlock = $depositContractDeployedAt,
+            err = e.msg
+      quit 1


ditto... and how does this work when you start nimbus with an unsynced eth1?

This merely maintains the Status quo as Nimbus would already exit at start-up when paired with an EL that hasn't synced the deposit contract deployment block.

I've improved the behavior in the multi EL branch where the BN is able to wait for the EL to sync before the rest of the Eth1Monitor activities start.

how do you mean, shut down? we can run the BN without an EL entirely (in optmistic mode). Also, geth doesn't even start syncing without a BN. Since when do we require a synced EL?

in fact, we're quite buggy in this aspect: we run the deposit monitoring even when we know the EL is not synced - see #4057 - this causes many support requests in general because of all the error / warning logging that ensues

The deposit contract deployment block is part of the PoW history that Geth will sync on its own.
Otherwise, yes, the general behavior can be improved, but as I explained these are more invasive changes that are currently part of another PR.

beacon_chain/rpc/rest_beacon_api.nim

arnetheduck · 2022-11-09T06:31:08Z

beacon_chain/trusted_node_sync.nim

@@ -229,6 +267,16 @@ proc doTrustedNodeSync*(
      databaseDir, head = shortLog(dbHead.get())
    (headSlot, dbHead.get())

+  # Fetch deposit snapshot.  This API endpoint is still optional.
+  let depositSnapshot = await fetchDepositSnapshot(client)


I think this is better to put behind a flag (default false) while v0 is a thing

The failure is relatively benign. Only a INFO level message is printed.

nonetheless, it's a draft feature - also, disabling it completely is useful in case something breaks - this was part of the rationale for moving TNS to a finalized state alone: this is the minimal set of data needed to get started, and we should keep the possibility to start like this regardless of what the server supports - blocks and deposit snapshots are optional extras.

I've added the suggested CLI flag, but I'm contemplating removing it since the API is now officially in the spec.

arnetheduck · 2022-11-09T06:33:50Z

Removed support for storing a deposit snapshot in the network metadata.

since no snapshot sources support the new snapshots, isn't this a regression? it seems reasonable to keep the old support until the new version has reached some reasonable penetration (basically when it's supported by https://eth-clients.github.io/checkpoint-sync-endpoints/)

beacon_chain/trusted_node_sync.nim

zah · 2022-11-09T10:25:38Z

since no snapshot sources support the new snapshots, isn't this a regression? it seems reasonable to keep the old support until the new version has reached some reasonable penetration (basically when it's supported by https://eth-clients.github.io/checkpoint-sync-endpoints/)

The old support was baking a snapshot in the binary. I haven't been updating these snapshots in a while and every running client will have a more recent record in its database. New clients that didn't obtain a snapshot from trusted node sync will be initialized to the deposit contract deployment block which seems reasonable (i.e. this is the behavior of all other clients)

arnetheduck · 2022-11-09T10:32:41Z

New clients that didn't obtain a snapshot from trusted node sync will be initialized to the deposit contract deployment block which seems reasonable (i.e. this is the behavior of all other clients)

this is the regression indeed - there's no support in servers for the eip, and there won't be fore the foreseeable future - since the code for reading the baked-in snapshot is still there, it seems reasonable to keep using it until the eip has reached penetration (which can take anything from days to months) - I'd prefer seeing this feature as a pure addon until the standard has settled (both EIP and beacon API are drafts at this point - it's quite possible there will be changes in the format meaning that we might have to go through a potentially painful DB migration, so a way out is needed)

arnetheduck · 2022-11-09T10:36:45Z

beacon_chain/trusted_node_sync.nim

+  # Fetch deposit snapshot.  This API endpoint is still optional.
+  let depositSnapshot = await fetchDepositSnapshot(client)
+  if depositSnapshot.isOk:
+    info "Writing deposit contracts snapshot",


shouldn't the snapshot be validated against the state before writing to disk?

zah · 2022-11-09T22:02:53Z

I still don't think this is a regression. The non-genesis snapshots were tested only in the testnets. The metadata for mainnet was never changed after the genesis event.

arnetheduck · 2022-11-10T07:51:37Z

I still don't think this is a regression.

the difference would be that we don't need to download deposits for the genesis validators, no?

github-actions · 2022-11-18T18:25:59Z

Unit Test Results

        9 files ±  0   1 023 suites +3 24m 8s ⏱️ -52s
  2 972 tests +  4   2 779 ✔️ +  4 193 💤 ±0 0 ❌ ±0
13 901 runs +12 13 682 ✔️ +12 219 💤 ±0 0 ❌ ±0

Results for commit bd3554a. ± Comparison against base commit 7cf432b.

♻️ This comment has been updated with latest results.

arnetheduck · 2022-12-06T19:48:27Z

beacon_chain/trusted_node_sync.nim

+        else:
+          warn "The downloaded deposit snapshot does not agree with the downloaded state"
+      else:
+        warn "Deposit tree snapshot was not imported", reason = depositSnapshot.error


the download is optional and nothing will happen if it doesn't work except a small delay, so this is a notice at most

Well, with the new CLI option the user explicitly requested the download of a deposit snapshot and it didn't work. I think a warning is reasonable. The user should check their configuration.

arnetheduck · 2022-12-06T19:48:57Z

beacon_chain/trusted_node_sync.nim

-          return await client.getBlockV2(BlockIdent.init(slot), cfg)
+          return awaitWithTimeout(client.getBlockV2(BlockIdent.init(slot), cfg),
+                                  smallRequestsTimeout):
+            raise newException(CatchableError, "Request timed out")


if there's a timeout, we should reattempt - that's what the retries are for

There will be retry on this error. An exception is raised here, so it can be handled in the except block below, which will produce the following message:

warn "Retrying download of block", slot, err = "Request timed out"

I didn't want to duplicate the logic in multiple places and the cost of raising an exception is not that high in this infrequent code path.

Other changes: * More optimal search for TTD block. * Add timeouts to all REST requests during trusted node sync. Fixes #4037 * Removed support for storing a deposit snapshot in the network metadata.

EIP-4881 was never correctly implemented, the `DepositTreeSnapshot` structure has nothing to do with its actual definition. Reflect that by renaming the type to a Nimbus-specific `DepositContractSnapshot`, so that an actual EIP-4881 implementation can use the correct names. - https://eips.ethereum.org/EIPS/eip-4881#specification Notably, `DepositTreeSnapshot` contains a compressed sequence in `finalized`, only containing the minimally required intermediate roots. That also explains the incorrect REST response reported in #5508. The non-canonical representation was introduced in #4303 and is also persisted in the database. We'll have to maintain it for a while.

arnetheduck reviewed Nov 9, 2022

View reviewed changes

beacon_chain/rpc/rest_beacon_api.nim Show resolved Hide resolved

arnetheduck reviewed Nov 9, 2022

View reviewed changes

beacon_chain/trusted_node_sync.nim Outdated Show resolved Hide resolved

arnetheduck reviewed Nov 9, 2022

View reviewed changes

zah force-pushed the deposit-snapshots-in-trusted-sync branch from d4b3288 to d0a208d Compare November 18, 2022 17:54

zah force-pushed the deposit-snapshots-in-trusted-sync branch 2 times, most recently from 37d6c7c to 051df25 Compare November 20, 2022 13:57

zah force-pushed the deposit-snapshots-in-trusted-sync branch 7 times, most recently from ce12332 to 6c9b2be Compare December 5, 2022 15:03

arnetheduck reviewed Dec 6, 2022

View reviewed changes

Support for obtaining deposit snapshots during trustedNodeSync

bd3554a

Other changes: * More optimal search for TTD block. * Add timeouts to all REST requests during trusted node sync. Fixes #4037 * Removed support for storing a deposit snapshot in the network metadata.

zah force-pushed the deposit-snapshots-in-trusted-sync branch from 6c9b2be to bd3554a Compare December 6, 2022 23:18

zah merged commit d30cb8b into unstable Dec 7, 2022

zah deleted the deposit-snapshots-in-trusted-sync branch December 7, 2022 10:24

This was referenced Dec 19, 2022

Client abruptly stops delivering sync committee messages #4320

Open

Support EIP-4881 #4278

Closed

etan-status mentioned this pull request Mar 7, 2024

rename DepositTreeSnapshot -> DepositContractSnapshot #6036

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for obtaining deposit snapshots during trustedNodeSync #4303

Support for obtaining deposit snapshots during trustedNodeSync #4303

zah commented Nov 8, 2022

arnetheduck Nov 9, 2022

zah Dec 6, 2022

arnetheduck Nov 9, 2022

zah Dec 6, 2022 •

edited

Loading

arnetheduck Dec 7, 2022

arnetheduck Dec 7, 2022 •

edited

Loading

zah Dec 7, 2022

arnetheduck Nov 9, 2022

zah Nov 9, 2022

arnetheduck Nov 9, 2022

zah Dec 6, 2022 •

edited

Loading

arnetheduck commented Nov 9, 2022

zah commented Nov 9, 2022 •

edited

Loading

arnetheduck commented Nov 9, 2022

arnetheduck Nov 9, 2022

zah commented Nov 9, 2022

arnetheduck commented Nov 10, 2022

github-actions bot commented Nov 18, 2022 •

edited

Loading

arnetheduck Dec 6, 2022

zah Dec 6, 2022 •

edited

Loading

arnetheduck Dec 6, 2022

zah Dec 6, 2022

Support for obtaining deposit snapshots during trustedNodeSync #4303

Support for obtaining deposit snapshots during trustedNodeSync #4303

Conversation

zah commented Nov 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zah Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnetheduck Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zah Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

arnetheduck commented Nov 9, 2022

zah commented Nov 9, 2022 • edited Loading

arnetheduck commented Nov 9, 2022

Choose a reason for hiding this comment

zah commented Nov 9, 2022

arnetheduck commented Nov 10, 2022

github-actions bot commented Nov 18, 2022 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

zah Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zah Dec 6, 2022 •

edited

Loading

arnetheduck Dec 7, 2022 •

edited

Loading

zah Dec 6, 2022 •

edited

Loading

zah commented Nov 9, 2022 •

edited

Loading

github-actions bot commented Nov 18, 2022 •

edited

Loading

zah Dec 6, 2022 •

edited

Loading