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

Uses aptos-core x25519-dalek library with relaxed zeroize dep #9354

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

CapCap
Copy link
Contributor

@CapCap CapCap commented Jul 28, 2023

Description

So there’s this fun thing in rust-landia where the cryptographic library we use - dalek-cryptography/x25519-dalek#92 - where they’ve pinned a version of their zeroize dependency to a
minor version, and have refused to backport relaxing that constraint for almost a year now. Instead they’ve been focused on their V2, which is in alpha. Other libraries have moved on from relying (at this
point) on the very old version of zeroize , and more and more often you see zeroize > 1.4 . This creates a fun incompatibility!
A few of us have run into this now, trying to use libraries from aptos core in other services. This creates an interesting situation: either we have to use an alpha (pre-release- currently on rc3) version of a
core cryptography library (which sounds very sketch), or we have to use much older versions of other dependencies (if any such exist). It’s affected indexer (twice now, in different services), and IC: it’s safe
to assume any rust users in our ecosystem would also run into the same issues.

Test Plan

Existing build + tests

@CapCap CapCap force-pushed the internal_zeroize_v1 branch 2 times, most recently from 22b7d23 to b102673 Compare July 28, 2023 02:04
@CapCap CapCap requested a review from davidiw July 28, 2023 04:33
@alinush
Copy link
Contributor

alinush commented Jul 28, 2023

It looks like we will slowly be forking every dalek library due to their poor maintenance.

We also had to fork merlin.

We should discuss a workflow / process for keeping these forks up to date, no?

Copy link
Contributor

@mstraka100 mstraka100 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 one reservation. Originally with the forked repo I patched zeroize, then force overwrote main in the fork with the patch to get the additional write protections the main branch has (so that main in the fork is now the latest stable release of x25519-dalek with the zeroize update). My intuition is that using main in the fork instead of a branch would be safer for this reason.

@rustielin rustielin added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Jul 28, 2023
@CapCap CapCap enabled auto-merge (squash) July 28, 2023 21:20
@CapCap
Copy link
Contributor Author

CapCap commented Jul 28, 2023

@mstraka100 merging to unblock; I was unable to force push the other version to main

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite compat success on aptos-node-v1.5.1 ==> 47392daf97a83dc1d0d15062e34a7b52242534f5

Compatibility test results for aptos-node-v1.5.1 ==> 47392daf97a83dc1d0d15062e34a7b52242534f5 (PR)
1. Check liveness of validators at old version: aptos-node-v1.5.1
compatibility::simple-validator-upgrade::liveness-check : committed: 4629 txn/s, latency: 6986 ms, (p50: 7400 ms, p90: 9600 ms, p99: 11100 ms), latency samples: 171300
2. Upgrading first Validator to new version: 47392daf97a83dc1d0d15062e34a7b52242534f5
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1770 txn/s, latency: 15727 ms, (p50: 19100 ms, p90: 21700 ms, p99: 22300 ms), latency samples: 92040
3. Upgrading rest of first batch to new version: 47392daf97a83dc1d0d15062e34a7b52242534f5
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1790 txn/s, latency: 16433 ms, (p50: 18900 ms, p90: 22500 ms, p99: 22600 ms), latency samples: 93100
4. upgrading second batch to new version: 47392daf97a83dc1d0d15062e34a7b52242534f5
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3521 txn/s, latency: 9084 ms, (p50: 10500 ms, p90: 12000 ms, p99: 12500 ms), latency samples: 137320
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> 47392daf97a83dc1d0d15062e34a7b52242534f5 passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 47392daf97a83dc1d0d15062e34a7b52242534f5

two traffics test: inner traffic : committed: 6472 txn/s, latency: 6041 ms, (p50: 5700 ms, p90: 7800 ms, p99: 11600 ms), latency samples: 2809020
two traffics test : committed: 100 txn/s, latency: 3000 ms, (p50: 2900 ms, p90: 3600 ms, p99: 5000 ms), latency samples: 1720
Max round gap was 1 [limit 4] at version 1349859. Max no progress secs was 3.8291569 [limit 10] at version 1349859.
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on aptos-node-v1.5.1 ==> 47392daf97a83dc1d0d15062e34a7b52242534f5

Compatibility test results for aptos-node-v1.5.1 ==> 47392daf97a83dc1d0d15062e34a7b52242534f5 (PR)
Upgrade the nodes to version: 47392daf97a83dc1d0d15062e34a7b52242534f5
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 4582 txn/s, latency: 7106 ms, (p50: 7700 ms, p90: 10200 ms, p99: 10700 ms), latency samples: 169560
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> 47392daf97a83dc1d0d15062e34a7b52242534f5 passed
Test Ok

@CapCap CapCap merged commit c1e330b into main Jul 28, 2023
82 of 93 checks passed
@CapCap CapCap deleted the internal_zeroize_v1 branch July 28, 2023 22:48
@alinush
Copy link
Contributor

alinush commented Jul 31, 2023

cc @gregnazario who recently had some issues with our merlin fork.

@gregnazario
Copy link
Contributor

This is a little problematic, every patch that we apply, requires uses of the Rust SDK to have to also apply the patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants