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

non-blocking-da v1: da errgroup #7

Closed
wants to merge 19 commits into from

Conversation

samlaf
Copy link
Collaborator

@samlaf samlaf commented Aug 23, 2024

This is the simplest possible implementation to fix the non-blocking da requests. We build off of @karlb's celo-org#213. We use an errgroup for da requests, with its own limit to the number of concurrent goroutines. See https://hackmd.io/@samlaf/op-batcher-concurrent-altda-requests-design-doc for more details.

Testing

We added a basic e2e test by using a fakeDaServer that sleeps for 5 seconds before returning commitments, in order to simulate slow EigenDA responses.

We also tested this manually by running the devnet and submitting large txs and observing the behavior.

@samlaf samlaf marked this pull request as draft August 24, 2024 05:18
@samlaf samlaf changed the base branch from devnet-monitoring to altda-devnet-monitoring August 28, 2024 05:08
Inphi and others added 7 commits August 29, 2024 02:26
* cannon: Fix stack patching

And add `memprofilerate=0` to envp

* Update cannon/mipsevm/program/patch.go

Co-authored-by: protolambda <proto@protolambda.com>

* cleanup argv/envp string ptrs

* nit

* fix envar name

* Update cannon/mipsevm/program/patch.go

Co-authored-by: mbaxter <meredith@oplabs.co>

* align op-program arg0

---------

Co-authored-by: protolambda <proto@protolambda.com>
Co-authored-by: mbaxter <meredith@oplabs.co>
To demonstrate how we can make our deployments more modular, this PR proposes archiving smart contract artifacts as tarballs that get uploaded to GCS. This  allows deployment tools to use precompiled artifacts rather than generating them on-the fly.

The archives are named after the hash of all Solidity files in the contracts-bedrock folder, including those in `lib/`, plus some additional metadata files like `foundry.toml` and `semver-lock.json`. See `calculate-checksum.sh` for details on how the algorithm works. I'm open to feedback around what should make up the checksum.

Since the atifacts are content-addressable, this PR also updates the CI pipeline to download the artifacts from GCS prior to running `pnpm monorepo`. When the Solidity codebase doesn't change, this allows the `pnpm monorepo` job to skip compiling Solidity altogether. While this won't work as well when we're actively modifying the Solidity codebase, since the hash will change, it does provide a modest speedup in CI.
…1605)

* cannon: Implement multithreaded clone fuzz test

* cannon: Add more clone evm tests

* cannon: Add evm test for GetTID syscall

* cannon: Add evm test for SysExit

* cannon: Add evm test for popping exited threads from the stack

* cannon: Fix futex wait handling, add evm test

* cannon: Add evm test for handling waiting thread

* cannon: Add test utils for defining / validating MTState expectations

* cannon: Add tests for futex wake, wake traversal

* cannon: Add test for SysYield

* cannon: Add SysOpen test, todos

* cannon: Add test for SchedQuantum preemption, fix inconsistency

* cannon: Add tests for noop, unsupported syscalls

* cannon: Remove duplicate constants

* cannon: Add tests for unsupported futex ops

* cannon: Group traversal tests, fix TestEVM_WakeupTraversalStep

* cannon: Add tests for nanosleep

* cannon: Add additional testcase for wakeup traversal

* cannon: Tweak futex wake tests

* cannon: Update mt fuzz test to use new test utils

* cannon: Rename contructor method for consistency

* cannon: Add some simple tests for ExpectedMTState util

* cannon: Add another validation test

* cannon: Move syscall lists to tests where they're used

* cannon: Add comment

* cannon: Extract some evm test helpers

* cannon: Cleanup - use require.Equalf for formatting

* cannon: Rename test util to AssertEVMReverts

* cannon: Add GetThreadStacks helper

* cannon: Add a few more traversal tests
* Shutdown sequencer before stopping p2p

* Check p2p isn't also disabled

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* Remove missed time.Sleep

* Fix up use of SetupP2P.Disabled

* Revert error check after RPC boundary

* Add comment about context for StopSequencer

* Add Config.p2pEnabled

* op-node: Make Config.P2PEnabled public

---------

Co-authored-by: Sebastian Stammler <seb@oplabs.co>
* test: fix L2 standard bridge interop tests

* test: mock factory implementation instead of proxy
…thereum-optimism#11633)

- protect txpool state vars with a mutex so they can be automically updated to avoid potential race
  condition
…ethereum-optimism#11670)

Bumps [github.com/hashicorp/raft](https://github.com/hashicorp/raft) from 1.7.0 to 1.7.1.
- [Release notes](https://github.com/hashicorp/raft/releases)
- [Changelog](https://github.com/hashicorp/raft/blob/main/CHANGELOG.md)
- [Commits](hashicorp/raft@v1.7.0...v1.7.1)

---
updated-dependencies:
- dependency-name: github.com/hashicorp/raft
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…#11511)

* Use context interrupts consistently in more places

* Fix CI lint errors

(cherry picked from commit 0410b7e)

* op-service/ctxinterrupt: address review comments

---------

Co-authored-by: protolambda <proto@protolambda.com>
@samlaf samlaf force-pushed the non-blocking-da-implv1-simple-waitgroup branch from c7f96cc to f4ca960 Compare August 30, 2024 01:26
@samlaf samlaf changed the base branch from altda-devnet-monitoring to develop August 30, 2024 01:27
@samlaf samlaf force-pushed the non-blocking-da-implv1-simple-waitgroup branch from f4ca960 to d78ffa9 Compare August 30, 2024 01:31
@samlaf samlaf changed the title non-blocking-da v1: Simple waitgroup non-blocking-da v1: simple errgroup Aug 30, 2024
@samlaf samlaf changed the title non-blocking-da v1: simple errgroup non-blocking-da v1: da errgroup Aug 30, 2024
Copy link

@karlb karlb left a comment

Choose a reason for hiding this comment

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

Looks good! I'll give it a second read through later today.

op-alt-da/damock.go Outdated Show resolved Hide resolved
op-batcher/batcher/driver.go Outdated Show resolved Hide resolved
@samlaf samlaf marked this pull request as ready for review August 30, 2024 19:47
agusduha and others added 3 commits August 30, 2024 20:35
---------

Co-authored-by: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com>
…m#11618)

* feat: add createX preinstall

* feat: change name from CreateXDeployer to CreateX
…hereum-optimism#11687)

Contexts are scoped to a specific GitHub user group, which doesn't work with the GitHub merge queue or OSS contributors. This PR updates the packaging job to use raw project-level env vars instead (which are not user-scoped), and to only run on commits to `develop`.
@samlaf samlaf force-pushed the non-blocking-da-implv1-simple-waitgroup branch from 81cf5db to c42fedd Compare August 30, 2024 22:24
test(batcher): add e2e test for concurrent altda requests

doc: add explanation comment for FakeDAServer

chore: fix if condition in altda sendTransaction path

feat: add maxConcurrentDaRequests config flag + semaphore

refactor: batcher to use errgroup for da instead of separate semaphore/waitgroup

fix: nil pointer bug after using wrong function after rebase

fix: defn of maxConcurrentDaRequests=0

fix: TestBatcherConcurrentAltDARequests

chore: remove unneeded if statement around time.Sleep

refactor: use TryGo instead of Go to make logic local and easier to read

chore: clean up some comments in batcher

chore: make batcher shutdown cancel pending altda requests by using shutdownCtx instead of killCtx
@samlaf samlaf force-pushed the non-blocking-da-implv1-simple-waitgroup branch from c42fedd to 6acfb99 Compare August 30, 2024 22:26
@samlaf
Copy link
Collaborator Author

samlaf commented Sep 12, 2024

Closing since this was merged upstream.

@samlaf samlaf closed this Sep 12, 2024
@samlaf samlaf deleted the non-blocking-da-implv1-simple-waitgroup branch September 12, 2024 05:25
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.

9 participants