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(eigen-client-extra-features): merge main #396

Merged

Conversation

juan518munoz
Copy link

@juan518munoz juan518munoz commented Feb 10, 2025

What ❔

This PR solves conflicts with latest changes from main branch.

⚠️ DO NOT SQUASH ⚠️

Why ❔

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zkstack dev fmt and zkstack dev lint.

StanislavBreadless and others added 16 commits February 4, 2025 14:19
## What ❔

<!-- What are the changes this PR brings about? -->
<!-- Example: This PR adds a PR template to the repo. -->
<!-- (For bigger PRs adding more context is appreciated) -->

## Why ❔

<!-- Why are these changes done? What goal do they contribute to? What
are the principles behind them? -->
<!-- Example: PR templates ensure PR reviewers, observers, and future
iterators are in context about the evolution of repos. -->

## Checklist

<!-- Check your PR fulfills the following items. -->
<!-- For draft PRs check the boxes as you complete them. -->

- [ ] PR title corresponds to the body of PR (we generate changelog
entries from PRs).
- [ ] Tests for the changes have been added / updated.
- [ ] Documentation comments have been added / updated.
- [ ] Code has been formatted via `zkstack dev fmt` and `zkstack dev
lint`.
## What ❔

This PR removes heavy transitive dependencies from `zksync_utils` by:
* Moving `ManagedTasks` into a separate crate (ideally needs to be
removed in favor of node_framework but unfortunately still used by
prover and contract-verifier)
* Deleting custom-written retry logic for reqwest. The only place that
was using was `RegionFetcher` from prover utils. I migrated it to
`reqwest-retry` instead (Cargo.lock diff looks bigger than it actually
is - almost every new crate is just some wasm-specific stuff that
doesn't matter).

## Why ❔

Depending on `zksync_contracts`, `zksync_multivm` and other seemingly
core crates pulls in a lot of transitive dependencies right now
(`reqwest`, `sentry`, `opentelemetry`, `openssl` etc)

## Checklist

<!-- Check your PR fulfills the following items. -->
<!-- For draft PRs check the boxes as you complete them. -->

- [x] PR title corresponds to the body of PR (we generate changelog
entries from PRs).
- [ ] Tests for the changes have been added / updated.
- [ ] Documentation comments have been added / updated.
- [x] Code has been formatted via `zkstack dev fmt` and `zkstack dev
lint`.
)

Fixes the `gas_per_pubdata_limit` field calculation. Right now, we
provide the value we get from deriving the existing batch input, which
is most likely lower than limit provided in fee estimation request.
Given that we scale the overall fee, it should be safe to scale the
limit as well, as long as we don't go beyond the limit supplied in the
tx.

Providing the limit as-is without scaling can be very flaky: transaction
risks to be left in the mempool over minor `gas_per_pubdata` changes.
## What ❔

<!-- What are the changes this PR brings about? -->
<!-- Example: This PR adds a PR template to the repo. -->
<!-- (For bigger PRs adding more context is appreciated) -->

## Why ❔

<!-- Why are these changes done? What goal do they contribute to? What
are the principles behind them? -->
<!-- Example: PR templates ensure PR reviewers, observers, and future
iterators are in context about the evolution of repos. -->

## Checklist

<!-- Check your PR fulfills the following items. -->
<!-- For draft PRs check the boxes as you complete them. -->

- [ ] PR title corresponds to the body of PR (we generate changelog
entries from PRs).
- [ ] Tests for the changes have been added / updated.
- [ ] Documentation comments have been added / updated.
- [ ] Code has been formatted via `zkstack dev fmt` and `zkstack dev
lint`.
matter-labs#3452)

## What ❔

Changes how `contractAddress` is assigned for transaction receipts. With
these changes, it is assigned for EVM deployment transactions (ones with
`to == None`) and EraVM deployments (i.e., calls to
`ContractDeployer.{create, create2, createAccount, create2Account}`)
regardless of whether the transaction succeeds.

## Why ❔

Improves EVM compatibility.

## Checklist

- [x] PR title corresponds to the body of PR (we generate changelog
entries from PRs).
- [x] Tests for the changes have been added / updated.
- [x] Documentation comments have been added / updated.
- [x] Code has been formatted via `zkstack dev fmt` and `zkstack dev
lint`.
`zksync_contracts` is some spilled spaghetti.
## What ❔

<!-- What are the changes this PR brings about? -->
<!-- Example: This PR adds a PR template to the repo. -->
<!-- (For bigger PRs adding more context is appreciated) -->

## Why ❔

<!-- Why are these changes done? What goal do they contribute to? What
are the principles behind them? -->
<!-- Example: PR templates ensure PR reviewers, observers, and future
iterators are in context about the evolution of repos. -->

## Checklist

<!-- Check your PR fulfills the following items. -->
<!-- For draft PRs check the boxes as you complete them. -->

- [ ] PR title corresponds to the body of PR (we generate changelog
entries from PRs).
- [ ] Tests for the changes have been added / updated.
- [ ] Documentation comments have been added / updated.
- [ ] Code has been formatted via `zkstack dev fmt` and `zkstack dev
lint`.
Currently, data is in format mode, which strips away a lot of the
troubleshooting capabilities. This change marks error as Debug, instead
of Display.

It will turn entries like:
```
error sending request for url (http://example.invalid/)
```
into:
```
reqwest::Error { kind: Request, url: "http://example.invalid/", source: hyper_util::client::legacy::Error(Connect, ConnectError("dns error", Custom { kind: Uncategorized, error: "failed to lookup address information: Name or service not known" })) }
```
## What ❔

Made sure that we only have one version of jsonrpsee in our dependencies
now and that version is latest (new version is also a bit lighter as
they finally stopped depending on wasm libraries for non-wasm targets).
Had to adapt to breaking changes in `secrecy` as a side effect.

## Why ❔

<!-- Why are these changes done? What goal do they contribute to? What
are the principles behind them? -->
<!-- Example: PR templates ensure PR reviewers, observers, and future
iterators are in context about the evolution of repos. -->

## Checklist

<!-- Check your PR fulfills the following items. -->
<!-- For draft PRs check the boxes as you complete them. -->

- [x] PR title corresponds to the body of PR (we generate changelog
entries from PRs).
- [ ] Tests for the changes have been added / updated.
- [ ] Documentation comments have been added / updated.
- [x] Code has been formatted via `zkstack dev fmt` and `zkstack dev
lint`.
## What ❔

This PR removes a few (effectively) unused dependencies from
`zksync_types` and hides a few more behind features flags:
* `protobuf` enables `prost`, `zksync_protobuf`, `zksync_protobuf_build`
(only needed because of `ProtoFmt` impls for couple of snapshot types)
* `contract-verification` enables `ciborium`

Alternatively to features I am open to just moving those types to a
separate crate as I would consider them to be "internal". Maybe
something like `zksync_internal_types`.

One open question: do we need `bigdecimal`? Its only usage is
[this](https://github.com/matter-labs/zksync-era/blob/main/core/lib/types/src/fee_model.rs#L289-L294)
but the calculation doesn't seem to require bigdecimal at all. I would
get rid of it too if there are no objections.

## Why ❔

zksync_types is widely used outside of zksync-era and should ideally be
a lot thinner than it is right now

## Checklist

<!-- Check your PR fulfills the following items. -->
<!-- For draft PRs check the boxes as you complete them. -->

- [x] PR title corresponds to the body of PR (we generate changelog
entries from PRs).
- [x] Tests for the changes have been added / updated.
- [ ] Documentation comments have been added / updated.
- [x] Code has been formatted via `zkstack dev fmt` and `zkstack dev
lint`.
## What ❔

Leftovers after matter-labs#3574 

## Why ❔

Even less dependencies

## Checklist

<!-- Check your PR fulfills the following items. -->
<!-- For draft PRs check the boxes as you complete them. -->

- [x] PR title corresponds to the body of PR (we generate changelog
entries from PRs).
- [x] Tests for the changes have been added / updated.
- [ ] Documentation comments have been added / updated.
- [x] Code has been formatted via `zkstack dev fmt` and `zkstack dev
lint`.
A couple of indices accumulated over time, parts of old prover and
different setups of house keeper and PJM.

This PR removes unused indices and adds only bare minimum.

Indices left after this PR:
- idx_prover_jobs_fri_get_next_job - used for most heavy query: get next
job
- prover_jobs_fri_composite_index - common index, used across prover job
monitor & witness generators (leaf, node, scheduler)
- prover_jobs_fri_l1_batch_number_is_node_final_proof_status - used for
recursion tip to collect all node proof data
- prover_jobs_fri_status_processing_started_at_idx - used for requeuing
stuck jobs
@juanbono
Copy link

Please check that we are using SecretString instead of Secret<String> on the extra-features branch

@juan518munoz juan518munoz merged commit bdcb268 into eigen-client-extra-features Feb 10, 2025
9 of 28 checks passed
@juan518munoz juan518munoz deleted the eigen-client-extra-features-merge-main branch February 10, 2025 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.