Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

parse different versions of genesis config #4973

Closed
wants to merge 1 commit into from
Closed

Conversation

winsvega
Copy link
Contributor

@winsvega winsvega commented Apr 20, 2018

parse test genesis format
ethereum/retesteth#4

# Genesis config for running a StateTest via RPC
{
       "version" : "1",
       "params": {
            "miningMethod" : "NoProof",
	    "forkRules" : "Frontier",
            "blockReward" : "0x00"
       },
       "genesis" : {
            "coinbase" : "0x2adc25665018aa1fe0e6bc666dac8fc2697ff9ba",
            "gasLimit" : "0x0f4240",
            "timestamp" : "0x03e8"
       },
       "state": {
            "0x095e7baea6a6c7c4c2dfeb977efac326af552d87" : {
                "balance" : "0x0de0b6b3a76586a0",
                "code" : "0x6001600101600055",
                "nonce" : "0x00",
                "storage" : {
                    "0x00" : "0x02"
                }
            },
            "0x2adc25665018aa1fe0e6bc666dac8fc2697ff9ba" : {
                "balance" : "0x29a2241af62ca034",
                "code" : "",
                "nonce" : "0x00",
                "storage" : {
                }
            }
       }
}

@winsvega winsvega requested review from pirapira and gumb0 April 20, 2018 19:19
@codecov-io
Copy link

codecov-io commented Apr 20, 2018

Codecov Report

Merging #4973 into develop will decrease coverage by 0.03%.
The diff coverage is 2.12%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4973      +/-   ##
===========================================
- Coverage    61.08%   61.05%   -0.04%     
===========================================
  Files          350      350              
  Lines        28303    28359      +56     
  Branches      2889     2885       -4     
===========================================
+ Hits         17290    17315      +25     
- Misses       10049    10091      +42     
+ Partials       964      953      -11

@pirapira
Copy link
Member

What is the benefit of having a new format?

@@ -77,10 +77,13 @@ struct ChainOperationParams
/// General chain params.
private:
u256 m_blockReward;
bool m_allowRewardOverwrite; //allow overwrite of the block reward that is set in genesis.json
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need this hack?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without this, Byzantium blockreward is reset to non zero value even if I want to run Byzantium tests with 0 mining reward that I set in chain params.

Copy link
Member

Choose a reason for hiding this comment

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

You're not running Byzantium rules then. Maybe create another config similar to Byzantium but without block reward?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well then I do not run any rules then. because I use 0 block reward for all tests with NoProof
the issue is that Byzantium set is overwriting the block reward and I dont have any means to prevent it from config.

Copy link
Member

Choose a reason for hiding this comment

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

You need to create new EVMSchedule in EVMSchedule.h, add it to ChainOperationParams::scheduleForBlockNumber(), then use new param like "byzantiumTestForkBlock" in actual chain config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isn't it too much just for the test purpose?

Copy link
Member

Choose a reason for hiding this comment

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

Well ad-hoc hacking lower-level data structures and mechanisms just for the test purpose seems too much to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is not really a hack. its an option to set mining reward in genesis config

Copy link
Member

Choose a reason for hiding this comment

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

To make it work you could also try just overriding NoProof::blockReward() and always returning 0 from there.

Copy link
Member

Choose a reason for hiding this comment

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

Though I wouldn't fully support such change, because it makes NoProof engine further away from real network mechanics and for unit tests I would like it to stay closer. But if you'd made another engine just for your tests like NoProofZeroRewards that would be fine I guess.

@winsvega
Copy link
Contributor Author

@pirapira this is a minimalistic genesis format for running a test.
besides every client has its own unique genesis format. it would be good to have a common genesis format for test executions via rpc.

@gumb0
Copy link
Member

gumb0 commented Apr 23, 2018

Still not clear what's exactly wrong for you with our regular genesis format.

Provide an example of new format please

@winsvega
Copy link
Contributor Author

ethereum/retesteth#7

@winsvega
Copy link
Contributor Author

winsvega commented Apr 24, 2018

so basically there should be setGenesis method which allows you to enable/disable or set mining algorythm (PoW, NoProof) and initial state and genesis block header.
Plus it should be possible to set chain rules. (homestead, eip158)

the issue is that currently each client has its own genesis config format

@gumb0
Copy link
Member

gumb0 commented Apr 24, 2018

So is this a special kind of config where instead of fork block numbers you just set which fork it is and it is in effect for the whole chain starting from genesis?

@winsvega
Copy link
Contributor Author

winsvega commented Apr 24, 2018

yes and no. we define Frontier as a test network that has Frontier rules enabled by default and so on for other forks. but we also have FrontierToHomesteadAt5 which has a transition at block 5.

so I see, you mean that chain options like forkBlock number should also be set in this method probably.
thus there will be no need to set Frontier and EIP158 names.

the issue is still that such transition block option names are depend on a client.

Copy link
Member

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

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

Anyway I don't think it should be merged until format in ethereum/retesteth#4 is finalized

@winsvega
Copy link
Contributor Author

winsvega commented May 7, 2018

Looks like the current set_chainParams method. (chain param init function) needs refactoring.
And we could support default values for the fields that are not set in .json

@winsvega winsvega closed this May 16, 2018
@chfast chfast deleted the smalltrpc branch August 2, 2018 08:58
@gumb0 gumb0 mentioned this pull request Jun 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants