Skip to content

op-proposer: cleanup proposer logic, update outputAtBlock API#3805

Merged
mslipper merged 4 commits intodevelopfrom
proposer-cleanup
Nov 2, 2022
Merged

op-proposer: cleanup proposer logic, update outputAtBlock API#3805
mslipper merged 4 commits intodevelopfrom
proposer-cleanup

Conversation

@protolambda
Copy link
Contributor

@protolambda protolambda commented Oct 27, 2022

Description

This PR updates the op-proposer and op-node to make proposals with an updated optimism_outputAtBlock API, which now ensures much better consistency between the L1 block and the L2 output which are combined into the output proposal transaction.

The op-proposer now does not have to communicate with a L2 engine anymore, and only communicates with the rollup-node and L1. And the L1 is only interacted with for txs, not for block hashes (since the rollup node tracks L1 block hashes in a more controlled/secure way already).

The rollup node provides a more extensive output response, to log more context and to use a L1 block that's consistent with the output for L1 reorg safety.

The ability to propose output roots for non-finalized (w.r.t. derivation from L1 inputs) L2 blocks is optional: enabling it reduces the latency of a L2 proposal on L1 to just confirmation depth and interval parameters, which helps with testing, and is technically still secure (the L1 contract checks the L1 block hash in the tx, to ensure reorg safety).

Although I dislike the naming of the module (Driver, confusing with op-node driver) it's already quite testable since it's separated from the proposer agent loop, so I'm avoiding changes to that structure in this PR, focusing on the API consistency/safety first.

This PR depends on #3802

Tests

Updated the output root e2e test, and added the proposer to action-testing with coverage of the common functionality.

Metadata

Fix ENG-2934

@changeset-bot
Copy link

changeset-bot bot commented Oct 27, 2022

⚠️ No Changeset found

Latest commit: 894af7f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mergify
Copy link
Contributor

mergify bot commented Oct 27, 2022

This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?

@protolambda
Copy link
Contributor Author

@trianglesphere @mslipper @tynes This is a draft PR until I extend it with additional testing, but if you have time I appreciate some early review.

@protolambda
Copy link
Contributor Author

Tried to update the devnet by removing the L2 RPC flag from the proposer options (since we don't define / use it anymore) but it's still failing somewhere. Will look into this later after work on base PRs in this and other stack.

Base automatically changed from driver-status-api to develop October 31, 2022 20:19
@protolambda
Copy link
Contributor Author

Rebased and added an action-test to cover the common proposer functionality. Will make a ticket for extended proposer testing, there are more edge cases but I don't want to make this PR larger, I need to wrap up a lot of open PRs first.

@protolambda protolambda marked this pull request as ready for review November 1, 2022 12:44
Copy link
Contributor

@trianglesphere trianglesphere 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 blocking this PR because the hive L1 Ops test failure appears to be directly related to changes made in this PR. I looked at the logs & it is timing out when waiting for a proposal on L1. In addition, there are some flag changes that need to be handled.

In general this PR is quite the mix of different API changes & I still would like the action tests to not peer into the batcher internals as much; however once the tests are passing & the flag issue is addressed, I am fine approving this PR.

@protolambda
Copy link
Contributor Author

@trianglesphere

I still would like the action tests to not peer into the batcher internals as much;

Same, but the batcher code was not in a place to pull it into testing well. We can improve that over time, the batcher should not be the focus of this PR. And I believe the proposer is in a somewhat better state (one state object, smaller interface), but can improve over time too. We definitely do need to test proposals, and being able to run the full action-test suite within seconds is quite powerful, so I think this is still the right approach.

OP_PROPOSER_L2_ETH_RPC

Thanks for highlighting it, managing all usages/changes to flags is not easy though. I'm going to work on a broader solution to improve this. Right now it's quite painful to find/fix all the flag usages, some help here is appreciated.

I'll try find some time to fix the devnet issue, but then like to continue with a wider solution for env/flags docs and reverse usage mapping. Will ping when you can review this PR again after those changes, but feel free to suggest more concrete changes to help this PR along if you know where to change it.

@trianglesphere
Copy link
Contributor

Same, but the batcher code was not in a place to pull it into testing well. We can improve that over time, the batcher should not be the focus of this PR. And I believe the proposer is in a somewhat better state (one state object, smaller interface), but can improve over time too. We definitely do need to test proposals, and being able to run the full action-test suite within seconds is quite powerful, so I think this is still the right approach.

Sorry, I mispoke when summarizing, I meant to write "I still would like the action tests to not peer into the batcher proposer internals as much"

@mslipper
Copy link
Contributor

mslipper commented Nov 2, 2022

I fixed the flags + updated Hive to specify the OP_PROPOSER_ALLOW_NON_FINALIZED env var.

The devnet thing is failing for the same unknown reasons it's failing on develop. It works fine when I run it locally, or when I run it on CircleCI with SSH enabled.

Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

Flags are fixed + tests passing so I'm approving.
Reminder that we need to update these flags/env vars in K8s & ansible.

@mslipper mslipper merged commit 05d367e into develop Nov 2, 2022
@mslipper mslipper deleted the proposer-cleanup branch November 2, 2022 17:53
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.

3 participants