string test_getLogHash(txHash) #4986

winsvega · 2018-04-27T20:32:42Z

string test_getLogHash(string _txHash)
Get pre-calculated log has of the _txHash. hash of emptyRLPList if transaction not found.
Log hash could be calculated from transaction receipt, but some transactions has too many logs, so we need a method to get just the log hash.

looks like this would be an optional method. because it is not required for state tests to check log hash.

ethereum/retesteth#7

pirapira · 2018-04-27T20:43:15Z

libweb3jsonrpc/Test.cpp

+        if (blockReceipts.receipts.size() != 0)
+            return exportLog(blockReceipts.receipts[t.transactionIndex()].log());
+     }
+     return toJS(dev::EmptyListSHA3);


Indentation around here is a bit strange. Did you run git-clang-format?

codecov-io · 2018-04-27T21:29:59Z

Codecov Report

Merging #4986 into develop will increase coverage by 0.27%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #4986      +/-   ##
===========================================
+ Coverage    60.67%   60.94%   +0.27%     
===========================================
  Files          350      350              
  Lines        28338    28368      +30     
  Branches      2864     2894      +30     
===========================================
+ Hits         17193    17289      +96     
+ Misses       10198    10115      -83     
- Partials       947      964      +17

gumb0 · 2018-04-30T13:24:52Z

What about logs bloom filter in transaction receipt - couldn't you just use it instead of this hash?
It's just 256 bytes long.

winsvega · 2018-04-30T16:05:56Z

current tests has log hash.
ask @felix from the go team. he might sometimes be in the berlin office.
He is mainly the one who uses this log hash information.

gumb0 · 2018-05-02T13:41:13Z

Ok, so it's a part of the filled test format, I see, ok then I guess we need it for client compatibility

gumb0 · 2018-05-02T13:41:51Z

libweb3jsonrpc/Test.cpp

 using namespace dev::rpc;
 using namespace jsonrpc;

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

+string exportLog(eth::LogEntries const& _logs)


Put free functions into unnamed namespace

"export" seems too generic in this context, I'd name it logsToRlpListHex

gumb0 · 2018-05-02T13:46:13Z

libweb3jsonrpc/Test.cpp

+        if (blockReceipts.receipts.size() != 0)
+            return exportLog(blockReceipts.receipts[t.transactionIndex()].log());
+    }
+    return toJS(dev::EmptyListSHA3);


Why should it be empty list in case transaction not found? This way the user of the method can't understand whether the transaction produced no log or it doesn't exist at all.

Maybe we should return error in case transaction not found?

hmm. that is a good question. the issue is that we don't have like interclient standart for this case.
as it is a test method. do you think its ok to return 0x00 hash in case of error?

I think returning error is cleaner than magic value of hash.
You should also be able to make the error more specific by adding error message like throw JsonRpcException(Errors::ERROR_RPC_INVALID_PARAMS, "Transaction not found")

Or you could introduce new error code and return it like throw JsonRpcException(TxNotFoundErrorCode)

Maybe check what geth returns from eth_getTransactionByHash in this case

ideally all json requests response should have a common scheme that all clients follow.

gumb0 · 2018-05-02T13:47:41Z

libweb3jsonrpc/Test.cpp

+
+string Test::test_getLogHash(string const& _txHash)
+{
+    if (m_eth.blockChain().isKnownTransaction(h256(_txHash)))


What happens when you send here invalid hash string? Does it crash or is appropriate error returned?

terminate called after throwing an instance of 'boost::exception_detail::clone_impl<dev::BadHexCharacter>' what(): /home/wins/Ethereum/cpp-ethereum/libdevcore/CommonData.cpp(93): Throw in function dev::bytes dev::fromHex(const string&, dev::WhenError) Dynamic exception type: boost::exception_detail::clone_impl<dev::BadHexCharacter>

So you should catch it here and rethrow as JsonRpcException, then method will correctly return error.

gumb0 · 2018-05-02T13:51:37Z

libweb3jsonrpc/Test.h

@@ -42,8 +42,8 @@ class Test: public TestFace
 	{
 		return RPCModules{RPCModule{"test", "1.0"}};
 	}
-
-	virtual bool test_setChainParams(const Json::Value &param1) override;
+    virtual std::string test_getLogHash(std::string const& param1) override;


_ prefix for parameters

My first complaint has been resolved.

gumb0 · 2018-05-04T08:34:51Z

libweb3jsonrpc/Test.cpp

+    s.appendList(_logs.size());
+    for (eth::LogEntry const& l : _logs)
+        l.streamRLP(s);
+    return toHexPrefixed(sha3(s.out()));


It would be the same result, but for consistency I'd use either toHexPrefixed or toJS in both places - here and in return toJS(dev::EmptyListSHA3);

gumb0 · 2018-05-04T08:38:52Z

libweb3jsonrpc/Test.cpp

+
+    if (m_eth.blockChain().isKnownTransaction(txHash))
+    {
+        LocalisedTransaction t = m_eth.localisedTransaction(txHash);


This could throw, too (that would mean something is not right in DB), so I would wrap all of this if in try..catch, too and rethrow as JsonRpcException(Errors::ERROR_RPC_INTERNAL_ERROR)

Yeap, I thought of that

gumb0

Please also convert all tabs to spaces in Test.h to avoid mixing them

gumb0 · 2018-05-07T16:04:25Z

libweb3jsonrpc/Test.cpp

+        {
+            txHash = h256(_txHash);
+        }
+        catch (BadHexCharacter const&)


Let's not mask all possible errors, but only catch the errors of parsing the hexstring. If at some point (maybe in the future) it start to throw other exceptions, we won't fail with confusing "invalid params" error.

gumb0 · 2018-05-07T16:05:44Z

Oh wait I made a mistake, I didn't mean to throw an enum

winsvega · 2018-05-07T16:10:06Z

you could squash the changes then and make a one finale commit

gumb0 · 2018-05-07T16:12:45Z

Didn't squash all of them, sorry

gumb0 · 2018-05-07T16:14:10Z

libweb3jsonrpc/Test.cpp

+        }
+        catch (BadHexCharacter const&)
+        {
+            throw JsonRpcException(Errors::ERROR_RPC_INVALID_PARAMS);


There was no any sense really in using BOOST_THROW_EXCEPTION for JsonRpcException which is not a boost exception

gumb0 · 2018-05-07T16:15:36Z

libweb3jsonrpc/Test.cpp

+    }
+    catch (std::exception const& ex)
+    {
+        cwarn << ex.what();


When something goes wrong we don't want to just silently return "internal error", we'd like to have a way to understand what happened, so let's output the error to the log.

you could still use boost exception for nicer error output like this:

BOOST_THROW_EXCEPTION(JsonRpcException(Errors::ERROR_RPC_INTERNAL_ERROR, _ex.what()));

No

BOOST_THROW_EXCEPTION wraps your exception with additional info like filename and line in debug build, but it can be useful only if it's caught and accessed with boost.exception facilities. Here JsonRpcException is always caught by our JSON-RPC framework, which doesn't know anything about boost.exception and just transforms it into JSON response.

The second argument to JsonRpcException constructor will also be added to JSON response, but what() string is usually for our exceptions not user-friendly at all, I don't think it will look appropriate at the client side. We should just log it in our log.

ex is not a function parameter, so no _ prefix needed

gumb0 · 2018-05-07T16:31:15Z

Ok now, but I still think returning empty list for "not found" is not the best design.

winsvega · 2018-05-07T16:50:28Z

the function should return in all it's paths. some compilers does not allow not returning the value.
is it ok if you use throw and no return after {} ?

actually in many throw cases right now client will just silently return json rpc internall error without any comment.

gumb0 · 2018-05-07T17:00:50Z

the function should return in all it's paths. some compilers does not allow not returning the value.
is it ok if you use throw and no return after {} ?

Sure it's allowed

actually in many throw cases right now client will just silently return json rpc internall error without any comment.

That should be fixed

pirapira

Looks good to me.

winsvega requested review from pirapira and gumb0 April 27, 2018 20:32

pirapira previously requested changes Apr 27, 2018

View reviewed changes

winsvega force-pushed the smallrpc3 branch from 9c54389 to 5f50619 Compare April 27, 2018 22:36

gumb0 suggested changes May 2, 2018

View reviewed changes

test_getLogHash

22c7cfb

winsvega force-pushed the smallrpc3 branch from 5f50619 to 22c7cfb Compare May 3, 2018 18:48

gumb0 reviewed May 4, 2018

View reviewed changes

gumb0 suggested changes May 4, 2018

View reviewed changes

fix issues

576402e

gumb0 reviewed May 7, 2018

View reviewed changes

Improve error handling and reporting in test_getLogHash

9dadd1b

gumb0 force-pushed the smallrpc3 branch from 9d76d82 to 9dadd1b Compare May 7, 2018 16:11

gumb0 reviewed May 7, 2018

View reviewed changes

gumb0 approved these changes May 7, 2018

View reviewed changes

pirapira approved these changes May 8, 2018

View reviewed changes

winsvega merged commit b693018 into develop May 9, 2018

axic deleted the smallrpc3 branch May 10, 2018 10:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

string test_getLogHash(txHash) #4986

string test_getLogHash(txHash) #4986

winsvega commented Apr 27, 2018 •

edited

Loading

pirapira Apr 27, 2018

codecov-io commented Apr 27, 2018 •

edited

Loading

gumb0 commented Apr 30, 2018

winsvega commented Apr 30, 2018

gumb0 commented May 2, 2018

gumb0 May 2, 2018

gumb0 May 2, 2018

gumb0 May 2, 2018

winsvega May 2, 2018

gumb0 May 3, 2018

gumb0 May 3, 2018

winsvega May 3, 2018

gumb0 May 2, 2018

winsvega May 2, 2018

gumb0 May 3, 2018

gumb0 May 2, 2018

gumb0 May 4, 2018

gumb0 May 4, 2018

winsvega May 4, 2018

gumb0 left a comment

gumb0 May 7, 2018

gumb0 commented May 7, 2018

winsvega commented May 7, 2018

gumb0 commented May 7, 2018

gumb0 May 7, 2018

gumb0 May 7, 2018

winsvega May 8, 2018

winsvega May 8, 2018 •

edited

Loading

gumb0 May 8, 2018

gumb0 May 8, 2018

gumb0 commented May 7, 2018

winsvega commented May 7, 2018

gumb0 commented May 7, 2018

pirapira left a comment

string test_getLogHash(txHash) #4986

string test_getLogHash(txHash) #4986

Conversation

winsvega commented Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Apr 27, 2018 • edited Loading

Codecov Report

gumb0 commented Apr 30, 2018

winsvega commented Apr 30, 2018

gumb0 commented May 2, 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

gumb0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented May 7, 2018

winsvega commented May 7, 2018

gumb0 commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winsvega May 8, 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 commented May 7, 2018

winsvega commented May 7, 2018

gumb0 commented May 7, 2018

pirapira left a comment

Choose a reason for hiding this comment

winsvega commented Apr 27, 2018 •

edited

Loading

codecov-io commented Apr 27, 2018 •

edited

Loading

winsvega May 8, 2018 •

edited

Loading