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

feat(da): improvements in avail da with availability check #1357

Open
wants to merge 55 commits into
base: main
Choose a base branch
from

Conversation

srene
Copy link
Contributor

@srene srene commented Feb 1, 2025

PR Standards

  • This PR objective is the following:
  • It refactors Avail DA using new avail-go-sdk
  • It adds blob commitment to da path, using blockhash and sequencer da address.
  • It implements checkbatchavailability()

Opening a pull request should be able to meet the following requirements

--

PR naming convention: https://hackmd.io/@nZpxHZ0CT7O5ngTp0TP9mg/HJP_jrm7A


Close #XXX

<-- Briefly describe the content of this pull request -->

For Author:

  • Targeted PR against correct branch
  • included the correct type prefix in the PR title
  • Linked to Github issue with discussion and accepted design
  • Targets only one github issue
  • Wrote unit and integration tests
  • All CI checks have passed
  • Added relevant godoc comments

For Reviewer:

  • confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • confirmed all author checklist items have been addressed

After reviewer approval:

  • In case targets main branch, PR should be squashed and merged.
  • In case PR targets a release branch, PR should be rebased.

@srene srene requested a review from a team as a code owner February 1, 2025 11:16
@srene srene changed the title feat(da): availability check and blob hash in da path added feat(da): improvements in avail da with availability check Feb 1, 2025
@srene srene changed the base branch from main to srene/1342-da-path February 1, 2025 11:18
omritoptix
omritoptix previously approved these changes Feb 3, 2025
Copy link
Contributor

@omritoptix omritoptix left a comment

Choose a reason for hiding this comment

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

approved with a question about batch unmrashalling

@@ -239,7 +259,10 @@ func (c *DataAvailabilityLayerClient) RetrieveBatches(daPath string) da.ResultRe
data = data[1:]
Copy link
Contributor

Choose a reason for hiding this comment

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

didn't we say we don't go byte by byte cause the intrinsic should be correct? i.e if we fail first byte assuming it's our intrinsic than there is a problem?

Copy link
Contributor Author

@srene srene Feb 4, 2025

Choose a reason for hiding this comment

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

i think this is the avoid the issue of extra bytes appended to the beginning of the blob (not added in this pr). the extra bytes are not part of the blob submitted, so its correct to disregard them.
what i added is, once an unmarshable blob is found, check the hash of it is the same compared to what was submitted. the intrinsic only provides appid info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in any case its been replaced by new avail-go-sdk and filtered by signer. so only submitted blobs by sequencer are returned.

Copy link
Contributor

@omritoptix omritoptix left a comment

Choose a reason for hiding this comment

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

not sure about the batch unmrashaling bytes traversing. afair we said the intrinsic should indicate it.

Copy link
Contributor

@omritoptix omritoptix left a comment

Choose a reason for hiding this comment

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

not sure about the batch unmrashaling bytes traversing. afair we said the intrinsic should indicate it.

Base automatically changed from srene/1342-da-path to main February 4, 2025 09:51
@omritoptix omritoptix dismissed their stale review February 4, 2025 09:51

The base branch was changed.

@srene srene marked this pull request as draft February 6, 2025 11:55
@srene srene marked this pull request as ready for review February 6, 2025 18:14
@srene srene requested a review from omritoptix February 6, 2025 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants