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

Upgrade dependencies #3625

Merged
merged 15 commits into from
Mar 7, 2022
Merged

Upgrade dependencies #3625

merged 15 commits into from
Mar 7, 2022

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Feb 23, 2022

Motivation

Zebra should use the newest versions of its dependencies.

Solution

I used cargo upgrade --workspace to upgrade all dependencies. This caused a lot of errors during the compilation. Here's a list containing the diffs of the problematic bumps:

-pin-project = "0.4.29"
+pin-project = "1.0.10"

-bitvec = "0.22"
+bitvec = "1.0.0"

-halo2 = "=0.1.0-beta.1"
+halo2 = "0.1.0-beta.2"

-ripemd160 = "0.9"
+ripemd160 = "0.10.0"

-secp256k1 = { version = "0.21.2", features = ["serde"] }
+secp256k1 = { version = "0.21.3", features = ["serde"] }

-sha2 = { version = "0.9.9", features=["compress"] }
+sha2 = { version = "0.10.2", features = ["compress"] }

-proptest = { version = "0.10.1", optional = true }
+proptest = { version = "1.0.0", optional = true }

-proptest = "0.10.1"
+proptest = "1.0.0"

-rand07 = { package = "rand", version = "0.7" }
+rand07 = { package = "rand", version = "0.8.5" }

-metrics = "0.17.1"
+metrics = "0.18.0"

-arti-client = { version = "0.0.2", optional = true }
+arti-client = { version = "0.0.4", optional = true }

-tor-rtcompat = { version = "0.0.2", optional = true }
+tor-rtcompat = { version = "0.0.4", optional = true }

-color-eyre = "0.5.11"
+color-eyre = "0.6.0"

-tracing-subscriber = "0.2.25"
+tracing-subscriber = "0.3.9"

-tracing-error = "0.1.2"
+tracing-error = "0.2.0"

-abscissa_core = "0.5"
+abscissa_core = "0.6.0"

-gumdrop = "0.7"
+gumdrop = "0.8.0"

-color-eyre = { version = "0.5.11", features = ["issue-url"] }
+color-eyre = { version = "0.6.0", features = ["issue-url"] }

-tracing-flame = "0.1.0"
+tracing-flame = "0.2.0"

-tracing-journald = "0.1.0"
+tracing-journald = "0.2.3"

-tracing-subscriber = { version = "0.2.25", features = ["tracing-log"] }
+tracing-subscriber = { version = "0.3.9", features = ["tracing-log"] }

-metrics-exporter-prometheus = "0.7.0"
+metrics-exporter-prometheus = "0.8.0"

-sentry-tracing = "0.23.0"
+sentry-tracing = "0.24.3"

-abscissa_core = { version = "0.5", features = ["testing"] }
+abscissa_core = { version = "0.6.0", features = ["testing"] }


Every other dependency was upgraded to the newest version. The way I produced the list above was that I ran cargo upgrade --workspace, and then I was manually identifying the bumps that were causing errors and reverting them until I got a clean build and all tests were passing.

Will close #3115.

Review

Anyone can review.

@upbqdn upbqdn marked this pull request as draft February 23, 2022 23:08
@upbqdn upbqdn added A-dependencies Area: Dependency file updates C-enhancement Category: This is an improvement C-security Category: Security issues P-Low ❄️ labels Feb 23, 2022
@upbqdn
Copy link
Member Author

upbqdn commented Feb 23, 2022

I wasn't looking for the last compatible version of each dependency because I didn't find a quick way to automate this process. I think it would be tedious to identify such versions by hand, and we should switch to the newest versions anyway at some point. Is the latter assumption correct?

I'm not sure what's meant by this statement from the ticket:

Revert any dependency changes that cause a lot of duplicates (particularly with tower, tracing, and orchard)

In particular, how do I identify the duplicates?

I labeled the PR as a draft for now.

@teor2345
Copy link
Contributor

I wasn't looking for the last compatible version of each dependency because I didn't find a quick way to automate this process. I think it would be tedious to identify such versions by hand, and we should switch to the newest versions anyway at some point. Is the latter assumption correct?

Yep, this seems like a lot of work.

Let's delay this decision until we have time to work on tricky dependency upgrades. (We might also decide to specify the minimum compatible version in library crates, and the latest in the lock file for binaries. Because this makes libraries easier for non-zebra projects to use.)

I'm not sure what's meant by this statement from the ticket:

Revert any dependency changes that cause a lot of duplicates (particularly with tower, tracing, and orchard)

In particular, how do I identify the duplicates?

Use cargo deny check bans or this CI job:
https://github.com/ZcashFoundation/zebra/runs/5311535602?check_suite_focus=true

Then for each duplicate, work out if the crate:

  • needs a single version to function correctly: metrics, tracing, sentry, tokio
  • needs a single version for consensus compatibility: maybe blake2 ?
  • is small and can be ignored in deny.toml
  • is large and should be de-duplicated

We try to de-duplicate crates where possible. Usually it just takes a version bump of another crate, or a version revert.

@teor2345
Copy link
Contributor

I think we're leaving the blake2 upgrades until #2982, with the rest of the testnet cryptography upgrades.

@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #3625 (b716627) into main (4cb0f36) will decrease coverage by 1.00%.
The diff coverage is n/a.

❗ Current head b716627 differs from pull request most recent head bd23665. Consider uploading reports for the commit bd23665 to get more accurate results

@@            Coverage Diff             @@
##             main    #3625      +/-   ##
==========================================
- Coverage   80.06%   79.06%   -1.01%     
==========================================
  Files         290      290              
  Lines       32850    33247     +397     
==========================================
- Hits        26302    26286      -16     
- Misses       6548     6961     +413     

@upbqdn
Copy link
Member Author

upbqdn commented Mar 2, 2022

I've reverted the following dependencies because of duplicates.

  • Needs a single version for consensus compatibility:

    • -blake2s_simd = "1.0.0"
      +blake2s_simd = "0.5.11"
  • Needs a single version to function correctly:

    • -metrics = "0.18.0"
      +metrics = "0.17.1"
    • -tracing-error = "0.2.0"
      +tracing-error = "0.1.2"
    • -tracing-subscriber = "0.3.9"
      +tracing-subscriber = "0.2.25"
    • -sentry = { version = "0.24.3", default-features = false, features = ["backtrace", "contexts", "reqwest", "rustls"] }
      +sentry = { version = "0.23.0", default-features = false, features = ["backtrace", "contexts", "reqwest", "rustls"] }

@upbqdn
Copy link
Member Author

upbqdn commented Mar 2, 2022

There are still duplicates. They seem to be caused by arti-client v0.0.2 and abscissa, but I didn't change the version of these crates. I'm trying to figure out why these duplicates occur, but no luck so far.

@upbqdn
Copy link
Member Author

upbqdn commented Mar 2, 2022

I noticed I'm not able to change abscissa to 0.5.1 or 0.5.0. cargo deny --workspace --all-features check bans still outputs abscissa_core v0.5.2.

@teor2345
Copy link
Contributor

teor2345 commented Mar 3, 2022

I noticed I'm not able to change abscissa to 0.5.1 or 0.5.0. cargo deny --workspace --all-features check bans still outputs abscissa_core v0.5.2.

It looks like we're almost there, I'll see what I can do to help here.

We might just need to ignore some duplicates.

@teor2345
Copy link
Contributor

teor2345 commented Mar 3, 2022

@upbqdn what do you think of the duplicate fixes in #3716?

Feel free to merge it into this PR.

* feat(actions)!: add full sync test (#3582)

* add(tests): full sync test

* fix(test): add build

* fix(deploy): escape double dashes '--' correctly

* fix(test): remove unexpected --no-capture arg

error: Found argument '--nocapture' which wasn't expected, or isn't valid in this context

* refactor(docker): use default executable as entrypoint

* refactor(startup): add a custom entrypoint

* fix(test): add missing TEST_FULL_SYNC variable

* test(timeout): use the biggest machine

* fix

* fix(deploy): use latest successful image

* typo

* refactor(docker): generate config file at startup

* revert(build): changes were made to docker

* fix(docker): send variables correctly to the entrypoint

* test different conf file approach

* fix(env): add RUN_TEST env variable

* ref: use previous approach

* fix(color): use environment variable

* fix(resources): use our normal machine size

* fix(ci): double CPU and RAM for full sync test

* fix(test): check for zebrad test output in the correct order

The mempool is only activated once, so we must check for that log first.
After mempool activation, the stop regex is logged at least once.
(It might be logged before as well, but we can't rely on that.)

When checking that the mempool didn't activate,
wait for the `zebrad` command to exit,
then check the entire log.

* fix(ci): run full sync test with full compiler optimisations

* fix(tests): reintroduce tests and run full sync on approval

* fix(tests): reduce the changelog

Co-authored-by: teor <teor@riseup.net>

* fix(ci): update CI job path triggers (#3692)

* ci(test): re-run tests when snapshot data changes

* fix(ci): rebuild state when disk format changes

* fix(ci): rebuild rust docs when code or dependencies change

* doc(ci): explain why we run jobs when files change

Co-authored-by: Gustavo Valverde <gustavo@iterativo.do>

* fix(build): use the right multistage target (#3700)

* fix(review): only assign one reviewer to general Rust reviews (#3708)

If we assign two teams, GitHub assigns two reviewers.

* fix(ci): change the color-eyre ignore to a tracing-subscriber ignore

* fix(ci): ignore duplicate darling dependencies

* doc(ci): remove an alternative resolution doc

Co-authored-by: Gustavo Valverde <gustavo@iterativo.do>
@upbqdn upbqdn marked this pull request as ready for review March 3, 2022 11:39
@upbqdn upbqdn requested review from a team as code owners March 3, 2022 11:39
@upbqdn upbqdn requested review from conradoplg and teor2345 and removed request for a team March 3, 2022 11:39
@teor2345 teor2345 removed the request for review from conradoplg March 3, 2022 22:15
teor2345
teor2345 previously approved these changes Mar 3, 2022
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@teor2345
Copy link
Contributor

teor2345 commented Mar 3, 2022

I opened PR #3726 for one of the CI failures, and the other is ticket #3712.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I'm going to merge this PR manually, because it's blocking other PRs like #3707.

And sometimes when we merge a new PR, there are conflicts with this PR.

I've tested it locally with PR #3755, and all the tests pass. (I didn't test the cached state or full sync.)

@teor2345
Copy link
Contributor

teor2345 commented Mar 7, 2022

I opened this PR for the integration test failure:

And this one for the rest of the test hangs:

@teor2345 teor2345 merged commit 2cc7e15 into main Mar 7, 2022
@teor2345 teor2345 deleted the upgrade-dependencies branch March 7, 2022 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates C-enhancement Category: This is an improvement C-security Category: Security issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to the latest compatible versions of all dependencies
2 participants