Deneb review

[paulhauner]

Description

I've been reviewing #4054 for a few days and I'm coming to the realisation that I can't review it alone. It's a 22k line diff across 250 files. I estimate it would take about 1.5 weeks full-time and I can't do any one task full-time anymore. Even if I were to get 50% of uninterrupted time (very optimistic), then it's going to take 3 weeks. Given that I'm away for 2+ weeks next week, we're looking at over a month of Deneb review. Furthermore, I think we need more that one person to review such a large change to give a decent quality assurance.

So, in the interests of getting #4054 merge into unstable expediently, I propose that we split up the review.

Splitting the Review

In the table below I've split the review into components with a lil' script.

I'd like to ask the full-time Lighthouse developers to identify components which:

1. You feel confident reviewing.
2. Did not do an overwhelming amount of the changes.

And then add their name to the "Reviewer" column (if you don't have perms to edit my post, just comment and someone with perms will update it). The "Status" column can be updated when the review is complete.

Some additional thoughts:

• It's reasonable to have two people on one component (e.g., ./consensus could use a couple of eyes).
• If you've already done detailed reviews on some components (e.g., when a PR was merged into deneb-free-blobs) then it would make sense to assign yourself to that component and only review the un-reviewed changes (i.e., don't double-review things).

Components and Assignments

Component                           Files    Lines -  Lines +    Status           Reviewer(s)
--------------------------------  -------  ---------  ---------  ---------------  -------------
./lighthouse                            1         71             Done             MS
./crypto                                6          1  833        Done             MS
./testing                              22         75  712        Done             PH
./database_manager                      1         69             Done             MS
./.github                               2         34  59         Done             JC
./consensus/state_processing           16         53  227        Done             MS
./consensus/ssz_types                   1         77             Done             MS
./consensus/proto_array                 1          1  1          Done             MS
./consensus/fork_choice                 3         19  37         Done             MS
./consensus/cached_tree_hash            1          3  3          Done             MS
./consensus/types                      28        155  1613       Done             MS
./common/slot_clock                     1          1  10         Done             PH
./common/eth2                           3         20  505        Done             JC
./common/validator_dir                  1          1  1          Done             PH
./common/eth2_network_config            7          5  80         Done             PH
./common/deposit_contract               1          1  1          Done             PH
./slasher                               1          2  2          Done             MS
./scripts                               8          8  26         Done             MS
./validator_client                      6        102  240        Done             MS
./beacon_node/execution_layer          12        282  1437       Done             JC,MS
./beacon_node/src                       3          2  110        Done             JC
./beacon_node/beacon_chain             32        528  5531       Done             PH
./beacon_node/operation_pool            3          4  18         Done             MS
./beacon_node/store                    12         50  671        Done             MS
./beacon_node/lighthouse_network       20         64  661        Done             Age
./beacon_node/client                    2         38             Done             Age
./beacon_node/eth1                      1          2  2          Done             PH
./beacon_node/builder_client            1          7  7          Done             PH
./beacon_node/genesis                   1          1  1          Done             PH
./beacon_node/network                  25       1169  6708       Done             Age
./beacon_node/beacon_processor          2         11  92         Done             Age
./beacon_node/http_api                 12        334  702        Done             MS
./account_manager                       1         18  3          Done             PH
./book                                  2          6  1          Done             MS
./lcli                                  5          7  58         Done             PH

[michaelsproul]

I've added my initials to a few parts that I'd like to review. I'll try to do that shortly.

[paulhauner]

I've just added a few things that I know I'll get done before I go on leave. Hopefully I can pick up some more things too.

[macladson]

I've also put my initials for a few things. I'm also tentatively thinking of looking into the /beacon_node/http_api.

[AgeManning]

Threw some age's in there too

[paulhauner]

In #4678 I've reviewed:

• ./common/slot_clock
• ./common/validator_dir
• ./common/eth2_network_config
• ./common/deposit_contract
• ./lcli

[jimmygchen]

I've put my initials to the remaining unassigned ones.

[jimmygchen]

I've done reviewing the ./common/eth2 crate, but I also implemented quite a bit of the changes - so maybe someone else want to take another look? Although I think we should be good, as most of the changes there are quite recent (after we started more structured review process on deneb work) and have been reviewed by others.

[divagant-martian]

so I heard deneb merge is imminent. Guys if you had asked me to review the networking part I would have done so

Result would have probable been that it must be ❇️ perfect ❇️ since I know you've put to much effort into it ☺️

[michaelsproul]

I've finished reviewing the parts I was assigned. Happy to merge!

[michaelsproul]

I also finished off the slab assigned to Mac, and re-reviewed beacon_node/execution_layer which had been reviewed by Jimmy but was still marked "Awaiting Review".

I think we are good to merge. We can address remaining feedback on unstable.

If you have unresolved review comments, please open an issue (or PR) to resolve them on unstable 🙏

Consider this the FINAL CALL before Deneb is merged to unstable. If you've noticed any show-stoppers, please raise them now!

[realbigsean]

done! #4054

appreciate the huge review team effort !!! 🙏 🎉