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: add support for alt-da with da-server #69

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

samlaf
Copy link

@samlaf samlaf commented Sep 4, 2024

Closes #67

Mimicing the behavior of the docker-compose devnet.

Testing

Only manually deployed everything, and checked that the op-batcher and da-server logs were normal.

README.md Outdated
Comment on lines 132 to 143
# Available services:
# - blockscout
# - da_server
additional_services: []

# Configuration place for da-server - https://github.com/ethereum-optimism/optimism/tree/develop/op-alt-da
da_server_params:
# A list of optional extra params that will be passed to the da-server container for modifying its behaviour
da_server_extra_args: []
generic_commitment: false
Copy link
Author

@samlaf samlaf Sep 4, 2024

Choose a reason for hiding this comment

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

Right now adding da_server to the additional_services list will by default turn on alt-da with default keccakCommitment mode. the commitment mode can be changed to generic_commitment in the da_server_params.

Given that alt-da and commitment mode and network parameters that affect other services (op-node at least), I feel like they should be somewhere more prominent. Wasn't sure if the network_params section of the config was the right place though. Also doing this might require double checking the settings passed to da_server_params to make sure they agree.

@@ -162,6 +165,9 @@ def get_beacon_config(
"--p2p.listen.ip=0.0.0.0",
"--p2p.listen.tcp={0}".format(BEACON_DISCOVERY_PORT_NUM),
"--p2p.listen.udp={0}".format(BEACON_DISCOVERY_PORT_NUM),
"--altda.enabled=" + str(da_server_context.enabled),
Copy link
Member

Choose a reason for hiding this comment

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

Similar check would be needed here instead of adding it by default to all nodes irrespective of if the da server is present

Copy link
Author

Choose a reason for hiding this comment

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

see my other comment. enabled is set to false by default, so its safe to add by default (and is what the other docker-compose devnet was doing).

da_server_extra_args,
generic_commitment,
):
image = "us-docker.pkg.dev/oplabs-tools-artifacts/images/da-server:devnet"
Copy link
Member

Choose a reason for hiding this comment

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

the devnet tag doesn't exist and is overriding the DEFAULT in the input parser. In general I'd avoid overriding it this way. I'd also add the da_image as a config variable under da_server_params and then set the image return value for ServiceConfig as image=da_server_params.da_server_image. The input parser will populate the default value if none is set, so by default the latest image is used and if specified the config image is used.

Copy link
Author

Choose a reason for hiding this comment

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

good catch! 4598487

"--addr=0.0.0.0",
"--port=3100",
"--log.level=debug",
"--generic-commitment=" + str(generic_commitment),
Copy link
Member

Choose a reason for hiding this comment

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

da_server image complains on this at startup: Incorrect Usage: flag provided but not defined: -generic-commitment

I'd image the latest image found here us-docker.pkg.dev/oplabs-tools-artifacts/images/da-server:latest is stale and missing the flag?

Copy link
Author

Choose a reason for hiding this comment

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

Yes confirmed that the latest image is missing the flag. And the more recent one v0.1.0-rc.1 from June gives me this error when I try to run on my mac: rosetta error: failed to open elf at /lib/ld-musl-x86_64.so.1

Copy link
Author

Choose a reason for hiding this comment

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

WIth the latest kurtosis 1.3 release I was able to add optimism repo as a submodule and build the da-server image locally: e709d7c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add alt-da workflow
2 participants