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

fix: response checking from RequestManager for blobs #6755

Merged
merged 5 commits into from
Dec 12, 2024
Merged

Conversation

agnxsh
Copy link
Contributor

@agnxsh agnxsh commented Dec 10, 2024

Previously checkResponse for blobs could potentially return some erroneous results for certain values of idList. The algo went like:

proc checkResponse(idList: seq[BlobIdentifier],
                   blobs: openArray[ref BlobSidecar]): bool =
  if len(blobs) > len(idList):
    return false
  for blob in blobs:
    let block_root = hash_tree_root(blob.signed_block_header.message)
    var found = false
    for id in idList:
      if id.block_root == block_root and id.index == blob.index:
        found = true
        break
    if not found:
      return false
    blob[].verify_blob_sidecar_inclusion_proof().isOkOr:
      return false
  true

here, initially found is false, say we check the first elem of idList, we see it passes correctly through the if statement, we may see found as true and exit the inner loop, thereby NOT checking the other elements of the idList, else we straightway return false. hence, in any case we DON'T correctly iterate through idList and check it.

one simple attack, could be a situation where the seq of BlobSidecar has the correct inclusion proof, but any of the non 1st response element(s) is/are missing/incorrect, checkResponse regardless should still respond as true.

looking at all of this, here's an updated algo, this iterates through the idList and the responded seq using a common loop control variable, and efficiently jumps if only a subset of the request has been responded correctly, it performs this check using a binary search.

thereby, the final time complexity of this response checker drops from O(n^2) (if idList was correctly iterated), to O(n log n). this optimization should be more relevant for data columns later as well, as we would often ask for columns in the order of 50-60 of them, if we were a supernode.

Copy link

github-actions bot commented Dec 11, 2024

Unit Test Results

       12 files  ±0    1 822 suites  ±0   56m 8s ⏱️ - 1m 43s
  5 290 tests ±0    4 942 ✔️ ±0  348 💤 ±0  0 ±0 
29 425 runs  ±0  28 973 ✔️ ±0  452 💤 ±0  0 ±0 

Results for commit 1504745. ± Comparison against base commit 031d24f.

♻️ This comment has been updated with latest results.

@agnxsh agnxsh enabled auto-merge (squash) December 12, 2024 13:41
@agnxsh agnxsh merged commit 98a7f44 into unstable Dec 12, 2024
13 checks passed
@agnxsh agnxsh deleted the resp-check-fix branch December 12, 2024 15:37
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