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

Remove devnet6 trusted setup #3288

Merged
merged 3 commits into from
Feb 21, 2024
Merged

Remove devnet6 trusted setup #3288

merged 3 commits into from
Feb 21, 2024

Conversation

acolytec3
Copy link
Contributor

Updates all references to the devnet6.txt trusted setup to point to the official.txt trusted setup for mainnet.

Also regenerates test data where necessary using official trusted setup.

Copy link
Contributor Author

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

Note, I removed the nethermind blob transaction in the tx eip4844 tests because it was created on devnet6 and I have no way of regnerating that test data using the mainnet trusted setup. I will try and find a full blob transaction from geth or another client that we can verify using the new settings.

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6e9cc95) 86.90% compared to head (c194221) 86.91%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 88.34% <ø> (ø)
blockchain 91.61% <ø> (ø)
client 84.65% <ø> (ø)
common 98.25% <ø> (ø)
devp2p 82.12% <ø> (ø)
ethash ∅ <ø> (∅)
evm 74.33% <ø> (ø)
genesis 99.98% <ø> (ø)
statemanager 77.02% <ø> (ø)
trie 89.25% <ø> (+0.27%) ⬆️
tx 95.45% <ø> (ø)
util 89.13% <ø> (ø)
vm 80.20% <ø> (ø)
wallet 88.35% <ø> (ø)

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

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

If you could leave some documentation so we can verify that the output data is correct, then I think all is fine :)

@@ -119,7 +119,7 @@ describe(method, () => {
const { executionPayload, blobsBundle } = res.result
assert.equal(
executionPayload.blockHash,
'0xe8175305416ee94c996164162044338b4f4d93a8dc458b574ecad4ce84323fb5',
'0x8c71ad199a3dda94de6a1c31cc50a26b1f03a8a4924e9ea3fd7420c6411cac42',
Copy link
Member

Choose a reason for hiding this comment

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

I am fine with this change, however, how do we know this is correct?

@@ -600,31 +598,6 @@ describe('Network wrapper deserialization test', () => {
},
'txMeta should match'
)

// override common chain id and test nethermind generated tx
common.chainId = () => BigInt(42)
Copy link
Member

Choose a reason for hiding this comment

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

This is the removed one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@acolytec3
Copy link
Contributor Author

If you could leave some documentation so we can verify that the output data is correct, then I think all is fine :)

Okay, I basically just updated the expected outputs to match what is produced when you use the official trusted setup instead of the devnet 6 one.

@holgerd77
Copy link
Member

Updated this via UI

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Great work, looks good! 👍 🙂

Will merge.

@holgerd77 holgerd77 merged commit 9994875 into master Feb 21, 2024
46 checks passed
@holgerd77 holgerd77 deleted the update-kzg-tests branch February 21, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants