search

chainwayxyz / citrea

Citrea, Bitcoin's First ZK Rollup 🍊🍋
https://citrea.xyz
GNU General Public License v3.0
129 stars 26 forks source link

Convert dyn to generics in `verified_blob_content #1517

Closed yaziciahmet closed 3 days ago

yaziciahmet commented 4 days ago

Description

Replaces dyn with generics as its slightly more efficient.

Linked Issues

Testing

Describe how these changes were tested. If you've added new features, have you added unit tests?

Docs

Describe where this code is documented. If it changes a documented interface, have the docs been updated?

codecov[bot] commented 3 days ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 76.3%. Comparing base (0494407) to head (e1994f5). Report is 2 commits behind head on nightly.

Additional details and impacted files | [Files with missing lines](https://app.codecov.io/gh/chainwayxyz/citrea/pull/1517?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=chainwayxyz) | Coverage Δ | | |---|---|---| | [crates/bitcoin-da/src/verifier.rs](https://app.codecov.io/gh/chainwayxyz/citrea/pull/1517?src=pr&el=tree&filepath=crates%2Fbitcoin-da%2Fsrc%2Fverifier.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=chainwayxyz#diff-Y3JhdGVzL2JpdGNvaW4tZGEvc3JjL3ZlcmlmaWVyLnJz) | `46.6% <100.0%> (-39.4%)` | :arrow_down: | ... and [10 files with indirect coverage changes](https://app.codecov.io/gh/chainwayxyz/citrea/pull/1517/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=chainwayxyz)
kpp commented 3 days ago

It's questionable since &dyn generates less code and generics monomorphize code for every unique T.

I would really love to have a test for prover/verifier to calculate exact cycle count for a block with a fixed content inside. It will help us a lot in cases like this.

Anyway, I am not against this PR.

rakanalh commented 3 days ago

It's questionable since &dyn generates less code and generics monomorphize code for every unique T.

I would really love to have a test for prover/verifier to calculate exact cycle count for a block with a fixed content inside. It will help us a lot in cases like this.

Anyway, I am not against this PR.

This would firstly affect the compile time aspect of the code & binary size. Secondly, I think that using generics would allow the compiler to better optimize the code after monomorphization takes place.

yaziciahmet commented 3 days ago

I also optimized the verified_blob_content method in this pr:

  1. removed clone, as the blob is already owned byte iterator, we can just return the reference
  2. no need for advance, as full_data method is already called in native, and the verified data is populated in CountedBufReader.

Step 2 has something unnecessary that is happenning right now. current impl of BlobWithSender is that it has its blob as CountedBufReader, which holds 1 reader, and 1 vec which is being read into, and anytime advance (or full_data) is called, it reads from the reader into the vec. the thing is, we always use another vector as the reader into this CountedBufReader, meaning that we just pass 2x the same data unnecessarily into the zk, and we have no partial reads whatsoever. we can just replace CountedBufReader with vec. but this is relatively tricky to do since BlobWithSender is part of the trait type definition of the BitcoinSpec, and versioning is going to be very uncomfortable.