Skip to content

Support Dencun consensus upgrade - #981

Merged
yrong merged 63 commits into
mainfrom
ron/dencun
Jan 19, 2024
Merged

Support Dencun consensus upgrade#981
yrong merged 63 commits into
mainfrom
ron/dencun

Conversation

@yrong

@yrong yrong commented Oct 30, 2023

Copy link
Copy Markdown
Contributor

Resolves: SNO-406, SNO-775

  • Upgrade geth and lodestar to recent release
  • Add VersionedExecutionPayloadHeader to support multiple versions of the beacon updates
  • Cleanup for unused codes/packages(e.g. package ethashproof based on the deprecated POW consensus which can be removed).
  • Test e2e work as expected
  • Test the switchover process

@yrong yrong changed the title Ron/dencun Dencun Oct 30, 2023
@vgeddes

vgeddes commented Oct 30, 2023

Copy link
Copy Markdown
Collaborator

Nice work Ron! 🙏

@vgeddes vgeddes changed the title Dencun Add support for Dencun consensus upgrade Oct 30, 2023
@yrong yrong changed the title Add support for Dencun consensus upgrade Prepare for Dencun consensus upgrade Nov 21, 2023
@yrong
yrong marked this pull request as ready for review November 21, 2023 08:41
@yrong

yrong commented Nov 21, 2023

Copy link
Copy Markdown
Contributor Author

@vgeddes I would prefer to split https://linear.app/snowfork/issue/SNO-406 into multiple PRs, in this one we only focus on upgrading packages and add versioned data support in next one.

@vgeddes vgeddes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I've just noticed that there are a lot of unrelated changes to the E2E scripts, like making processes run in parallel etc. I vaguely recall previously reviewing such a change and saying that it makes troubleshooting harder if the E2E tests break.

The other reason for not wanting such changes is that roughly 80% of the initialization time is spent in compiling rust code. And the parallelization changes you're introducing here don't improve that. So I think the net gain is minimal.

So I'd like for you to please focus this PR on only the code relevant to the Dencun upgrades.

@yrong

yrong commented Nov 21, 2023

Copy link
Copy Markdown
Contributor Author

There are a lot of unrelated changes to the E2E scripts, like making processes run in parallel etc. The other reason for not wanting such changes is that roughly 80% of the initialization time is spent in compiling rust code. And the parallelization changes you're introducing here don't improve that.

Yes, rust compile is the most time consuming and the gain is minimal in this case. But for the second round of start up when there is no rust code change(e.g. cases when there is only change for relayer or solidity code) parallelization will decrease startup time to almost half(from 2 minutes 18 seconds to 1 minture 20 seconds in my laptop) so IMHO it's benefit.

But anyway just remove unrelated changes in 28f23dd to be more focused.

Comment thread relayer/go.mod Outdated
Comment thread .gitmodules Outdated
Comment thread .gitmodules
@codecov

codecov Bot commented Nov 22, 2023

Copy link
Copy Markdown

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (4e24732) 76.25% compared to head (1460d55) 75.08%.

❗ Current head 1460d55 differs from pull request most recent head 8e392c3. Consider uploading reports for the commit 8e392c3 to get more accurate results

Files Patch % Lines
parachain/primitives/beacon/src/ssz.rs 0.00% 30 Missing ⚠️
parachain/primitives/beacon/src/types.rs 55.17% 13 Missing ⚠️
parachain/pallets/ethereum-client/src/lib.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #981      +/-   ##
==========================================
- Coverage   76.25%   75.08%   -1.17%     
==========================================
  Files          58       58              
  Lines        2400     2464      +64     
  Branches       72       72              
==========================================
+ Hits         1830     1850      +20     
- Misses        553      597      +44     
  Partials       17       17              
Flag Coverage Δ
rust 74.00% <33.33%> (-1.37%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment thread relayer/relays/beacon/header/syncer/syncer.go Outdated
@yrong
yrong requested a review from vgeddes November 23, 2023 04:52
@yrong
yrong requested a review from vgeddes January 12, 2024 07:31

@vgeddes vgeddes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

+1.

@claravanstaden claravanstaden left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @yrong! Great job, this is awesome!

I made a few comments. I would have preferred the relayer cleanup (removing the Ethereum PoW code) in a separate PR, but what's done is done.

I am not convinced adding geth and lodestar as submodules is necessary. Did we make any changes at all that warrants using forked versions? Seems like unnecessary maintenance and I would prefer we use the original projects.

Comment thread web/packages/test/scripts/deploy-ethereum.sh
Comment thread web/packages/test/scripts/deploy-ethereum.sh
Comment thread .gitmodules Outdated
Comment thread parachain/pallets/ethereum-client/src/lib.rs
Comment thread parachain/pallets/ethereum-client/src/mock.rs
Comment thread relayer/go.mod
Comment thread relayer/magefile.go
Comment thread relayer/relays/beacon/header/syncer/api/api.go
Comment thread relayer/relays/beacon/header/syncer/scale/beacon_scale.go
Comment thread relayer/relays/beacon/header/syncer/util/util.go
Comment thread .gitmodules Outdated
Comment thread relayer/relays/beacon/state/beacon.go Outdated
Comment thread relayer/relays/beacon/state/beacon.go Outdated
Comment thread scripts/init.sh Outdated
(cd web && pnpm install)

echo "Download geth to replace the nix version"
geth_package=geth-$(uname | tr '[:upper:]' '[:lower:]')-amd64-1.13.10-bc0be1b1

@vgeddes vgeddes Jan 18, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think its simpler to hardcode the source filenames based on the output of:

nix eval --impure --raw --expr 'builtins.currentSystem'

also, we need to support arm/macos too, not just linux/amd64

@yrong yrong Jan 18, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It seems the download page only provides the amd64 versions, and I tested the darwin-amd64 package did work in my M1.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think you need to look that download page a bit more - there are links to downloads for many architectures:

Screenshot 2024-01-18 at 15 20 55

So depending on the value of builtins.currentSystem, which should be one of the architectures listed in flake.nix, download the appropriate binary.

@yrong yrong Jan 18, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I missed that. Fixed in fb5366f

Btw: nix eval --impure --raw --expr 'builtins.currentSystem' return aarch64-darwin in my laptop which does not match the darwin-arm64* in the download link so switch to uname -m instead.

@yrong
yrong merged commit 727728a into main Jan 19, 2024
@yrong
yrong deleted the ron/dencun branch January 19, 2024 14:06
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.

4 participants