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

Use OffchainLabs/go-ethereum fork #2470

Closed
wants to merge 2 commits into from
Closed

Conversation

leszko
Copy link
Contributor

@leszko leszko commented Jun 21, 2022

What does this pull request do? Explain your changes. (required)
Arbitrum Nitro requires to use their fork of go-ethereum to run Arbitrum Nitro. Please check the related Discord discussion.

Specific updates (required)

  • Update go-ethereum dependency to use OffchainLabs fork instead of ethereum/go-ethereum.

How did you test each of these updates (required)

Run go-livepeer in Arbitrum Nitro Devnet.

Does this pull request close any open issues?

N/A

Checklist:

Arbitrum Nitro requires to use their fork.
@leszko leszko requested review from thomshutt and yondonfu June 21, 2022 12:01
@yondonfu
Copy link
Member

Looks like this PR is against master which means that the go-ethereum dependency update would be reflected in the next release.

@leszko Could this dependency update have any negative side effects for go-livepeer releases connecting to arbitrum-one-mainnet prior to the Nitro upgrade? If we are not sure then I think it would be preferable to not merge the dependency update into master and instead test the dependency update on a separate branch.

@thomshutt
Copy link
Contributor

Similarly to @yondonfu's comment, do you know if they have any plans to merge their fork back into master? If not, then we'll be reliant on them rebasing off anything that comes into mainline go-ethereum

@leszko
Copy link
Contributor Author

leszko commented Jun 22, 2022

If they merge it upstream, that would be the best for us. However, they currently don't know if it's going to be merged upstream. Here's the comment from the related Discord conversation:

here is the relevant PR OffchainLabs/go-ethereum#92 I am not sure it will be merged upstream

Now, what we can do about it?

  1. Manually test if it works and merge this PR to master
  2. Create the nitro branch and wait for switching Arbitrum Mainnet to Nitro
    • if they merge to upstream, we don't need to do anything
    • if they don't merge to upstream, we'll merge nitro branch into master during the Mainnet switch

I'm leaning towards Point 1, because of the following reasons:

  • During Mainnet=>Arbitrum, Os will still use the old version of Livepeer and suddenly it will start crashing, so it's better if they have the right version upront
  • During Mainnnet=>Arbitrum, we'll have two things changing at the same time 1) Chain switching to Nitro 2) Us changing the go-ethereum dep; this may result in hard times debugging what's wrong

Apart from that, Arbitrum uses their fork, so I believe it may be actually even safer to use for the current Mainnet.

Wdyt? @yondonfu @thomshutt

@thomshutt
Copy link
Contributor

👍 Makes sense and looks like they are keeping it reasonably up to date vs. the original go-ethereum. Happy for you two to make a call on this

@yondonfu
Copy link
Member

From glancing at the OffchainLabs/go-ethereum commit history, it looks like the last time it was synced with upstream ethereum/go-ethereum was for the v1.10.17 release OffchainLabs/go-ethereum@7dc8af1.

From glancing at the ethereum/go-ethereum commit history since the v1.10.17 there are at least a few commits that are associated with the packages we use i.e. accounts/abi, ethclient:

I have not gone through the details of any of the commits to confirm that they include fixes or features that are important for us. However, I think it is worth noting the following:

  • There are already commits not in the OffchainLabs fork that are relevant to packages we depend on
  • There could be future commits relevant to the packages we depend on upstream that will also not be in the OffchainLabs fork

Given the above considerations, I could see a scenario where we switch to the OffchainLabs fork, but then realize we want to pull in some of the upstream commits relevant to the packages we depend on. At that point, the best option would likely be to merge upstream/cherry-pick commits into our own fork based off of the OffchainLabs fork.

Since Nitro is still under development and the timeline for Nitro mainnet is still TBD, I am inclined to not use the OffchainLabs fork right now to avoid introducing headaches around the above in the short term and instead letting development of Nitro proceed a bit and we can always switch to the OffchainLabs fork at a later point in time (we don't necessarily need to wait until the Nitro upgrade on mainnet - we could just wait until we get a little closer to then) and figure out the right strategy for combining more recent relevant commits from upstream with the required Arbitrum commits in the OffchainLabs fork. I believe this lines up with option 2 suggested by @leszko.

@leszko
Copy link
Contributor Author

leszko commented Jun 23, 2022

Ok, then let's keep this PR (and the branch) open for now and see how it goes with the Arbitrum fork. Maybe they'll merge upstream. Or maybe they'll pull the current go-ethereum in.

@leszko
Copy link
Contributor Author

leszko commented Jul 29, 2022

I checked again the current master of go-livepeer and it seems to work correctly on Arbitrum Testnet Nitro. That means that Arbitrum fixed the issue with the incompatible transaction format, and therefore we do not need to merge the OffchainLabs/go-ethereum PR.

Closing this PR.

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