Devnet6 by realbigsean · Pull Request #4404 · sigp/lighthouse

realbigsean · 2023-06-15T17:05:19Z

Issue Addressed

Which issue # does this PR address?

Proposed Changes

Please list or describe the changes introduced by this PR.

Additional Info

Please provide any additional information. For example, future considerations
or information useful for reviewers.

…into some-blob-reprocessing-work

…e missing blob ids in the da checker.

…into some-blob-reprocessing-work

…bigsean/lighthouse into some-blob-reprocessing-work

…into some-blob-reprocessing-work

…into devnet6

realbigsean · 2023-06-28T20:35:53Z

-        if self.blocks.contains_key(&block.block_hash()) {
-            return Err(format!("{:?} is already known", block.block_hash()));
-        } else if block.parent_hash() != ExecutionBlockHash::zero()
+        if block.parent_hash() != ExecutionBlockHash::zero()


Rejecting duplicates ends up being a big a pain in tests if we re-use a mock el between test beacon chains for example

Worth adding a comment here just in case we add another test in the future that requires rejecting duplicates?

realbigsean · 2023-06-28T20:36:38Z

    pub fn get_blobs_bundle(&mut self, id: &PayloadId) -> Option<BlobsBundleV1<T>> {
-        // remove it to free memory
-        self.blobs_bundles.remove(id)
+        self.blobs_bundles.get(id).cloned()


This also ended up causing issues, I'm not sure it's worth saving the memory in our tests, but maybe in other tools that may use this? not sure

I think it is fine. Pinging @ethDreamer since he wrote this part initially

realbigsean · 2023-06-28T20:37:21Z

        // each field element contained in the blob is < BLS_MODULUS
-        for i in 0..FIELD_ELEMENTS_PER_BLOB {
-            blob_bytes[i * BYTES_PER_FIELD_ELEMENT + BYTES_PER_FIELD_ELEMENT - 1] = 0;
+        for i in 0..T::Kzg::FIELD_ELEMENTS_PER_BLOB {


The blob endianness switch necessitated this

pawanjay176

Looking good 🚀 Just a couple of questions

pawanjay176 · 2023-06-29T01:44:57Z

 hex = "0.4.2"
 ethereum_hashing = "1.0.0-beta.2"
-c-kzg = {git = "https://github.com/ethereum/c-kzg-4844", rev = "fd24cf8e1e2f09a96b4e62a595b4e49f046ce6cf" }
+c-kzg = { git = "https://github.com/michaelsproul/c-kzg-4844", rev = "9c47e976fe1133148742866c0ad10568ce182950" , features = ["mainnet-spec"]}


Can point this to the main repo now since Michael's PR got merged

pawanjay176 · 2023-06-29T02:39:51Z


+    pub fn random<R: Rng>(rng: &mut R) -> Self {
+        let mut blob_bytes = Vec::with_capacity(T::Kzg::BYTES_PER_BLOB);
+        rng.fill_bytes(&mut blob_bytes);


This could end up being an invalid blob. You would need to do something like this
https://github.com/ethereum/c-kzg-4844/blob/main/bindings/rust/src/bindings/mod.rs#L544C5-L553C6

pawanjay176 · 2023-06-29T02:39:58Z


+    pub fn random<R: Rng>(rng: &mut R) -> Self {
+        let mut blob_bytes = Vec::with_capacity(T::Kzg::BYTES_PER_BLOB);
+        rng.fill_bytes(&mut blob_bytes);


This could end up being an invalid blob. You would need to do something like this
https://github.com/ethereum/c-kzg-4844/blob/main/bindings/rust/src/bindings/mod.rs#L544C5-L553C6

good shout!

I realized the test where I'm using this doesn't interpret blob bytes at all, so I'm opting to just use BlobSidecar::default there since the blob size is fixed. I did a litter refactor here to make the code you linked a bit more accessible to anyone trying to test with random valid blobs.

pawanjay176 · 2023-06-29T03:27:01Z

+    let shanghai_time = spec.capella_fork_epoch.map(|epoch| {
+        HARNESS_GENESIS_TIME + spec.seconds_per_slot * T::slots_per_epoch() * epoch.as_u64()
+    });
+    let deneb_time = spec.deneb_fork_epoch.map(|epoch| {


A little pedantic here, but should this be cancun?

pawanjay176 · 2023-06-29T03:41:45Z

    pub fn get_blobs_bundle(&mut self, id: &PayloadId) -> Option<BlobsBundleV1<T>> {
-        // remove it to free memory
-        self.blobs_bundles.remove(id)
+        self.blobs_bundles.get(id).cloned()


I think it is fine. Pinging @ethDreamer since he wrote this part initially

pawanjay176 · 2023-06-29T03:42:53Z

-        if self.blocks.contains_key(&block.block_hash()) {
-            return Err(format!("{:?} is already known", block.block_hash()));
-        } else if block.parent_hash() != ExecutionBlockHash::zero()
+        if block.parent_hash() != ExecutionBlockHash::zero()


Worth adding a comment here just in case we add another test in the future that requires rejecting duplicates?

pawanjay176 · 2023-06-29T03:54:45Z

+                snapshot_cache.get_cloned(block_parent_root, CloneConfig::committee_caches_only())
+            })
+        {
+            if snapshot.beacon_state.slot() == blob_slot {


Wondering why we need to clone the state if we don't need to advance it in this case.

Calling get_state_for_block_processing evicts the state from the snapshot cache which we don't want because we want to use the same state for block and all blob verification. This is actually making me realize we want to make sure we aren't evicting the state during block processing or we will always get cache misses here 🤔

tree states would end up making this a lot simpler... i'm not really sure how we should be handling this here..

pawanjay176 · 2023-06-29T03:55:47Z

-                    .get_beacon_proposer_index(blob_slot, &chain.spec)?,
-                snapshot.beacon_state.fork(),
-            )
+        if let Some(mut snapshot) = chain


Trying to understand why the snapshot_cache is better than the cacnonical head in this case. Is it because it has less lock contention?
P.S. I don't remember why I used the cached_head in the first place 😂

So we need to make sure we are getting shuffling based on the parent state of this blob we're verifying, and our cached head might not be the one this blob builds on, but we still need to verify it. So we could use the cached head and fall back to the snapshot cache if the cached head isn't what the blob builds on but I figured this was simpler.

pawanjay176 · 2023-06-29T04:04:48Z

+                    &chain.spec,
+                )?;
+                (
+                    state.get_beacon_proposer_index(blob_slot, &chain.spec)?,


Should we prime the proposer cache here as well?

Yes we should... I'm wondering if we should separate this state-loading issue into a new task/pr.. what's here should work but is probably pretty suboptimal in practice. I'm not sure we should block merging this on this

Sounds good.

pawanjay176

I'm happy to merge this and handle the blob states loading in another PR

realbigsean · 2023-06-29T19:35:26Z

Thanks for the review @pawanjay176 !

realbigsean and others added 30 commits March 28, 2023 18:29

some blob reprocessing work

8d80200

remove ForceBlockLookup

9642ec0

reorder enum match arms in sync manager

b78a6e8

a lot more reprocessing work

8403402

impl logic for triggerng blob lookups along with block lookups

a17405a

Merge branch 'deneb-free-blobs' of https://github.com/sigp/lighthouse …

22694d1

…into some-blob-reprocessing-work

deal with rpc blobs in groups per block in the da checker. don't cach…

6f12df3

…e missing blob ids in the da checker.

make single block lookup generic

38e0994

Merge branch 'deneb-free-blobs' of https://github.com/sigp/lighthouse …

66e09f4

…into some-blob-reprocessing-work

more work

25ff6e8

Merge branch 'deneb-free-blobs' of https://github.com/sigp/lighthouse …

2f8c885

…into some-blob-reprocessing-work

add delayed processing logic and combine some requests

8618c30

start fixing some compile errors

195d802

fix compilation in main block lookup mod

0ad9fdf

much work

374ec48

get things compiling

c714249

Merge branch 'deneb-free-blobs' of https://github.com/sigp/lighthouse …

0cc1704

…into some-blob-reprocessing-work

parent blob lookups

bacec52

fix compile

3e854ae

revert red/stevie changes

d8ed811

Merge branch 'deneb-free-blobs' of https://github.com/sigp/lighthouse …

b2a4c4e

…into some-blob-reprocessing-work

Merge branch 'some-blob-reprocessing-work' of https://github.com/real…

269122f

…bigsean/lighthouse into some-blob-reprocessing-work

Merge branch 'deneb-free-blobs' of https://github.com/sigp/lighthouse …

03de817

…into some-blob-reprocessing-work

fix up sync manager delay message logic

93df0f5

add peer usefulness enum

381044a

should remove lookup refactor

b6531aa

consolidate retry error handling

274aba9

improve peer scoring during certain failures in parent lookups

0560b7d

improve retry code

1d18756

drop parent lookup if either req has a peer disconnect during download

76c09de

realbigsean and others added 9 commits June 27, 2023 17:04

fix lints

c81ef8c

Fix KZG linking issues

57f55d5

Merge pull request #36 from michaelsproul/devnet6-kzg-fix

03524ec

Merge branch 'deneb-free-blobs' of https://github.com/sigp/lighthouse …

2259190

…into devnet6

remove unused dep

8c9cd29

lockfile

49528c4

test fixes

d12db6f

remove dbgs

923baee

remove unwrap

372f97f

realbigsean commented Jun 28, 2023

View reviewed changes

realbigsean added 9 commits June 28, 2023 16:43

cleanup tx generator

3469283

small fixes

bde5987

fixing fixes

a515fd9

more self reivew

7d8106a

more self review

c2e045c

refactor genesis header initialization

c9c49e6

refactor mock el instantiations

b24cf80

fix compile

32a4e7e

fix network test, make sure they run for each fork

89306dd

pawanjay176 reviewed Jun 29, 2023

View reviewed changes

realbigsean mentioned this pull request Jun 29, 2023

Investigate why batch kzg proof verification with batch size 1 was failing #4446

Closed

pr feedback

afc4290

pawanjay176 approved these changes Jun 29, 2023

View reviewed changes

fix last test (hopefully)

e51ad1b

realbigsean merged commit adbb62f into sigp:deneb-free-blobs Jun 29, 2023

realbigsean mentioned this pull request Jun 29, 2023

Deneb Devnet 6 #4374

Closed

8 tasks

realbigsean deleted the devnet6 branch November 21, 2023 16:13

Uh oh!

Conversation

realbigsean commented Jun 15, 2023

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawanjay176 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawanjay176 left a comment

Choose a reason for hiding this comment

Uh oh!

realbigsean commented Jun 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants