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

remove risky/heavy unused dependencies #4450

Closed
aoudiamoncef opened this issue Oct 5, 2023 · 8 comments · Fixed by #4451
Closed

remove risky/heavy unused dependencies #4450

aoudiamoncef opened this issue Oct 5, 2023 · 8 comments · Fixed by #4451
Assignees
Labels
enhancement New feature or request need spec This issue need more specification

Comments

@aoudiamoncef
Copy link
Contributor

aoudiamoncef commented Oct 5, 2023

Candidates to removal:

  • paw is not referenced in the codebase.
  • quiche is not a direct massa dependency but It's deeply integrated in PeerNet . CC @AurelienFT
  • rayon is used once in the codebase in:
pub fn verify_sigs_batch(ops: &[(Hash, Signature, PublicKey)]) -> Result<(), ProtocolError> {
    // if it's a small batch, use single-core verification
    if ops.len() <= SMALL_BATCH_LIMIT {
        return verify_signature_batch(ops).map_err(|_err| ProtocolError::WrongSignature);
    }

    // otherwise, use parallel batch verif

    // compute chunk size for parallelization
    let chunk_size = std::cmp::max(1, ops.len() / rayon::current_num_threads());
    // process chunks in parallel
    ops.par_chunks(chunk_size)
        .try_for_each(verify_signature_batch)
        .map_err(|_err| ProtocolError::WrongSignature)
}
  • paginate crate could be removed as we can remove pagination in get_stakers in JsonRPC API.
  • All dependencies should be forked officially (with massa namespace) to avoid bad surprises:
unsigned-varint = { version = "=0.7", "git" = "https://github.com/cyphar/unsigned-varint.git", "branch" = "nom6-errors" }
substruct = { git = "https://github.com/sydhds/substruct", branch = "main" }
machine = { git = "https://github.com/antifuchs/machine", "branch" = "fix-workspace-build" }
@aoudiamoncef aoudiamoncef changed the title remove risky/heavy dependencies that are not in use (eg. Quiche) @aoudiamoncef remove risky/heavy unused dependencies Oct 5, 2023
@aoudiamoncef aoudiamoncef added enhancement New feature or request need spec This issue need more specification labels Oct 5, 2023
@aoudiamoncef aoudiamoncef self-assigned this Oct 5, 2023
@aoudiamoncef aoudiamoncef mentioned this issue Oct 5, 2023
8 tasks
@aoudiamoncef
Copy link
Contributor Author

We can go beyond by disabling the default features in dependencies crate and choose only what we need.

See the complete output dep_tree_features.txt

@aoudiamoncef
Copy link
Contributor Author

cc @Leo-Besancon

@Leo-Besancon
Copy link
Contributor

Candidates to removal:

* `paw` is not referenced in the codebase.

We should indead remove it

* `quiche` is not a direct massa dependency but It's deeply integrated in `PeerNet` . CC @AurelienFT

I think we should clean it on the massa repo then, I don't see why not. (If we do need it in the future we just have to make sure we align the versions).

* `rayon` is used once in the codebase in:

Can you compare the alternatives? I see https://docs.rs/num_threads/latest/num_threads/index.html that could be lighter to depend on if it's the same behaviour.

* `paginate` crate could be removed as we can remove pagination in `get_stakers` in JsonRPC API.

If there are no bad consequences of this, go for it. But if we aim to support JsonRPC for a bit longer we should not "downgrade" it IMO.

* All  dependencies should be forked officially (with massa namespace) to avoid bad surprises:
unsigned-varint = { version = "=0.7", "git" = "https://github.com/cyphar/unsigned-varint.git", "branch" = "nom6-errors" }
substruct = { git = "https://github.com/sydhds/substruct", branch = "main" }
machine = { git = "https://github.com/antifuchs/machine", "branch" = "fix-workspace-build" }

Maybe it's not necessary for all of them, the .lock should prevent most trouble, no? Or maybe it's easier to just target a fixed rev, not sure.

@aoudiamoncef
Copy link
Contributor Author

aoudiamoncef commented Oct 9, 2023

@Leo-Besancon we can't remove rayonas it's used not only to get the number of threads but also to do the calculation.

@Leo-Besancon
Copy link
Contributor

Leo-Besancon commented Oct 9, 2023

@Leo-Besancon we can'"t remove rayonas it's used not only to get the number of threads but also to do the calculation.

Indeed I misread the code!

@aoudiamoncef
Copy link
Contributor Author

We have a last point to see with @sydhds about forking the 3 repositories above

@sydhds
Copy link
Contributor

sydhds commented Oct 9, 2023

We have a last point to see with @sydhds about forking the 3 repositories above

  • substruct is a crate I've written and it can be forked in Massa orga (would be nice to keep on contributing it upstream :))
  • machine can be forked in Massa orga too (used by Versioning)
  • unsigned-varint could use the (not verified) more up to date lib here: https://github.com/paritytech/unsigned-varint

@Leo-Besancon
Copy link
Contributor

We have a last point to see with @sydhds about forking the 3 repositories above

* substruct is a crate I've written and it can be forked in Massa orga (would be nice to keep on contributing it upstream :))

https://github.com/massalabs/substruct

* machine can be forked in Massa orga too (used by Versioning)

https://github.com/massalabs/machine/tree/fix-workspace-build

(I'll let you @aoudiamoncef merge to main if you think it's relevant to do so)

* unsigned-varint could use the (not verified) more up to date lib here: https://github.com/paritytech/unsigned-varint

@aoudiamoncef can you try and update to this one? Check if there is a release on crates, etc. We'll fork it if needed

@bors bors bot closed this as completed in edf4c1c Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request need spec This issue need more specification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants