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.

Sign up for GitHub

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

Jump to bottom

Avoid computing columns from EL blobs if block has already been imported #6816

Merged
merged 2 commits into from
Jan 22, 2025
Merged

Avoid computing columns from EL blobs if block has already been imported #6816

merged 2 commits into from
Jan 22, 2025

Conversation

jimmygchen
Copy link
Member

@jimmygchen jimmygchen commented Jan 17, 2025 •
edited
Loading

Issue Addressed

From PeerDAS local testing I noticed some errors with failing to send computed columns to receiver:

lighthouse/beacon_node/beacon_chain/src/fetch_blobs.rs

Line 252 in dd7591f

error!(log, "Failed to send computed data columns"; "error" => ?e);

I believe there is a race condition, the sequence:

  1. Fetch EL blobs triggered, received 4 blobs from the EL, triggered column computation
  2. All 8 custody columns received via gossip
  3. Columns written to db, block imported. (Column receiver dropped)
  4. Computed columns but fail to send as receiver was dropped.

We should not even try to compute the columns if we've already imported the block from gossip. This consumes extra resources and bandwidth and is unnecessary.

@jimmygchen jimmygchen added ready-for-review The code is ready for review das Data Availability Sampling labels Jan 17, 2025
@jimmygchen jimmygchen added the low-hanging-fruit Easy to resolve, get it before someone else does! label Jan 17, 2025
@jimmygchen jimmygchen force-pushed the do-not-compute-columns-block-imported branch from e09d3d1 to b3584ac Compare January 17, 2025 11:47
jimmygchen added a commit to jimmygchen/lighthouse that referenced this pull request Jan 17, 2025
@jimmygchen
Avoid computing columns from EL blobs if block has already been impor…
302e225 
…ted (sigp#6816)

Squashed commit of the following:

commit b3584ac
Author: Jimmy Chen <[email protected]>
Date:   Fri Jan 17 22:39:16 2025 +1100

    Avoid computing columns from EL blobs if block has already been imported.
@realbigsean realbigsean requested review from realbigsean and removed request for realbigsean January 17, 2025 23:28
@jimmygchen jimmygchen requested review from dapplion and removed request for dapplion January 18, 2025 13:59
dapplion
dapplion reviewed Jan 20, 2025
View reviewed changes
Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

We should address the error log if the receiver is dropped after import

@jimmygchen
Copy link
Member Author

We should address the error log if the receiver is dropped after import

I've downgraded the error to warn and added some comments here in the Fulu PR: https://github.com/sigp/lighthouse/pull/6795/files#diff-52c135e6abd229a6d90ebb9a954e3f28145ec85c311e95da86dd7a1c590e102fR251-R258

What do you propose the error handling to be? I think this PR should prevent this, and I think it's fine to just log it if it happens? If block has already been imported, then we just don't re-import and drop them .

@jimmygchen jimmygchen added bug Something isn't working das Data Availability Sampling and removed das Data Availability Sampling labels Jan 20, 2025
@michaelsproul
Copy link
Member

Agree debug log is better than a warning for the fail to send case

@jimmygchen
Copy link
Member Author

Downgraded to debug and also just return without publishing as the block is likely available via gossip.
0df2384

jimmygchen added a commit that referenced this pull request Jan 21, 2025
@jimmygchen
Avoid computing columns from EL blobs if block has already been impor…
c0e1597 
…ted (#6816)

Squashed commit of the following:

commit 0df2384
Author: Jimmy Chen <[email protected]>
Date:   Tue Jan 21 16:41:30 2025 +1100

    Downgrade a `warn` log to `debug` and update handling.

commit b3584ac
Author: Jimmy Chen <[email protected]>
Date:   Fri Jan 17 22:39:16 2025 +1100

    Avoid computing columns from EL blobs if block has already been imported.
@jimmygchen jimmygchen mentioned this pull request Jan 21, 2025
Draft
pawanjay176
pawanjay176 approved these changes Jan 21, 2025
View reviewed changes
Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

LGTM

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jan 21, 2025
@michaelsproul
Copy link
Member

@mergify queue

@mergify Mergify
Copy link

mergify bot commented Jan 22, 2025 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at f008b84

mergify bot added a commit that referenced this pull request Jan 22, 2025
@mergify
Merge of #6816
64c96ff
This was referenced Jan 22, 2025
Closed
Merged
@mergify mergify bot merged commit f008b84 into sigp:unstable Jan 22, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dapplion dapplion dapplion left review comments

@pawanjay176 pawanjay176 pawanjay176 approved these changes

@michaelsproul michaelsproul michaelsproul approved these changes

@realbigsean realbigsean Awaiting requested review from realbigsean

Assignees
No one assigned
Labels
bug Something isn't working das Data Availability Sampling low-hanging-fruit Easy to resolve, get it before someone else does! ready-for-merge This PR is ready to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jimmygchen @michaelsproul @pawanjay176 @dapplion