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.

Sign up for GitHub

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

Jump to bottom

Correct usage of chain ID and network ID #4850

Merged
merged 299 commits into from
Jan 20, 2023
Merged

Correct usage of chain ID and network ID #4850

merged 299 commits into from
Jan 20, 2023

Conversation

flcl42
Copy link
Contributor

@flcl42 flcl42 commented Oct 31, 2022 •
edited
Loading

Resolves #4848

Devnets may have chain id different from network id, but we use network id everywhere as chain id, so it breaks transaction verification.

Changes:

  • Use real chain id when network id was improperly used

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Testing

Requires testing

  • Yes
  • No

In case you checked yes, did you write tests??

  • Yes
  • No

Steps to reproduce:

  • Create a network of Nethermind / Geth / Prysm with chain ID = 1 / network ID = 2
  • Try to sign transactions, synchronize
  • Errors appear in logs about broken transactions and communication refusals

Should be:

  • All the communication work, the clients synchronize

Additional Details

Chain ID is needed for:

  • tx signing
  • tx hashing
  • account abstraction hashing

Network ID is needed for P2P:

  • eth/67
  • les/4

@flcl42 flcl42 marked this pull request as draft October 31, 2022 17:07
@flcl42 flcl42 marked this pull request as ready for review November 2, 2022 11:29
rubo and others added 26 commits November 2, 2022 23:28
@rubo
Add engine_getPayloadV2
1f1a08f
@rubo
Format whitespaces
5275510
@rubo
Refactor and remove redundant "V1"
ba0219b
@MarekM25 @rubo
started adding Enginge V2 tests - V2_processing_block_should_serializ…
3d09efa 
…e_valid_responses
@MarekM25 @rubo
formatting
10dcda2
@MarekM25 @rubo
formatting
94c17cb
@rubo
Implement withdrawal root validation
45aae0a
@MarekM25 @rubo
working on tests and fixes
6cc1089
@MarekM25 @rubo
Merge branch 'feature/shanghai-eip-4895-withdrawals' of https://githu…
86e8f72 
…b.com/nethermindeth/nethermind into feature/shanghai-eip-4895-withdrawals
@MarekM25 @rubo
fix block for processing
2a724f9
@MarekM25 @rubo
BlockValidator WithdrawalRoot
56f0774
@rubo
Fix withdrawal trie proof validation
1224c48
@MarekM25 @rubo
add WithdrawalApplier
135b9f9
@MarekM25 @rubo
Merge branch 'feature/shanghai-eip-4895-withdrawals' of https://githu…
473cd20 
…b.com/nethermindeth/nethermind into feature/shanghai-eip-4895-withdrawals
@rubo
Remove redundant withdrawals hash check
f3be3ea
@rubo
Remove withdrawal check for null
ec2f25d
@rubo
Add withdrawals test chain spec
b00b594
@rubo
Reformat whitespaces
a43ca6d
@MarekM25
add withdrawalApplier to blockchainProcessor
6fd2c3d
@MarekM25
Merge branch 'feature/shanghai-eip-4895-withdrawals' of https://githu…
c0adc96 
…b.com/nethermindeth/nethermind into feature/shanghai-eip-4895-withdrawals

# Conflicts:
#	src/Nethermind/Nethermind.Blockchain.Test/Proofs/WithdrawalTrieTests.cs
#	src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs
#	src/Nethermind/Nethermind.Consensus/Withdrawals/IWithdrawalApplier.cs
#	src/Nethermind/Nethermind.Consensus/Withdrawals/ProductionWithdrawalApplier.cs
#	src/Nethermind/Nethermind.Consensus/Withdrawals/ValidationWithdrawalApplier.cs
#	src/Nethermind/Nethermind.Core/BlockHeader.cs
#	src/Nethermind/Nethermind.Core/Specs/IReleaseSpec.cs
#	src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.V2.cs
#	src/Nethermind/Nethermind.Serialization.Rlp/WithdrawalDecoder.cs
#	src/Nethermind/Nethermind.Specs/Forks/15_Shanghai.cs
#	src/Nethermind/Nethermind.State/Proofs/WithdrawalTrie.cs
@MarekM25
add withdrawals to ExecutionPayload
cd18844
@MarekM25
test fixes
d9e2a08
@MarekM25
fix EVM.test
ea9ff25
@MarekM25
fix EthereumTests
78a06d4
@MarekM25
fix Benchmarks.sln
117225f
@rubo
Revise withdrawals representation according to the spec and refactor
37f703c
@flcl42 flcl42 marked this pull request as draft January 11, 2023 15:53
@flcl42 flcl42 force-pushed the bugfix/4848-Correct-usage-of-chain-and-network-ids branch 3 times, most recently from c3d8414 to 4f62200 Compare January 11, 2023 18:12
@flcl42 flcl42 force-pushed the bugfix/4848-Correct-usage-of-chain-and-network-ids branch from 4f62200 to 6968572 Compare January 11, 2023 18:14
@flcl42 flcl42 marked this pull request as ready for review January 11, 2023 18:14
LukaszRozmej
LukaszRozmej reviewed Jan 18, 2023
View reviewed changes
src/Nethermind/Nethermind.Blockchain/IBlockTree.cs Show resolved Hide resolved
src/Nethermind/Nethermind.EthStats/EthStatsPlugin.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Evm/Precompiles/EcRecoverPrecompile.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Runner.Test/ChainspecFilesTests.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Specs.Test/ChainSpecStyle/ChainSpecLoaderTests.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Specs.Test/CustomSpecProvider.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpec.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Specs/ChainSpecStyle/Json/ChainSpecParamsJson.cs Show resolved Hide resolved
flcl42 and others added 3 commits January 19, 2023 17:21
@flcl42
Merge remote-tracking branch 'origin/master' into bugfix/4848-Correct…
222affe 
…-usage-of-chain-and-network-ids
@LukaszRozmej
Fix
87dc35b
@flcl42
Merge branch 'bugfix/4848-Correct-usage-of-chain-and-network-ids' of …
00459a4 
…github.com:NethermindEth/nethermind into bugfix/4848-Correct-usage-of-chain-and-network-ids
@LukaszRozmej LukaszRozmej merged commit fd34651 into master Jan 20, 2023
@LukaszRozmej LukaszRozmej deleted the bugfix/4848-Correct-usage-of-chain-and-network-ids branch January 20, 2023 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukaszRozmej LukaszRozmej LukaszRozmej approved these changes

@asdacap asdacap asdacap approved these changes

@marcindsobczak marcindsobczak Awaiting requested review from marcindsobczak

@kamilchodola kamilchodola Awaiting requested review from kamilchodola

@MarekM25 MarekM25 Awaiting requested review from MarekM25

Assignees

@kamilchodola kamilchodola

Labels
eip-4844
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix chain Id usage
7 participants
@flcl42 @asdacap @MarekM25 @LukaszRozmej @kamilchodola @rubo @smartprogrammer93