Small collection of improved diagnostics for partials.
- In the peer info struct (exposing peer data via `/lighthouse/peers`), add a new field to indicate on which subnets the peer supports partials.
- Fix the description of several metrics.
- Downgrade some noisy logging when sending partials to trace, and downgrade one warning that should not worry the user.
Co-Authored-By: Daniel Knopik <daniel@dknopik.de>
N/A
Implement range sync in gloas.
Basically requests blocks and payloads post gloas from the same peer, couples them and sends it for processing.
Does not change sync much at all other than adding the machinery for payloads by range requests.
Main changes are:
`RangeSyncBlock` which used to be a struct is an enum to account for the Gloas case. This allows a clear separation between gloas and pre-gloas code.
`AvailableBlockData` now has a `BlockInEnvelope` variant. This is to clearly indicate the post gloas case. I feel this is simpler to follow compared to `NoData` variant.
Tries to extract post gloas logic into its own functions so that there is minimal logic change in mainnet range sync behaviour.
This is meant as a stable base on which we can iterate further to make range sync cleaner and for unblocking range sync support on devnet. Some ideas for later is removing the retry mechanism in favour of delegating column fetching to lookup sync which can be done post #9155 and batch signature verifying envelopes.
Co-Authored-By: Pawan Dhananjay <pawandhananjay@gmail.com>
Co-Authored-By: dapplion <35266934+dapplion@users.noreply.github.com>
Co-Authored-By: Eitan Seri-Levi <eserilev@ucsc.edu>
The initial partial send after getBlobs only sends incomplete partial columns, causing issues as we do not propagate these columns.
Return complete and incomplete columns from `get_columns_and_mark_as_local_fetched`, and convert any complete columns to partials if necessary. Send all columns retrieved this way after getBlobs
Co-Authored-By: Daniel Knopik <daniel@dknopik.de>
N/A
Currently, we have `EnvelopeError` having a `ImportError` wrapping a `BlockError`. I feel this is extremely unintuitive because most of the envelope processing functions can simply return an `EnvelopeError` that makes sense in the function's context. It revealed further ugliness when implementing range sync in #9362
This PR does 2 main things:
1. Removes `ImportError(BlockError)` variant
2. Adds `EnvelopeError(EnvelopeError)` variant to a `BlockError`.
I feel this is more natural as there can be envelope errors when we try importing a Block but envelope errors can be contained to just envelope related errors.
The main blocker to doing this was `PayloadVerificationHandle` returning a `BlockError`. It uses a very small subset of `BlockError` which I extracted to its own error type which can be converted into both a BlockError and EnvelopeError.
This allows us to keep most of the pure envelope processing functions to just return EnvelopeErrors while we convert it to a `BlockError` only in import paths where we need to return a consolidated `BlockError`.
Co-Authored-By: Pawan Dhananjay <pawandhananjay@gmail.com>
- https://github.com/sigp/lighthouse/pull/9155 remove the trait abstraction for processing block / blobs / columns / payloads
As a result we would have to duplicate x3 the big match on `BlockProcessingResult` we currently have in block lookups mod.rs
This PR moves the match of `BlockProcessingResult` to `sync_methods` to reduce the diff of https://github.com/sigp/lighthouse/pull/9155. There are some subtle changes that deserve dedicated attention, and may be drowned in the bigger diff of https://github.com/sigp/lighthouse/pull/9155 otherwise:
| Unstable | This PR / #9115 |
| - | - |
| Some error conditions immediately `Drop` the lookup (no retries). For example for "internal" errors like the BeaconChainError | Retries ALL errors 4 times. I believe assuming some errors are internal is risky as dropping a lookup drops all its children potentially forcing the node to resync a lot of blocks because of an internal timeout
Co-Authored-By: dapplion <35266934+dapplion@users.noreply.github.com>
On Glamsterdam devnets we started seeing Lighthouse nodes unable to start with errors like:
> May 26 04:34:01.582 CRIT Failed to start beacon node reason: "Unable to load fork choice from disk: ForkChoiceError(ProtoArrayStringError(\"find_head failed: InvalidBestNode(InvalidBestNodeInfo { current_slot: Slot(23550), start_root: 0x2c70b1641c29ec46360c99f9a8512f077862cbbc603e16f4a423007d210b0c5f, justified_checkpoint: Checkpoint { epoch: Epoch(712), root: 0x2c70b1641c29ec46360c99f9a8512f077862cbbc603e16f4a423007d210b0c5f }, finalized_checkpoint: Checkpoint { epoch: Epoch(710), root: 0xede5e0b09b51bdb5445ade3398e685bd193b845e0b0ffb827f0c3fec8277ea51 }, head_root: 0x2c70b1641c29ec46360c99f9a8512f077862cbbc603e16f4a423007d210b0c5f, head_justified_checkpoint: Checkpoint { epoch: Epoch(710), root: 0xede5e0b09b51bdb5445ade3398e685bd193b845e0b0ffb827f0c3fec8277ea51 }, head_finalized_checkpoint: Checkpoint { epoch: Epoch(709), root: 0xbb243eff616ff362c52b83113e7c536d0a68cb9ca3d6a1cb1055e732219d9736 } })\"))"
This error was the result of an overly-strict sanity check, based on assumptions that are not true under extreme network conditions.
Completely remove the `InvalidBestNode` failure path: it is not compliant with the spec, and is actively harmful when triggered (it prevents Lighthouse from starting at all). The error was reachable in any situation where all leaf nodes of fork choice were ineligible to be the head. The payload invalidation tests show some examples of cases where this would happen, and the [newly-added regression test](9a5df1d982) shows a contrived case where it can happen on a Gloas network without _any_ slashings or invalid blocks. There are probably many more cases where it can happen.
We do not lose anything by removing it. The spec's implementation of `get_head` _always_ returns something (unless it crashes), and in these cases it is correct to return the starting node of the traversal: the justified checkpoint block. This is what we now do, and what the new test verifies.
I've also added some facilities to the harness for injecting attestations with fixed `payload_present` fields. @hopinheimer found himself needing something similar when messing with reorg tests, so I think these are probably useful. It might be possible to do without them by juggling the payload reveal timing in just the right way, but I think this approach is just way simpler.
Co-Authored-By: Michael Sproul <michael@sigmaprime.io>
Breakout from:
- https://github.com/sigp/lighthouse/pull/9295
We currently do not handle the verification of payload attestations on non-canonical side chains, we always attempt to use the head. The included regression test demonstrates this, and there is _also_ a fork choice compliance test in #9295 that triggers it.
This PR is a bit opinionated, but I'll explain my judgements:
- We need a way to get the PTC for an arbitrary slot from an arbitrary state. This involves potential state advances, database lookups, etc. There is some fiddly logic required to check that states are in range/etc.
- We _already have_ a cache with the exact same lifecycle as the PTCs, namely the attester shuffling cache. Therefore, we can de-duplicate a lot of the complexity by storing the PTCs for a given epoch (and decision block) in this cache.
The other opinionated change is in the tests. The previous tests were set up kind of nicely to avoid instantiating a `BeaconChainHarness`. However they were not using mocking, which made testing the non-canonical chain case kind of infeasible. To remedy this, I've changed them to just use a beacon chain harness and create two chains using its relatively easy to use methods for doing this. The running time of the tests goes from something like 2.6s for 8 tests to 3.3s for 9 tests, which is only an increase of 0.04s/test. Negligible. Another plus to using the `BeaconChainHarness` is that it avoids a bunch of the cruft to create synthetic non-mocked beacon chain bits.
At the same time, I've made some attempt to improve modularity (and fit with the `GossipVerificationContext`) by pulling out the guts of `with_committee_cache` into a new function (`with_cached_shuffling`) that clearly shows its dependency surface.
Co-Authored-By: Michael Sproul <michael@sigmaprime.io>
Co-Authored-By: dapplion <35266934+dapplion@users.noreply.github.com>
