test_getPostState #4972

winsvega · 2018-04-20T12:40:48Z

return current actual post state of the blockchain.

pirapira · 2018-04-20T12:56:21Z

libweb3jsonrpc/Test.cpp

+            continue;
+
+        Json::Value o;
+        if (mapEmpty || _map.at(a.first).hasBalance())


Why is this empty case treated in a special way?

this is actually a copy of the same method from tests. but that one works with json_spirit.
I guess the account map here could be removed. because it is needed for expect sections with incomplete post states.

codecov-io · 2018-04-20T13:30:54Z

Codecov Report

Merging #4972 into develop will decrease coverage by 0.13%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #4972      +/-   ##
===========================================
- Coverage    61.08%   60.95%   -0.14%     
===========================================
  Files          350      350              
  Lines        28303    28379      +76     
  Branches      2889     2893       +4     
===========================================
+ Hits         17290    17299       +9     
- Misses       10049    10118      +69     
+ Partials       964      962       -2

pirapira · 2018-04-20T14:21:37Z

libweb3jsonrpc/Test.cpp

+        if (_state.code(a.first).size() > 0)
+            o["code"] = toHexPrefixed(_state.code(a.first));
+        else
+            o["code"] = "";


Do you really need to separate these two cases? What's wrong if o["code"] becomes 0x?

you right. changed this now

pirapira · 2018-04-20T14:23:51Z

libweb3jsonrpc/Test.cpp

+        else
+            return toJS(EmptyListSHA3);
+    }
+    return "";


I think this case should be an error, rather than just an empty reply.

how do we report an error via RPC reply ?

I don't know, look at other methods.

well there is BOOST_THROW_EXCEPTION(JsonRpcException(Errors::ERROR_RPC_INVALID_PARAMS));

but it stops the client. which is not a good thing. you dont want a client to stop on malicious rpc

It shouldn't stop the client, all RPC methods report errors kind of like that and the last time I tried it I saw correct error message on the caller side.

Could you investigate whether it crashes because of unhandled exception or for other reason?

Also related issue is #4726

no. it does not crash the client. I was wrong. the rpc error is returned.

pirapira · 2018-04-20T14:26:23Z

libweb3jsonrpc/Test.cpp

+{
+    Json::Value out;
+    Json::FastWriter fastWriter;
+    if (u256(param1["version"].asString()) == c_postStateJustHashVersion)


Does the RPC method take 1, 2, or 3, indicating different versions? In other parts of the RPC, "latest" means "latest", and so on. I guess strings like "jushash" "fullstate" or "loghash" are easier.

no problem here. it could be anything.
the discussion is here
ethereum/retesteth#7

winsvega · 2018-04-20T16:37:25Z

@pirapira do you have any more proposals?
I have one change: missing method note in test.json file

winsvega · 2018-04-20T16:38:35Z

Actually should this method be a test method only ?

gumb0 · 2018-04-23T09:57:49Z

API-design-wise it should better be three different API methods instead of one method doing completely different things based on "version" param
Like getStateRootHash, getState... and I'm not sure why you need the last one

gumb0 · 2018-04-23T09:58:23Z

libweb3jsonrpc/Test.cpp

+    else if (param1["version"].asString() == c_postStateLogHashVersion)
+    {
+        if (m_eth.blockChain().receipts().receipts.size() != 0)
+            return exportLog(m_eth.blockChain().receipts().receipts[0].log());


This gets the receipt of only the first transaction of the block, is this your intention?

Couldn't you just use receipts root which is in the block header?

gumb0 · 2018-04-23T10:01:16Z

libweb3jsonrpc/Test.cpp

+    Json::Value out;
+    Json::FastWriter fastWriter;
+    if (param1["version"].asString() == c_postStateJustHashVersion)
+        return toJS(m_eth.postState().state().rootHash());


Is this any different from getting eth.getBlock("latest").stateRoot ?

gumb0 · 2018-04-23T10:03:04Z

libweb3jsonrpc/Test.cpp

@@ -33,6 +34,60 @@ using namespace jsonrpc;

 Test::Test(eth::Client& _eth): m_eth(_eth) {}

+Json::Value fillJsonWithState(eth::State const& _state)


Put free functions into the unnamed namespace

winsvega · 2018-04-23T12:18:55Z

Casey said there are debug methods specified for mainnet.
ethereum/retesteth#5


I'd favor a simpler primitive that would also be useable on the mainnet: 
debug_accountRangeAt(blockNum, startKey, limit), inspired by storageRangeAt. 
The method would iterate over the leaves of the state trie (starting at startKey), 
and return a list of address hashes (up to the limit). 
Address hashes are returned because most clients (fast sync'd clients in particular) 
do not have the hash preimages, and it is not much effort to maintain
 the hash preimages separately (if the client has them, they can be queried with debug_preimage. 
They can also be imported/exported separately).
 The storage for each account can then be dumped with storageRangeAt.

this getPost state method should probably be implemented with those debug methods.

chfast · 2018-04-23T13:03:31Z

API-design-wise it should better be three different API methods instead of one method doing completely different things based on "version" param

I'm also for it. It's better to have many simple RPC methods than not many but complex RPC methods. Having simple ones you can who is using what and why. And also able to deprecate individual methods in future.

winsvega · 2018-04-24T17:20:24Z

This method will be replaced by

debug_accountRangeAt
debug_storageRangeAt

which could work on mainnet. with the first one you could select accounts in state at specific block after specific transaction.

with the second you could select post state values of a specific account in a specific block after specific transaction.

winsvega requested review from pirapira and gumb0 April 20, 2018 12:40

pirapira reviewed Apr 20, 2018

View reviewed changes

winsvega force-pushed the smallrpc3 branch from 834ea6d to c12fcf6 Compare April 20, 2018 14:03

pirapira reviewed Apr 20, 2018

View reviewed changes

winsvega force-pushed the smallrpc3 branch 2 times, most recently from 23615c2 to d37e20c Compare April 20, 2018 15:09

test_getPostState

a186c9e

winsvega force-pushed the smallrpc3 branch from d37e20c to a186c9e Compare April 20, 2018 18:18

gumb0 reviewed Apr 23, 2018

View reviewed changes

winsvega closed this Apr 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_getPostState #4972

test_getPostState #4972

winsvega commented Apr 20, 2018 •

edited

Loading

pirapira Apr 20, 2018

winsvega Apr 20, 2018

codecov-io commented Apr 20, 2018 •

edited

Loading

pirapira Apr 20, 2018

winsvega Apr 20, 2018

pirapira Apr 20, 2018

winsvega Apr 20, 2018

pirapira Apr 20, 2018

winsvega Apr 20, 2018

gumb0 Apr 20, 2018

gumb0 Apr 20, 2018

winsvega Apr 20, 2018

pirapira Apr 20, 2018

winsvega Apr 20, 2018

winsvega commented Apr 20, 2018

winsvega commented Apr 20, 2018

gumb0 commented Apr 23, 2018

gumb0 Apr 23, 2018

gumb0 Apr 23, 2018

gumb0 Apr 23, 2018

gumb0 Apr 23, 2018

winsvega commented Apr 23, 2018 •

edited

Loading

chfast commented Apr 23, 2018

winsvega commented Apr 24, 2018

		@@ -33,6 +34,60 @@ using namespace jsonrpc;

		Test::Test(eth::Client& _eth): m_eth(_eth) {}

		Json::Value fillJsonWithState(eth::State const& _state)

test_getPostState #4972

test_getPostState #4972

Conversation

winsvega commented Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 20, 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

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 Apr 20, 2018

winsvega commented Apr 20, 2018

gumb0 commented Apr 23, 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

winsvega commented Apr 23, 2018 • edited Loading

chfast commented Apr 23, 2018

winsvega commented Apr 24, 2018

winsvega commented Apr 20, 2018 •

edited

Loading

codecov-io commented Apr 20, 2018 •

edited

Loading

winsvega commented Apr 23, 2018 •

edited

Loading