Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deneb #4054

Merged
merged 672 commits into from
Oct 16, 2023
Merged

Deneb #4054

merged 672 commits into from
Oct 16, 2023

Conversation

divagant-martian
Copy link
Collaborator

@divagant-martian divagant-martian commented Mar 6, 2023

Issue Addressed

This branch is currently targeting Devnet 9. Devnet 7 compat can be found here: https://github.com/sigp/lighthouse/tree/deneb-devnet-7

* rename to follow name in spec

* use roots and indexes

* wip

* fix req/resp types

* move blob identifier to consensus types
@realbigsean realbigsean added the major-task A significant amount of work or conceptual task. label Mar 8, 2023
divagant-martian and others added 18 commits March 10, 2023 16:23
* Modify blob topics

* add signedblol type

pubsun messages are signed

* improve subnet topic index

* improve display code

* fix parse code

---------

Co-authored-by: Pawan Dhananjay <pawandhananjay@gmail.com>
* wip

* fix router

* arc the byroot responses we send

* add placeholder for blob verification

* respond to blobs by range and blobs by root request in the most horrible and gross way ever

* everything in sync is now unimplemented

* fix compiation issues

* http_pi change is very small, just add it

* remove ctrl-c ctrl-v's docs
* Update kzg interface

* Update utils

* Update dependency

* Address review comments
Comment on lines +367 to +371
let max_value = Uint256::from(12u8) * Uint256::exp10(25);
if base_value > max_value || comparison_value > max_value {
return None;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is really just a sanity check on the value, as a valid payload is unlikely to hit this value. Is it really necessary to do this check though? (btw this would be 125 million xDai on Gnosis Chain)

Copy link
Member

@realbigsean realbigsean Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check is integral to the following assumption though (couple lines after this):

// Now we should be able to calculate the difference without division by zero or overflow

So i think we should leave it for now and we could potentially re-work the rest of the method in a different PR to handle division by zero/overflow otherwise

ethDreamer and others added 4 commits October 10, 2023 23:51
* Initial Commit of State LRU Cache

* Build State Caches After Reconstruction

* Cleanup Duplicated Code in OverflowLRUCache Tests

* Added Test for State LRU Cache

* Prune Cache of Old States During Maintenance

* Address Michael's Comments

* Few More Comments

* Removed Unused impl

* Last touch up

* Fix Clippy
* remove remaining uses of serde_derive

* fix lockfile

---------

Co-authored-by: João Oliveira <hello@jxs.pt>
* Add `blob_sidecar` event to SSE.

* Return 202 if a block is published but failed blob validation when validation level is `Gossip`.

* Move `BlobSidecar` event to `process_gossip_blob` and add test.

* Emit `BlobSidecar` event when blobs are received over rpc.

* Improve test assertions on `SseBlobSidecar`s.

* Add quotes to blob index serialization in `SseBlobSidecar`

Co-authored-by: realbigsean <seananderson33@GMAIL.com>

---------

Co-authored-by: realbigsean <seananderson33@GMAIL.com>
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The consensus dir is looking good. Happy to merge!

consensus/types/src/chain_spec.rs Outdated Show resolved Hide resolved
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate all the misc code improvements.

Sync itself is a beast with waay to many edge cases. I did a high-level review and the code changes all seem to be improvements and necessary modifications for deneb.

I don't want to delay this further, so I didn't review sync-logic in great detail. I think it's best we merge to unstable and test on live networks to find possible edge cases. I didn't see anything in sync that would be catastrophic and bugs are likely to just stall or prevent syncing, so testing on live networks I think is the best way to go.

beacon_node/lighthouse_network/src/config.rs Outdated Show resolved Hide resolved
beacon_node/network/src/router.rs Outdated Show resolved Hide resolved
beacon_node/network/src/sync/block_lookups/mod.rs Outdated Show resolved Hide resolved
beacon_node/network/src/sync/block_lookups/mod.rs Outdated Show resolved Hide resolved
/// NOTE: Removed the shift by one for deneb because otherwise the last batch before the blob
/// fork boundary will be of mixed type (all blocks and one last blockblob), and I don't want to
/// deal with this for now.
/// This means finalization might be slower in deneb
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall the purpose of this was that when we sync the last epoch/batch we downloaded could be finalized in fork-choice. I think originally it was handy to debug as we got confirmation that we have successfully processed and finalized a whole epoch.

After the fork, do we intend to shift this back?

Off the top of my head, I can't see why current shift won't work, in principle we should start another batch even if there is only 1 slot to download and that should finalize the previous batch. I guess we have to wait to download and process the next batch before we see the finalization. I thought there was a more significant downside, but if thats it, seem alright?

I think potentially the other reason we went down this path was because it was designed to sync from genesis, and we always had the genesis block. So it made sense to shift by one to avoid re-downloading one block.

If this new system works fine, then I think we probably should remove the comments above which explains the shift, just so its not confusing for the next person to look at what is going on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the fork, do we intend to shift this back?

Yea I think we can whenever we are able to stop supporting the capella -> deneb transition, so maybe in a couple forks. I'll leave the note in case we end up switchiing it back

Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My review of scripts. Tracking issue here: #4847

scripts/tests/genesis.json Show resolved Hide resolved
scripts/tests/vars.env Show resolved Hide resolved
Co-authored-by: Jimmy Chen <jchen.tc@gmail.com>
@michaelsproul michaelsproul added RFC Request for comment ready-for-merge This PR is ready to merge. labels Oct 16, 2023
This was referenced Oct 16, 2023
* use workspace deps in kzg crate

* delete unused blobs dp path field

* full match on fork name in engine api get payload v3

* only accept v3 payloads on get payload v3 endpoint in mock el

* remove FIXMEs related to merge transition tests

* move static tx to test utils

* default max_per_epoch_activation_churn_limit to mainnet value

* remove unnecessary async

* remove comment

* use task executor in `blob_sidecars` endpoint
@michaelsproul michaelsproul merged commit 8b0545d into unstable Oct 16, 2023
37 checks passed
@michaelsproul michaelsproul deleted the deneb-free-blobs branch October 16, 2023 23:58
@michaelsproul michaelsproul restored the deneb-free-blobs branch October 16, 2023 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deneb major-task A significant amount of work or conceptual task. ready-for-merge This PR is ready to merge. RFC Request for comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.