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

fix: anvil-zksync able to start even if the port is busy #542

Merged
merged 7 commits into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/checks.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ jobs:
name: lint
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4
- name: Install Rust
uses: actions-rust-lang/setup-rust-toolchain@v1
with:
Expand All @@ -39,7 +39,7 @@ jobs:
matrix:
os: [ubuntu-latest, macos-latest]
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4

- name: Install Rust
uses: actions-rust-lang/setup-rust-toolchain@v1
Expand All @@ -55,7 +55,7 @@ jobs:
tar -czf anvil-zksync-${{ matrix.os }}.tar.gz ./anvil-zksync*

- name: Upload artifact
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: anvil-zksync-${{ matrix.os }}.tar.gz
path: ./target/release/anvil-zksync-${{ matrix.os }}.tar.gz
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/e2e-docker.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ jobs:

steps:
- name: Checkout code
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/e2e-rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ jobs:
os: [ ubuntu-latest, macos-latest ]
name: e2e-rust
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4
with:
fetch-depth: 0

- name: Download artifacts
uses: actions/download-artifact@v3
uses: actions/download-artifact@v4
with:
name: anvil-zksync-${{ matrix.os }}.tar.gz

Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/e2e.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ jobs:
cache-dependency-path: 'e2e-tests/yarn.lock'

- name: Download artifacts
uses: actions/download-artifact@v3
uses: actions/download-artifact@v4
with:
name: anvil-zksync-${{ matrix.os }}.tar.gz

Expand Down
8 changes: 4 additions & 4 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ jobs:
needs: [extract-version]
steps:
- name: Checkout sources
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: Install Rust
uses: actions-rust-lang/setup-rust-toolchain@v1
Expand Down Expand Up @@ -99,7 +99,7 @@ jobs:
# This is required to share artifacts between different jobs
# =======================================================================
- name: Upload artifact
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: anvil-zksync-${{ needs.extract-version.outputs.VERSION }}-${{ matrix.arch }}.tar.gz
path: anvil-zksync-${{ needs.extract-version.outputs.VERSION }}-${{ matrix.arch }}.tar.gz
Expand All @@ -121,15 +121,15 @@ jobs:
steps:
# This is necessary for generating the changelog. It has to come before "Download Artifacts" or else it deletes the artifacts.
- name: Checkout sources
uses: actions/checkout@v3
uses: actions/checkout@v4
with:
fetch-depth: 0

# ==============================
# Download artifacts
# ==============================
- name: Download artifacts
uses: actions/download-artifact@v3
uses: actions/download-artifact@v4

# ==============================
# Create release draft
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/spec.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ jobs:
runs-on: ubuntu-latest
name: spec
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4
with:
submodules: "recursive"

- name: Download artifacts
uses: actions/download-artifact@v3
uses: actions/download-artifact@v4
with:
name: anvil-zksync-ubuntu-latest.tar.gz

Expand Down
4 changes: 4 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ lint-fix:
cd e2e-tests && yarn && yarn lint:fix && yarn fmt:fix
cargo clippy --fix
cargo fmt
cd e2e-tests-rust && cargo fmt --all
cd e2e-tests-rust && cargo clippy --fix
cd spec-tests && cargo fmt --all
cd spec-tests && cargo clippy --fix

# Run unit tests for Rust code
test:
Expand Down
22 changes: 13 additions & 9 deletions crates/api_server/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ impl NodeServerBuilder {
rpc
}

pub async fn build(self, addr: SocketAddr) -> NodeServer {
pub async fn build(self, addr: SocketAddr) -> Result<NodeServer, String> {
let cors_layers = tower::util::option_layer(self.cors_enabled.then(|| {
// `CorsLayer` adds CORS-specific headers to responses but does not do filtering by itself.
// CORS relies on browsers respecting server's access list response headers.
Expand All @@ -86,14 +86,18 @@ impl NodeServerBuilder {
)
.set_rpc_middleware(RpcServiceBuilder::new().rpc_logger(100));

let server = server_builder.build(addr).await.unwrap();
let local_addr = server.local_addr().unwrap();
let rpc = Self::default_rpc(self.node);
// `jsonrpsee` does `tokio::spawn` within `start` method, so we cannot invoke it here, as this method
// should only build the server. This way we delay the launch until the `NodeServer::run` is invoked.
NodeServer {
local_addr,
run_fn: Box::new(move || server.start(rpc)),
match server_builder.build(addr).await {
Ok(server) => {
let local_addr = server.local_addr().unwrap();
let rpc = Self::default_rpc(self.node);
// `jsonrpsee` does `tokio::spawn` within `start` method, so we cannot invoke it here, as this method
// should only build the server. This way we delay the launch until the `NodeServer::run` is invoked.
Ok(NodeServer {
local_addr,
run_fn: Box::new(move || server.start(rpc)),
})
}
Err(e) => Err(format!("Failed to bind to address {}: {}", addr, e)),
}
}
}
Expand Down
41 changes: 37 additions & 4 deletions crates/cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -317,10 +317,43 @@ async fn main() -> anyhow::Result<()> {
}
let mut server_handles = Vec::with_capacity(config.host.len());
for host in &config.host {
let addr = SocketAddr::new(*host, config.port);
let server = server_builder.clone().build(addr).await;
config.port = server.local_addr().port();
server_handles.push(server.run());
let mut addr = SocketAddr::new(*host, config.port);

match server_builder.clone().build(addr).await {
Ok(server) => {
config.port = server.local_addr().port();
server_handles.push(server.run());
}
Err(err) => {
tracing::info!(
"Failed to bind to address {}:{}: {}. Retrying with a different port...",
host,
config.port,
err
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I don't think this is a good fallback. If I were running a tool in CI and saw this behaviour I would have been unpleasantly surprised.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, indeed. I though for a second that anvil does that -- but no:

anvil
Error: Address already in use (os error 98)

Location:
    /home/runner/work/foundry/foundry/crates/anvil/src/lib.rs:229:28

I guess we have to replace the fallback with just error propagation.


// Attempt to bind to a dynamic port
addr.set_port(0);
match server_builder.clone().build(addr).await {
Ok(server) => {
config.port = server.local_addr().port();
tracing::info!(
"Successfully started server on port {} for host {}",
config.port,
host
);
server_handles.push(server.run());
}
Err(err) => {
return Err(anyhow!(
"Failed to start server on host {} with port: {}",
host,
err
));
}
}
}
}
}
let any_server_stopped =
futures::future::select_all(server_handles.into_iter().map(|h| Box::pin(h.stopped())));
Expand Down
10 changes: 5 additions & 5 deletions crates/core/src/node/inner/in_memory_inner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1628,7 +1628,7 @@ mod tests {
let system_contracts = node
.system_contracts
.system_contracts_for_initiator(&node.impersonation, &tx.initiator_account());
let (block_ctx, batch_env, mut vm) = test_vm(&mut *node, system_contracts).await;
let (block_ctx, batch_env, mut vm) = test_vm(&mut node, system_contracts).await;
let err = node
.run_l2_tx(tx, 0, &block_ctx, &batch_env, &mut vm)
.unwrap_err();
Expand All @@ -1647,7 +1647,7 @@ mod tests {
let system_contracts = node
.system_contracts
.system_contracts_for_initiator(&node.impersonation, &tx.initiator_account());
let (block_ctx, batch_env, mut vm) = test_vm(&mut *node, system_contracts).await;
let (block_ctx, batch_env, mut vm) = test_vm(&mut node, system_contracts).await;
let err = node
.run_l2_tx(tx, 0, &block_ctx, &batch_env, &mut vm)
.unwrap_err();
Expand All @@ -1670,7 +1670,7 @@ mod tests {
let system_contracts = node
.system_contracts
.system_contracts_for_initiator(&node.impersonation, &tx.initiator_account());
let (block_ctx, batch_env, mut vm) = test_vm(&mut *node, system_contracts).await;
let (block_ctx, batch_env, mut vm) = test_vm(&mut node, system_contracts).await;
let err = node
.run_l2_tx(tx, 0, &block_ctx, &batch_env, &mut vm)
.unwrap_err();
Expand Down Expand Up @@ -1740,7 +1740,7 @@ mod tests {
let system_contracts = node
.system_contracts
.system_contracts_for_initiator(&node.impersonation, &tx.initiator_account());
let (_, _, mut vm) = test_vm(&mut *node, system_contracts).await;
let (_, _, mut vm) = test_vm(&mut node, system_contracts).await;
node.run_l2_tx_raw(tx, &mut vm)
.expect("transaction must pass with external storage");
}
Expand Down Expand Up @@ -1790,7 +1790,7 @@ mod tests {
let system_contracts = node
.system_contracts
.system_contracts_for_initiator(&node.impersonation, &tx.initiator_account());
let (_, _, mut vm) = test_vm(&mut *node, system_contracts).await;
let (_, _, mut vm) = test_vm(&mut node, system_contracts).await;
let TxExecutionOutput { result, .. } = node.run_l2_tx_raw(tx, &mut vm).expect("failed tx");

match result.result {
Expand Down
15 changes: 15 additions & 0 deletions e2e-tests-rust/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion e2e-tests-rust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@ pub use provider::{
init_testing_provider, init_testing_provider_with_client, AnvilZKsyncApi, TestingProvider,
DEFAULT_TX_VALUE,
};
pub use utils::get_node_binary_path;
pub use utils::{get_node_binary_path, LockedPort};
30 changes: 29 additions & 1 deletion e2e-tests-rust/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ use alloy::providers::Provider;
use alloy::{
network::primitives::BlockTransactionsKind, primitives::U256, signers::local::PrivateKeySigner,
};
use alloy_zksync::node_bindings::AnvilZKsync;
use anvil_zksync_core::node::VersionedState;
use anvil_zksync_core::utils::write_json_file;
use anvil_zksync_e2e_tests::{
get_node_binary_path, init_testing_provider, init_testing_provider_with_client, AnvilZKsyncApi,
ReceiptExt, ZksyncWalletProviderExt, DEFAULT_TX_VALUE,
LockedPort, ReceiptExt, ZksyncWalletProviderExt, DEFAULT_TX_VALUE,
};
use anyhow::Context;
use flate2::read::GzDecoder;
Expand Down Expand Up @@ -778,3 +779,30 @@ async fn load_state_on_fork() -> anyhow::Result<()> {

Ok(())
}

#[tokio::test]
async fn test_server_port_fallback() -> anyhow::Result<()> {
let locked_port = LockedPort::acquire_unused().await?;

let node1 = AnvilZKsync::new()
.path(get_node_binary_path())
.port(locked_port.port)
.spawn();
let port1 = node1.port();

let node2 = AnvilZKsync::new()
.path(get_node_binary_path())
.port(locked_port.port)
.spawn();
let port2 = node2.port();

assert_ne!(
port1, port2,
"The second instance should have a different port due to fallback"
);

drop(node1);
drop(node2);

Ok(())
}
Loading