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: return bool for set_max_da_size #13423

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

SangyunOck
Copy link
Contributor

@SangyunOck SangyunOck commented Dec 17, 2024

ref: alloy-rs/op-alloy#346, #13422

Motivation

While op-batcher tries to set SetMaxDASize by calling miner_setMaxDASize of op-reth, returns error

ERROR[12-17|18:17:23.568] Result of SetMaxDASize was false, retrying.
ERROR[12-17|18:17:25.566] Result of SetMaxDASize was false, retrying.
INFO [12-17|18:17:25.567] Starting batch-submitter work at safe-head safe=576f49..32600c:0
INFO [12-17|18:17:25.568] Added L2 block to local state            block=b3d6c5..31bcad:1 tx_count=1 time=1,734,423,746
ERROR[12-17|18:17:25.569] Result of SetMaxDASize was false, retrying.
INFO [12-17|18:17:25.569] Added L2 block to local state            block=94dd18..cca0b0:2 tx_count=1 time=1,734,423,748
ERROR[12-17|18:17:25.569] Result of SetMaxDASize was false, retrying.
INFO [12-17|18:17:25.569] Added L2 block to local state            block=66c587..a527bf:3 tx_count=1 time=1,734,423,750
ERROR[12-17|18:17:25.570] Result of SetMaxDASize was false, retrying.
INFO [12-17|18:17:25.570] Added L2 block to local state            block=9425c1..828a77:4 tx_count=1 time=1,734,423,752
ERROR[12-17|18:17:25.570] Result of SetMaxDASize was false, retrying.
INFO [12-17|18:17:25.570] Added L2 block to local state            block=b6cae3..91f714:5 tx_count=1 time=1,734,423,754
ERROR[12-17|18:17:25.570] Result of SetMaxDASize was false, retrying.
INFO [12-17|18:17:25.571] Added L2 block to local state            block=e298dd..63bee4:6 tx_count=1 time=1,734,423,756
ERROR[12-17|18:17:25.571] Result of SetMaxDASize was false, retrying.
INFO [12-17|18:17:25.571] Added L2 block to local state            block=41e4eb..fbfead:7 tx_count=1 time=1,734,423,758
ERROR[12-17|18:17:25.571] Result of SetMaxDASize was false, retrying.
INFO [12-17|18:17:25.571] Added L2 block to local state            block=a1edc1..9a645e:8 tx_count=1 time=1,734,423,760
ERROR[12-17|18:17:25.571] Result of SetMaxDASize was false, retrying.
INFO [12-17|18:17:25.572] Added L2 block to local state

Original golang implementation of op-geth returns bool type, op-batcher expects true for the result, or treat as error for others, including null.

expected

{
  "jsonrpc": "2.0",
  "id": 0,
  "result": true
}

as-is

{
  "jsonrpc": "2.0",
  "id": 0,
  "result": null
}

Solution

Change set_max_da_size return type from RpcResult<()> to RpcResult<bool>
https://github.com/paradigmxyz/reth/blob/main/crates/optimism/rpc/src/miner.rs#L27

  • TODO: After PR merged on op-alloy side, dependency on Cargo.toml should be replaced.

closes #13422

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

ty

Cargo.toml Outdated
@@ -479,7 +479,7 @@ alloy-transport-ws = { version = "0.8.1", default-features = false }
# op
op-alloy-rpc-types = "0.8.3"
op-alloy-rpc-types-engine = "0.8.3"
op-alloy-rpc-jsonrpsee = "0.8.3"
op-alloy-rpc-jsonrpsee = { git = "https://github.com/ffddw/op-alloy.git", branch = "issues/fix-return-type" } # TODO: to be replaced
Copy link
Collaborator

Choose a reason for hiding this comment

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

I release a new version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! would it works with 4a648d1?

@mattsse
Copy link
Collaborator

mattsse commented Dec 17, 2024

can't push the update because opened from org fork

@mattsse mattsse enabled auto-merge December 17, 2024 21:50
@mattsse mattsse added this pull request to the merge queue Dec 17, 2024
Merged via the queue into paradigmxyz:main with commit c51a188 Dec 17, 2024
42 checks passed
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.

Optimism miner_setMaxDASize call should return bool type
2 participants