-
Notifications
You must be signed in to change notification settings - Fork 782
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
Various Hive related fixes #2532
Conversation
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
Additional things needed to properly run this code with hive:
|
Use this command -- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some remarks :)
@@ -354,8 +354,10 @@ export class BlockHeader { | |||
if (this._common.consensusAlgorithm() === ConsensusAlgorithm.Ethash) { | |||
// PoW/Ethash | |||
if ( | |||
number > BigInt(0) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this disables genesis extradata check by default, I am not sure if we want this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be tricky since this check is in common and doesn't have access to the clients config flags that would indicate a test environment. We could have the hive simulator remove the extradata field if we're on PoW and that should circumvent the issue
@@ -292,6 +295,10 @@ const args: ClientOpts = yargs(hideBin(process.argv)) | |||
'To run client in single node configuration without need to discover the sync height from peer. Particularly useful in test configurations. This flag is automically activated in the "dev" mode', | |||
boolean: true, | |||
}) | |||
.option('loadBlocksFromRlp', { | |||
describe: 'path to a file of RLP encoded blocks', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should take a look how other clients import these blocks. I would be more in favor to only allow this if some kind of "test network" flag is provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's pretty similar. They look for a specific flag and load them if so. Why would this be an issue for us as currently designed? This flag never gets set for a production run.
packages/client/bin/cli.ts
Outdated
}` | ||
) | ||
} catch { | ||
config.logger.info('Encountered invalid block while preloading chain') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the case, should we not immediately quit the client, instead of proceeding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/ethereum/hive/blob/master/docs/clients.md No, we're supposed to import to to the last valid block, per the hive docs
packages/client/bin/cli.ts
Outdated
) | ||
} catch { | ||
config.logger.info('Encountered invalid block while preloading chain') | ||
done = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
break
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, yes, that should have been obvious to me. Will update.
Ok, I would regard this as too much work-in-progress as to consider for tomorrows releases. Please do not merge before releases are published. |
4c39892
to
7edeab8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
73095a8
to
3f00fc4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
New fixes related to Hive tests
--loadBlocksFromRlp
CLI parameter that allows for loading RLP encoded blocks on chain start that are fed via the./chain.rlp
path in various Hive tests. Needed by the Hive testsgo run ./hive.go --client ethereumjs --sim ethereum/engine --sim.limit "engine-transition/Single Block PoW Re-org to Higher-Total-Difficulty Chain, Equal Height"
extradata
that does not conform to PoW extradata requirements