fix setting author in setChainParams for --test mode #4922

winsvega · 2018-03-29T12:40:36Z

when setChainParams was called. coinbase author was not reset correctly

gumb0 · 2018-03-29T12:44:26Z

Interesting, could it fix RPC tests?

gumb0 · 2018-03-29T12:44:37Z

libethereum/ClientTest.cpp

@@ -60,8 +60,9 @@ void ClientTest::setChainParams(string const& _genesis)
        if (params.sealEngineName != "NoProof")
            BOOST_THROW_EXCEPTION(ChainParamsNotNoProof() << errinfo_comment("Provided configuration is not well formatted."));

+        setAuthor(params.author);  // Must be called before reopenChain !!!


Why must it?

see at reopenChain it replaces the author from m_preSeal.
so setAuthor would set an author for the m_preSeal

yes it should fix author for rpc tests. but we don't run that script anymore.

It looks like this reopenChain is called only from here.
(also from Client::killChain() which looks to be not used and should be removed)

If so, set the author from params.author inside reopenChain instead of "backup and restore author" logic there

gumb0 · 2018-03-29T12:45:02Z

libethereum/ClientTest.cpp

        reopenChain(params, WithExisting::Kill);
-        setAuthor(params.author); //for some reason author is not being set
+        completeSync();  // set sync state to idle for mining


Why this? What happens without it?

without this. sync state is not being set to idle (sometimes) and test_mineblock does not work

This function doesn't do anything related to the sync, I don't think it's the right place for this... Can you maybe move it up the stack, to where setChainParams() is called? (comleteSync() is a public method)

completeSync does not do anything related to Sync ?
anyway without this function the sync state is not idle and new blocks not being mined in test mode.

I meant not completeSync but setChainParams

codecov-io · 2018-03-29T15:33:52Z

Codecov Report

Merging #4922 into develop will decrease coverage by 1.96%.
The diff coverage is 92%.

@@            Coverage Diff             @@
##           develop   #4922      +/-   ##
==========================================
- Coverage    60.56%   58.6%   -1.97%     
==========================================
  Files          349     348       -1     
  Lines        28218   23715    -4503     
  Branches      2895    2720     -175     
==========================================
- Hits         17089   13897    -3192     
+ Misses       10160    8905    -1255     
+ Partials       969     913      -56

winsvega · 2018-03-29T16:35:22Z

libethereum/Client.cpp

        h->reset();
+        h->completeSync();  // set sync state to idle for mining


@gumb0 this looks like kostil` around m_state variable that must be set to Idle. but I am afraid that changing it in internal funcs might break the client.

You move it down the stack instead of up the stack, as I asked. This is probably even worse.

Where is setChainParams called for your case? Can you put completeSync call there?

setChainParams is test client method. so the best place is to keep it where it was. or make it a separate method of the test client. (which is unnecessary)

There is already ClientTest::completeSync(), you could call it from from RPC method implementation Test::test_setChainParams() (add asClientTest(m_eth).completeSync(); there), similar to what happens in Test::test_rewindToBlock(). It still would be ugly, but less so.

Starting eth --test doesn't enable sync by itself, can you find out at which moment does it become enabled? (the stack where BlockChainSync::m_state changes)

the moment where it changes is strange. it works for 1 time and then it set to notSynced after blockchain reset.

chfast · 2018-03-30T21:34:18Z

Why nobody is adding unit tests along with the fixes?

gumb0 · 2018-04-03T10:06:35Z

Yeah, maybe try to to create a unit test for Client::reopenChain
We don't have any for test mode-specific functions though, I think

chfast · 2018-04-03T15:48:07Z

Can you also squash commits before merging?

gumb0 · 2018-04-03T15:51:13Z

test/unittests/libethereum/ClientTest.cpp

+using namespace dev::p2p;
+namespace fs = boost::filesystem;
+
+std::unique_ptr<dev::WebThreeDirect> g_web3;


Why do you make it global and not the member of the fixture class?

how to access fixture from the test case?

test cases are methods of the fixture.

^ this or more likely methods of a subclass of a fixture class, so you would access web3 member just like web3 (it should be public or protected)

gumb0 · 2018-04-03T15:51:50Z

Please convert all tabs to spaces in libweb3jsonrpc/Test.cpp

winsvega · 2018-04-03T16:59:21Z

huh set author still does not work on mac

chfast · 2018-04-03T18:47:40Z

test/unittests/libethereum/ClientTest.cpp

+    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 ClientTest.cpp


Don't add this. There is no information added with this comment.

convert tabs to spaces in Test.cpp

winsvega · 2018-04-04T11:55:50Z

@chfast the test was failing on macOS when the web3 was declared as global variable in cpp.
when I moved it into the class it seems to work.

chfast · 2018-04-04T12:00:16Z

RandomCodeTests/rndStateTest (Failed)

chfast

Code looks good, but check out the failing tests.

winsvega · 2018-04-04T12:31:06Z

random test failure is a bug. sometimes gasLimit is wrong and transaction is not being executed. thus post state check errors out. better fix in a separate PR

winsvega requested a review from axic March 29, 2018 12:40

winsvega mentioned this pull request Mar 29, 2018

[after Constantinople] test rpc #4859

Closed

gumb0 reviewed Mar 29, 2018

View reviewed changes

winsvega commented Mar 29, 2018

View reviewed changes

gumb0 reviewed Apr 3, 2018

View reviewed changes

chfast reviewed Apr 3, 2018

View reviewed changes

winsvega added 3 commits April 4, 2018 13:58

fix setting author in reopenChain

7567d52

set syncState to idle in test_setChainParams

6912850

convert tabs to spaces in Test.cpp

add unittest for setAuthor in setChainParams

ef71136

winsvega force-pushed the smalltrpc branch from 364f98e to ef71136 Compare April 4, 2018 11:00

chfast approved these changes Apr 4, 2018

View reviewed changes

gumb0 approved these changes Apr 4, 2018

View reviewed changes

winsvega merged commit 740da50 into develop Apr 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix setting author in setChainParams for --test mode #4922

fix setting author in setChainParams for --test mode #4922

winsvega commented Mar 29, 2018

gumb0 commented Mar 29, 2018

gumb0 Mar 29, 2018

winsvega Mar 29, 2018

winsvega Mar 29, 2018

gumb0 Mar 29, 2018 •

edited

Loading

gumb0 Mar 29, 2018

winsvega Mar 29, 2018 •

edited

Loading

gumb0 Mar 29, 2018

winsvega Mar 29, 2018 •

edited

Loading

gumb0 Apr 3, 2018

codecov-io commented Mar 29, 2018 •

edited

Loading

winsvega Mar 29, 2018 •

edited

Loading

gumb0 Apr 3, 2018

gumb0 Apr 3, 2018

winsvega Apr 3, 2018

gumb0 Apr 3, 2018

winsvega Apr 3, 2018

chfast commented Mar 30, 2018

gumb0 commented Apr 3, 2018

chfast commented Apr 3, 2018

gumb0 Apr 3, 2018

winsvega Apr 3, 2018

chfast Apr 3, 2018

gumb0 Apr 4, 2018 •

edited

Loading

gumb0 commented Apr 3, 2018

winsvega commented Apr 3, 2018

chfast Apr 3, 2018

winsvega commented Apr 4, 2018

chfast commented Apr 4, 2018

chfast left a comment

winsvega commented Apr 4, 2018

		h->reset();
		h->completeSync(); // set sync state to idle for mining

fix setting author in setChainParams for --test mode #4922

fix setting author in setChainParams for --test mode #4922

Conversation

winsvega commented Mar 29, 2018

gumb0 commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winsvega Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winsvega Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 29, 2018 • edited Loading

Codecov Report

winsvega Mar 29, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Mar 30, 2018

gumb0 commented Apr 3, 2018

chfast commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

gumb0 commented Apr 3, 2018

winsvega commented Apr 3, 2018

Choose a reason for hiding this comment

winsvega commented Apr 4, 2018

chfast commented Apr 4, 2018

chfast left a comment

Choose a reason for hiding this comment

winsvega commented Apr 4, 2018

gumb0 Mar 29, 2018 •

edited

Loading

winsvega Mar 29, 2018 •

edited

Loading

winsvega Mar 29, 2018 •

edited

Loading

codecov-io commented Mar 29, 2018 •

edited

Loading

winsvega Mar 29, 2018 •

edited

Loading

gumb0 Apr 4, 2018 •

edited

Loading