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

block: Add fix for kovan nonce #1334

Merged
merged 1 commit into from
Jul 6, 2021
Merged

block: Add fix for kovan nonce #1334

merged 1 commit into from
Jul 6, 2021

Conversation

acolytec3
Copy link
Contributor

@acolytec3 acolytec3 commented Jul 6, 2021

The Kovan block nonce is a hex string of 65 0s and our block package expects a nonce of 8 bytes in length so trying to sync Kovan with the client results in an error.

INFO [07-06|11:00:47] Connecting to network: kovan
ERROR [07-06|11:00:47] Error: nonce must be 8 bytes, received 65 bytes
    at BlockHeader._validateHeaderFields (/home/jim/development/ethereumjs-monorepo/packages/block/src/header.ts:323:13)
    at new BlockHeader (/home/jim/development/ethereumjs-monorepo/packages/block/src/header.ts:267:10)
    at Function.fromHeaderData (/home/jim/development/ethereumjs-monorepo/packages/block/src/header.ts:77:12)
    at Function.fromBlockData (/home/jim/development/ethereumjs-monorepo/packages/block/src/block.ts:36:32)
    at Function.genesis (/home/jim/development/ethereumjs-monorepo/packages/block/src/block.ts:131:18)
    at Blockchain._init (/home/jim/development/ethereumjs-monorepo/packages/blockchain/src/index.ts:327:28)
    at Blockchain.initAndLock (/home/jim/development/ethereumjs-monorepo/packages/blockchain/src/index.ts:413:5)
    at Blockchain.getLatestHeader (/home/jim/development/ethereumjs-monorepo/packages/blockchain/src/index.ts:776:12)
    at Chain.update (/home/jim/development/ethereumjs-monorepo/packages/client/lib/blockchain/chain.ts:222:22)
    at Chain.open (/home/jim/development/ethereumjs-monorepo/packages/client/lib/blockchain/chain.ts:186:5)

Adds hack to _validateHeaderData for Kovan genesis nonce so that client can try to start syncing Kovan testnet.

What does the team think? Do we need to support Kovan in the client?

@codecov
Copy link

codecov bot commented Jul 6, 2021

Codecov Report

Merging #1334 (ab47cc5) into master (cff1517) will increase coverage by 7.33%.
The diff coverage is n/a.

❗ Current head ab47cc5 differs from pull request most recent head 3b9a8a7. Consider uploading reports for the commit 3b9a8a7 to get more accurate results
Impacted file tree graph

Flag Coverage Δ
block ?
blockchain ?
client ?
common 93.75% <ø> (ø)
devp2p ?
ethash ?
tx 88.36% <ø> (+0.11%) ⬆️
vm ?

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

@ryanio
Copy link
Contributor

ryanio commented Jul 6, 2021

Nice, thanks for opening this PR. It looks like all blocks (not just genesis) have this 65-byte length nonce so it may make sense to check if common is on chain kovan and pass if it is exactly 65 bytes instead of checking the stateRoot to be the genesis block.

@acolytec3 acolytec3 force-pushed the validate-kovan-genesis branch from ed8bf7d to 67574e2 Compare July 6, 2021 17:33
@acolytec3
Copy link
Contributor Author

Nice, thanks for opening this PR. It looks like all blocks (not just genesis) have this 65-byte length nonce so it may make sense to check if common is on chain kovan and pass if it is exactly 65 bytes instead of checking the stateRoot to be the genesis block.

Cool, I've updated to check for networkId instead of my previous hack. It still starts up but looks like none of the bootnodes we have in the genesis config are accepting incoming connections so I'm not able to get any blocks. But, I think it's probably worth merging this if everyone is okay with it so if someone else is running a kovan node and wants to test with us, they have access to it.

@acolytec3 acolytec3 force-pushed the validate-kovan-genesis branch from ab47cc5 to 3b9a8a7 Compare July 6, 2021 20:24
@acolytec3 acolytec3 changed the title block: Add fix for kovan genesis nonce block: Add fix for kovan nonce Jul 6, 2021
@acolytec3
Copy link
Contributor Author

@ryanio I've fixed the typo and looks like everything is passing now. Anything else you'd add at this point?

Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

lgtm!

@acolytec3 acolytec3 merged commit 9c5d6ea into master Jul 6, 2021
@acolytec3 acolytec3 deleted the validate-kovan-genesis branch July 6, 2021 22:21
@holgerd77
Copy link
Member

I am not sure if I am a fan of this change. Some background: Kovan uses the Aura (this is generally not so well documented or specified) PoA protocol developed by Parity Technologies and others. This is not compatible with the PoA protocol from geth (Clique).

We currently just don't support Aura, neither in Common nor in the upper layer libraries (e.g. block validator selection/voting in blockchain which we implemented for Clique e.g.) up to the Client.

If we now start to hotfix things for Kovan we just start tricking people into thinking that Kovan is supported (we should rather state more clearly that it is not in the documentation) while this is just not supported and will likely break on various occasions when Kovan specific mechanisms are triggered.

If we would want to support that would be a major dedicated effort, just some context for Andrew: we implemented Clique earlier this year and this was 3-4 weeks of work for 1-2 people working on the implementation. For Kovan we very likely not want to do this, the network is not very heavily used and will likely get either obsolete or at least very much less relevant after the PoS merge.

So: also no super strong opinion on this change here. From my side we can also leave but it might also make sense to just revert.

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