explicit chainParams check #5030

winsvega · 2018-05-16T16:04:27Z

Instead of validating fields names with c_fieldname constant
I moved requireJson function to libdevcode
so you could specify how json object should look like, and what fields it must have and of what time those fields could be.

example code in validating the chainParams. requires review.
this is a step toward understanding what chain params are required and what could be set optional.

codecov-io · 2018-05-16T19:46:26Z

Codecov Report

Merging #5030 into develop will decrease coverage by 0.33%.
The diff coverage is 81.6%.

@@             Coverage Diff             @@
##           develop    #5030      +/-   ##
===========================================
- Coverage    58.71%   58.37%   -0.34%     
===========================================
  Files          334      346      +12     
  Lines        26424    26888     +464     
  Branches      3133     3228      +95     
===========================================
+ Hits         15514    15696     +182     
- Misses        9909    10180     +271     
- Partials      1001     1012      +11

pirapira · 2018-05-21T18:24:42Z

libdevcore/JsonUtils.h

+
+// Check json _o with validation map that reuires certain field of certain type to be present in
+// json
+typedef std::set<json_spirit::Value_type> possibleJsonType;


For me jsonTypeSet sounds better.

First I thought "possibly it is a json type, but possibly it is not a json type".

Right, JsonTypeSet would be better.
Also better use using instead of typedef.
And better place type definitions at the top, before function definitions.

pirapira · 2018-05-21T18:35:14Z

libdevcore/JsonUtils.cpp

+        {
+            if (sTypes.size())
+                sTypes += ", or ";
+            sTypes += jsonTypeAsString(type);


You need this concatenation only when there is an error.

so you think two loops on for (auto const& type : vmap.second) is better ?

winsvega · 2018-05-21T20:28:17Z

ethereum/EIPs#1085

winsvega · 2018-05-22T10:09:37Z

@gumb0 @pirapira
Cpp client has specific declaration of a precompiled contracts in the genesis state.

My question is do we really need it? Precompiled contracts are hardcoded anyway. non other clients support this specific account desctiption anyway. retesteth and hive has to prepare a specific genesis for cpp in this case. could we move those params to hardcode of the client?

"0000000000000000000000000000000000000001": { "precompiled": { "name": "ecrecover", "linear": { "base": 3000, "word": 0 } } },
		"0000000000000000000000000000000000000002": { "precompiled": { "name": "sha256", "linear": { "base": 60, "word": 12 } } },
		"0000000000000000000000000000000000000003": { "precompiled": { "name": "ripemd160", "linear": { "base": 600, "word": 120 } } },
		"0000000000000000000000000000000000000004": { "precompiled": { "name": "identity", "linear": { "base": 15, "word": 3 } } },
		"0000000000000000000000000000000000000005": { "precompiled": { "name": "modexp" } },
		"0000000000000000000000000000000000000006": { "precompiled": { "name": "alt_bn128_G1_add", "linear": { "base": 500, "word": 0 } } },
		"0000000000000000000000000000000000000007": { "precompiled": { "name": "alt_bn128_G1_mul", "linear": { "base": 40000, "word": 0 } } },
		"0000000000000000000000000000000000000008": { "precompiled": { "name": "alt_bn128_pairing_product" } }

gumb0 · 2018-05-22T16:04:59Z

I think we should be able to get rid of them.
I actually don't like that we now need to duplicate the fork block number in startingBlock for precompiles that appear at Byzantium. And flexibility with setting up gas cost formula looks like not needed complexity.

On the other hand I think ewasm team requested or even implemented the feature with config file to refer to external files with precompiled code when they want to create wasm-precompiles. Not sure what to do with that.

cc @chfast

gumb0 · 2018-05-22T16:15:56Z

libethereum/ChainParams.cpp

@@ -98,7 +133,8 @@ ChainParams ChainParams::loadConfig(
 	json_spirit::read_string_or_throw(_json, val);
 	js::mObject obj = val.get_obj();

-	validateFieldNames(obj, c_knownChainConfigFields);
+    validateConfigJson(obj);
+    validateFieldNames(obj, c_knownChainConfigFields);


Did you mean to leave both of these validations here?
So now we have two separate list of allowed fields to maintain?

the other one could be removed from this place. once we come to a standart in genesis config.
the question remains is wether we should use constants instead of strings in scheme validation functions.

I don't think we should validate it two times. So let's not merge it then until we have final version

pirapira · 2018-05-22T17:36:11Z

@winsvega @gumb0 I have no opinions around precompiles. If WASM people need a place, the format should allow that as an optional field.

winsvega · 2018-05-22T18:10:29Z

optional fields are ok.
but in the current implementation it is required.
otherwise even if you set the fork block in configuration, the client wont initialize a precompiled for the forkRules.

gumb0 · 2018-05-23T13:32:20Z

libethereum/ChainParams.cpp

@@ -98,7 +133,8 @@ ChainParams ChainParams::loadConfig(
 	json_spirit::read_string_or_throw(_json, val);
 	js::mObject obj = val.get_obj();

-	validateFieldNames(obj, c_knownChainConfigFields);
+    validateConfigJson(obj);
+    validateFieldNames(obj, c_knownChainConfigFields);


I don't think we should validate it two times. So let's not merge it then until we have final version

gumb0 · 2018-05-23T13:33:10Z

libethereum/ChainParams.cpp

+void validateConfigJson(js::mObject const& _obj)
+{
+    requireJsonFields(_obj, "ChainParams::loadConfig",
+        {{"sealEngine", {json_spirit::str_type}}, {"params", {json_spirit::obj_type}},


So why did you choose not to use the constants for the field names here?

I guess this check is like a constant itself. if it does not match, then it throws an error.
so if you have a typo, it will break the test run.
although it is said that it is not good to use char in the code this way. maybe we need a dictionary class that would return a string by id for all text checks and messages in the client.

We shouldn't have duplicate string literals in the code here, it's maintenance pain and might waste resources. So use here c_sealEngine, c_params etc.

gumb0 · 2018-05-23T13:34:21Z

libdevcore/JsonUtils.cpp

+    }
+}
+
+void dev::requireJsonFields(json_spirit::mObject const& _o, std::string const& _config,


I'm not sure about having it in libdevcore

the previous checks were put into this file. @pirapira ?

If there is a better place, feel free to move.

so we could remove jsonUtils from libdevcore. and keep it in libethereum. but I dont think its a better choise

If libdevcore or libethereum, I'd vote for libdevcore. This is not so particularly about Ethereum.

winsvega · 2018-05-28T15:53:16Z

looks like client devs wont come up with the common scheme any time soon.
I suggest we figure out the minimum required config fields for cpp and create our own scheme validation.

pirapira · 2018-05-29T17:17:22Z

libethereum/ValidationSchemes.cpp

+        // A precompiled contract
+        requireJsonFields(_obj, "validateAccountObj",
+            {{c_precompiled, {json_spirit::obj_type}}},
+            {c_wei});


Don't you want wei as str_type?

Oh, it's not required?

pirapira · 2018-05-29T17:20:14Z

libdevcore/JsonUtils.h

+// Check json _o with validation map that reuires certain field of certain type to be present in
+// json
+using jsonTypeSet = std::set<json_spirit::Value_type>;
+void requireJsonFields(json_spirit::mObject const& _o, std::string const& _config,


Please document this function.

chfast · 2018-05-29T21:35:51Z

Have you considered using JSON Schema to specify and validate JSON configs? I've found validator for JSON Schema: https://github.com/tristanpenman/valijson.

winsvega · 2018-05-30T10:51:40Z

Hmm. That repo looks like a lot of code to add for dependencies.
Here we have one small function that does a verification

pirapira · 2018-05-30T15:40:38Z

libdevcore/JsonUtils.h

+};
+using jsonTypeSet = std::set<json_spirit::Value_type>;
+using jsonType = std::pair<jsonTypeSet, jsonField>;
+void requireJsonFields(json_spirit::mObject const& _o, std::string const& _config,


The function does a little bit different job from what "require Json Fields" suggests.

Please document the function.

could you clarify what "document the function" should look like ?

gumb0 · 2018-06-12T15:51:06Z

libdevcore/JsonUtils.h

+// Converts json value type to string
+std::string jsonTypeAsString(json_spirit::Value_type _type);
+
+enum jsonField


enum class is preferable to enum. Also start type names with capital letter. Also this should be better named something like JsonFieldPresence maybe

gumb0 · 2018-06-12T15:51:25Z

libdevcore/JsonUtils.h

+    Required,
+    Optional
+};
+using jsonTypeSet = std::set<json_spirit::Value_type>;


Start type name with capital letter

gumb0 · 2018-06-12T15:52:44Z

libdevcore/JsonUtils.h

+    Optional
+};
+using jsonTypeSet = std::set<json_spirit::Value_type>;
+using jsonType = std::pair<jsonTypeSet, jsonField>;


Start type name with capital letter, also this is something like JsonTypesAndPresence I'd say

gumb0 · 2018-06-12T16:08:58Z

libdevcore/JsonUtils.cpp

+        if (matched == false)
+        {
+            std::string sTypes;
+            for (auto const& type : vmap.second.first)


I'd use boost::algorithm::join here instead of reinventing joining the strings, but it's up to you

gumb0 · 2018-06-12T16:10:08Z

libdevcore/JsonUtils.cpp

+                sTypes += jsonTypeAsString(type);
+            }
+            std::string const comment =
+                "Field '" + vmap.first + "' expected to be " + sTypes + ", but set to " +


is (or was) expected to be, but is (or was) set

gumb0 · 2018-06-12T16:13:59Z

libethereum/ValidationSchemes.h

+    You should have received a copy of the GNU General Public License
+    along with cpp-ethereum.  If not, see <http://www.gnu.org/licenses/>.
+*/
+/** @file ValidationSchemes.h


remove these @file and @date
Use @file only if you provide some meaningful description of the file

gumb0 · 2018-06-12T16:22:00Z

libethereum/Account.cpp

 		// FIXME: Do not copy every account object.
 		auto o = account.second.get_obj();
+        validateAccountMapObj(o);


Why do you do this now inside the loop? Validating the whole map object in each iteration?

checking one account instead of a list of accounts

Ah wow, don't use o for both the whole map and one account object.
Use more meaningful names than o anyway

gumb0 · 2018-06-12T16:37:08Z

libethereum/ValidationSchemes.h

+namespace eth {
+namespace validation {
+
+extern std::string const c_sealEngine;


I liked these constants more in ChainParams.cpp / Account.cpp ... where they are closer to the actual useful work being done with them.
Are you sure with moving all of this into separate file? I'd leave the three validation functions closer to the other work with the json configs

this constants could grow over time into a big amount of variables

gumb0 · 2018-06-12T17:05:57Z

libethereum/ValidationSchemes.h

+void validateAccountMapObj(json_spirit::mObject const& _o);
+
+
+}}}


This doesn't look like it was clang-formatted

gumb0 · 2018-06-13T10:46:35Z

libethereum/ValidationSchemes.cpp

+            {c_precompiled, {{json_spirit::obj_type}, JsonFieldPresence::Optional}},
+            {c_shouldnotexist, {{json_spirit::str_type}, JsonFieldPresence::Optional}},
+            {c_wei, {{json_spirit::str_type}, JsonFieldPresence::Optional}}});
+    }


Indentation is off here. Use clang-format

I used clang-format

gumb0 · 2018-06-13T10:48:03Z

libethereum/ValidationSchemes.cpp

+#include <string>
+
+using namespace std;
+using namespace dev;


You don't need this, everything is defined inside dev anyway.
You probably don't need using namespace std; either

gumb0 · 2018-06-13T10:48:42Z

libethereum/ValidationSchemes.cpp

+
+using namespace std;
+using namespace dev;
+namespace js = json_spirit;


You don't use this below, it's json_spirit everywhere. So maybe just delete it.

gumb0 · 2018-06-13T10:51:01Z

libethereum/ValidationSchemes.cpp

+        }
+        else
+        {
+            // A standart account with all fields


What's the difference between standard account and genesis account? Do these standard accounts really always have code and storage?

yes. standart account required to have all 4 fields.
in genesis it is not required.
and account map could be empty

gumb0 · 2018-06-13T10:52:08Z

libethereum/ChainParams.cpp

@@ -27,15 +27,16 @@
 #include <libethcore/SealEngine.h>
 #include <libethcore/BlockHeader.h>
 #include <libethcore/Precompiled.h>
+#include "ValidationSchemes.h"


Use clang-format here, it should reorder includes

gumb0 · 2018-06-13T14:02:54Z

libethereum/ValidationSchemes.cpp

+    }
+    else
+    {
+        // A standart account with all fields


In which configs are they used?

Ah it's allowed to have in genesis accounts either balance only or all 4 fields? ok then

In tests also

gumb0 · 2018-06-13T14:08:00Z

libethereum/ValidationSchemes.h

+// Validate account json object
+void validateAccountObj(json_spirit::mObject const& _o);
+
+// Validate account Map json object. Map indicates which fields are set


Second part of comment is not needed I guess?

the confusion is because it should be account mask here

gumb0 · 2018-06-13T14:17:57Z

libethereum/Account.cpp

 		// FIXME: Do not copy every account object.
-		auto o = account.second.get_obj();
+        auto const& accountMapJson = account.second.get_obj();


Maybe just accountJson or accountObject, because acountMap sounds like a map of accounts, but as I understand it refers to single account

Map is because it indicates which fields are set and which are not. there is a class for that: AccountMask. should be renamed to accountMask to avoid confusion

gumb0 · 2018-06-13T14:19:56Z

libethereum/ValidationSchemes.h

+void validateAccountObj(json_spirit::mObject const& _o);
+
+// Validate account Map json object. Map indicates which fields are set
+void validateAccountMapObj(json_spirit::mObject const& _o);


Also this name, if it validates just one account object I would name it differently

accountMaskObj

gumb0 · 2018-06-13T14:22:45Z

libethereum/ChainParams.cpp

+    // Strict account check
+    js::read_string_or_throw(genesisStateStr, val);
+    for (auto const& account: val.get_obj())
+        validateAccountObj(account.second.get_obj());


Do you need this here at all? You call validateConfigJson(obj); above and inside it it already goes over all accounts, no?

gumb0 · 2018-06-14T12:11:41Z

libethereum/ValidationSchemes.h

+void validateConfigJson(json_spirit::mObject const& _obj);
+
+// Validate account json object
+void validateAccountObj(json_spirit::mObject const& _o);


This is now used only by validateConfigJson so make it internal to ValidationSchemes.cpp, remove from header

I don't think it shoud be removed. it should be used in testeth.

gumb0 · 2018-06-14T12:37:08Z

I'm confused now about these account masks.

dev::eth::jsonToAccountMap() function parses json with accounts and returns all accounts as AccountMap. It also produces AccountMaskMap but that's a side-effect, not it's main purpose.

Now you want to validate input json to this function. Input json has account description, not "account masks". So the validation of json of each account it seems should be called something like validateAccountJsonObject() not validateAccountMaskObj()

Also: I'm looking at ChainParams::loadConfig() which is supposed to parse config including genesis header and accounts. Now it looks like it would validate

first the accounts list inside validateConfigJson() (calling validateAccountObj())
and then it would validate the same account list inside dev::eth::jsonToAccountMap() calling validateAccountMaskObj() for each.

So do you even need both of validateAccountObj() and validateAccountMaskObj()?
It looks to me like that they both validate the same json structure but in a different way for some weird reason.

winsvega · 2018-06-14T14:12:38Z

Could we have a call?

winsvega · 2018-06-14T14:22:49Z

your confusion might be because

accountMask is totally a different object. it is not a json object of account.
accountMask used only in the tests. but for some reason Gavin implemented it in libethereum.

double check in jsonToAccountMap is because we don't store data in objects. instead we pass arguments from function to function and this function here jsonToAccountMap could not rely on the correctness of the input data. what would be better is an abstraction layer between json data and logical objects. so once you read json data into those objects you wont need to verify every input argument of the function.

gumb0 · 2018-06-14T14:38:42Z

Sure, ping me in discord for a call, you're offline now

Dismissing Yoichi's review as he says he is not available for doing reviews at the moment

winsvega requested a review from pirapira May 16, 2018 16:04

winsvega force-pushed the smallrpc branch 2 times, most recently from 98a1af7 to 9b9d269 Compare May 16, 2018 17:20

pirapira suggested changes May 21, 2018

View reviewed changes

gumb0 self-requested a review May 22, 2018 10:10

gumb0 reviewed May 22, 2018

View reviewed changes

gumb0 suggested changes May 23, 2018

View reviewed changes

gumb0 reviewed May 23, 2018

View reviewed changes

winsvega added 2 commits May 28, 2018 22:52

explicit chainParams check

7edcb24

fix tests

49e1e24

winsvega force-pushed the smallrpc branch from 7372e65 to d5d6a86 Compare May 28, 2018 19:53

pirapira reviewed May 29, 2018

View reviewed changes

pirapira suggested changes May 29, 2018

View reviewed changes

winsvega force-pushed the smallrpc branch from d5d6a86 to ed136ce Compare May 29, 2018 21:16

pirapira previously requested changes May 30, 2018

View reviewed changes

add validation schemes

41dc392

winsvega force-pushed the smallrpc branch from ed136ce to 41dc392 Compare June 7, 2018 14:52

gumb0 reviewed Jun 12, 2018

View reviewed changes

winsvega force-pushed the smallrpc branch from c3a27ff to d83f34b Compare June 12, 2018 19:26

gumb0 reviewed Jun 13, 2018

View reviewed changes

winsvega force-pushed the smallrpc branch from d83f34b to f412526 Compare June 13, 2018 12:07

gumb0 reviewed Jun 13, 2018

View reviewed changes

refactor validation schemes

2dd0bf0

winsvega force-pushed the smallrpc branch from f412526 to 2dd0bf0 Compare June 13, 2018 19:00

gumb0 reviewed Jun 14, 2018

View reviewed changes

remove double check of accountMask upon loading chain config

5549af4

gumb0 approved these changes Jun 14, 2018

View reviewed changes

winsvega merged commit b859d94 into develop Jun 15, 2018

gumb0 deleted the smallrpc branch June 15, 2018 09:09

		void validateAccountMapObj(json_spirit::mObject const& _o);


		}}}

explicit chainParams check #5030

explicit chainParams check #5030

Conversation

winsvega commented May 16, 2018

codecov-io commented May 16, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winsvega commented May 21, 2018

winsvega commented May 22, 2018

gumb0 commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirapira commented May 22, 2018

winsvega commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winsvega commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented May 29, 2018

winsvega commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winsvega Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Jun 14, 2018 • edited Loading

winsvega commented Jun 14, 2018

winsvega commented Jun 14, 2018 • edited Loading

gumb0 commented Jun 14, 2018

codecov-io commented May 16, 2018 •

edited

Loading

winsvega Jun 13, 2018 •

edited

Loading

gumb0 Jun 13, 2018 •

edited

Loading

gumb0 commented Jun 14, 2018 •

edited

Loading

winsvega commented Jun 14, 2018 •

edited

Loading