spec loading cleanup

[debris]

the goal of this pr is to cleanup loading spec and get rid of all unwraps (or at least as many as possible).

changes:

• in json spec files:
  • replaced engineName with engine
  • moved engine specific params to engine field
  • seal fields are grouped together
• completely removed usages of FromJson trait from ethcore
• removed toEngine method from spec. engine is now a spec property
• spec is no longer a property of engine
• engine params are no longer decoded from rlp.

This pr is so big mostly because I had to change a lot of test functions...

todo:

• update spec wiki

[NikVolf]

looks great

[gavofyork]

trailing comma missing.

[gavofyork]

this is a rather disstressing change.

the engine factory was a temporary place-holder for a dynamically-loaded plugin architecture; this change doesn't help achieve that goal.

it rearchitects the Spec/Engine ownership relationship, reducing Engine to a second-class utility type unable to be used without an owning Spec. at best, the name(s) need changing. at worst there are unforseen consequences of the design alteration (e.g. can we have an Engine in test code without needing a JSON description or using the placeholder Engine factory?) - keeping the design centred around Engine (which actually does the work) and not a JSON deserialisation type was an important design consideration. there is no clear addressing of the consequences of the design alteration.

[gavofyork]

removing unwraps is great and all, but what happens if the data of the json is in incorrect format? there's really only three possibilities:

• panic (no better than unwrap);
• bubble-up error on call stack (i don't see any try!s);
• silently give default value (probably just bad).

[debris]

I will try address all questions and concerns here:

removing unwraps is great and all, but what happens if the data of the json is in incorrect format?

Currently there is no single place where whole json spec is validated. we have multiple unwraps in spec, builtin and verification. Besides that, there are try in client constructor. In my pr, the whole json spec is validated deserialized in a single place.

Spec is a specification that describes the consensus engine. It was not designed to be used as a parent instance.

This may be my misunderstanding... I see spec as a chain specification. And engine as a chain property.

Refactoring it in this way probably has unforseen consequences. Engine is meant to be the parent type to be passed around.

I had the same concerns, but then I noticed that the only spec function that we use* (excluding Engine) is ensure_db_good. And it's used only just after spec is loaded.

it rearchitects the Spec/Engine ownership relationship, reducing Engine to a second-class utility type unable to be used without an owning Spec.

I think it's the other way around. On master branch, it's very difficult to create Ethash engine without loading json spec. Now it's trivial.

at best, the name(s) need changing. at worst there are unforseen consequences of the design alteration (e.g. can we have an Engine in test code without needing a JSON description or using the placeholder Engine factory?) - keeping the design centred around Engine (which actually does the work) and not a JSON deserialisation type was an important design consideration. there is no clear addressing of the consequences of the design alteration.

Using JSON is completely optional and nothing relies on it now.

the engine factory was a temporary place-holder for a dynamically-loaded plugin architecture; this change doesn't help achieve that goal.

I'm aware of that. And there is the same issue with genesis seal. That's true that this pr doesn't help achieve that goal, but imo it does not make it more difficult.

[debris]

I understand that this pr introduces two things that could be done separately (deserialization && object ownership refactoring), but it was just much easier to do this things together.

[gavofyork]

Engine certainly isn't a chain property. It's an auxiliary, used for much of the chain mechanics which may be defined (and thus constructed) by a portion of the chain Spec.

[gavofyork]

why removed?

[debris]

Exactly the same structure was defined in ethcore/src/tests/helpers.rs

[gavofyork]

probably good to go in, but i'd reconsider the naming in light of the architecture alteration.

[debris]

Do you have any suggestions?

[gavofyork]

Having thought about it Chain or ChainSpec is the only thing I can think of which sounds as though it "owns" Engine. But I don't really like double-barrel names and just "Chain" doesn't seem much better than "Spec".