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

eth: Add sidecars validation to sender and recipient #2685

Closed

Conversation

ngotchac
Copy link
Contributor

@ngotchac ngotchac commented Sep 4, 2024

Some "sidecars" validation steps were missing in a few places still: the downloader when validating a Header, and the handler when delivering Blocks.

@buddh0
Copy link
Collaborator

buddh0 commented Sep 5, 2024

if distance between the target block and the latest block is larger than MinBlocksForBlobRequests,
then the blob data is not need, either download form others or deliver to others.

@buddh0
Copy link
Collaborator

buddh0 commented Sep 5, 2024

when download from others
the check logic is in func IsDataAvailable

@ngotchac ngotchac force-pushed the ngotchac/block-sidecars-checks branch from 65d1b64 to 7a4b540 Compare September 5, 2024 08:12
@ngotchac
Copy link
Contributor Author

ngotchac commented Sep 5, 2024

If the distance between the target block and the latest block is larger than MinBlocksForBlobRequests, then the blob data is not needed

I wasn't aware of that, thanks! I updated the PR so that it takes it into account. The reasoning here is just to make the same check for Sidecars() availability as we do for the block body in:

body := chain.GetBody(hash)
if body == nil {
    continue
}

and if we're missing a required Sidecars() for don't want for other peers to mark us as invalid ; it's better to just not respond to this request's block.

When downloading from others, the check logic is in func IsDataAvailable()

The issue is that currently, the IsDataAvailable() check when receiving blocks is only in func (bc *BlockChain) insertChain(...) (int, error) which returns an error when the Sidecars() part isn't valid, which eventually bubbles up to func (d *Downloader) LegacySync and drops the peer.

Having this check in func (q *queue) DeliverBodies -> validate will enable still importing valid blocks, without droping the peer if they don't have the Sidecars() available.

@ngotchac ngotchac force-pushed the ngotchac/block-sidecars-checks branch from 7a4b540 to 9ae3b8a Compare September 5, 2024 08:39
@@ -837,6 +841,9 @@ func (q *queue) DeliverBodies(id string, txLists [][]*types.Transaction, txListH
}

// do some sanity check for sidecar
if blobTxs != len(sidecars[index]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

when do full sync,
if distance between the target block and the latest block is larger than MinBlocksForBlobRequests, then the blob data is not need
this check will reject the target block in the wrong way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer as below, I think it's a good thing to not mark a peer as invalid if some sidecars are missing. I could add the check for MinBlocksForBlobRequests here though, WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same answer as below, I think it's a good thing to not mark a peer as invalid if some sidecars are missing. I could add the check for MinBlocksForBlobRequests here though, WDYT?

👌

@ngotchac
Copy link
Contributor Author

I'm closing this PR. After thinking about it, adding the check for MinBlocksForBlobRequests in the sender might not be necessary, and adding it in (q *queue) DeliverBodies -> validate isn't feasible since the queue doesn't know what the top of the chain should be.

It seems that issues related to sidecars not being delivered have vanished, at least for now...

@ngotchac ngotchac closed this Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants