-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[after Constantinople] test rpc #4859
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4859 +/- ##
==========================================
+ Coverage 58.46% 60.96% +2.5%
==========================================
Files 337 350 +13
Lines 27024 28415 +1391
Branches 3164 2894 -270
==========================================
+ Hits 15799 17323 +1524
+ Misses 10161 10134 -27
+ Partials 1064 958 -106 |
libethereum/ChainParams.cpp
Outdated
rules = Network::HomesteadTest; | ||
else if (forkRules == "EIP150") | ||
rules = Network::EIP150Test; | ||
else if (forkRules == "EIP158") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer the general config it would be nice to refer to these with their semi-"official" names, such as SpuriousDragon and TangerineWhistle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still we need docs on what those names mean exactly. What changes are applied.
ethereum/retesteth#12
When are you planning to finish this? The more you add here the longer it will take to do the review process. |
eth/main.cpp
Outdated
@@ -1063,6 +1063,13 @@ int main(int argc, char** argv) | |||
web3.setIdealPeerCount(peers); | |||
web3.setPeerStretch(peerStretch); | |||
// std::shared_ptr<eth::BasicGasPricer> gasPricer = make_shared<eth::BasicGasPricer>(u256(double(ether / 1000) / etherPrice), u256(blockFees * 1000)); | |||
|
|||
// allow any gasPrice in testing mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a good candidate for a PR on its own.
I was planning to run all existing tests with RPC. If you fine with that. I could start refactoring this PR and split it into multimple if you want. |
libethereum/Client.cpp
Outdated
callQueuedFunctions(); | ||
|
||
if (shouldStop()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what these shouldStop
expressions are doing but it seems it would be better to have it as a PR on its own?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will put small changes from this PR into a separate and then rebase this on top develop.
this shouldStop() accelerating blockchain reset. Is is a tweak by Andrey. Perhaps better be moved to TestClient.cpp
libethereum/ChainParams.cpp
Outdated
@@ -281,3 +291,63 @@ bytes ChainParams::genesisBlock() const | |||
block.appendRaw(RLPEmptyList); | |||
return block.out(); | |||
} | |||
|
|||
#include <libethashseal/GenesisInfo.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't includes go to the top of the file?
libethereum/ClientTest.cpp
Outdated
reopenChain(params, WithExisting::Kill); | ||
setAuthor(params.author); //for some reason author is not being set | ||
completeSync(); // set sync state to idle for mining |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These seems to be a bugfix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. it resolve setAuthor issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to create a new PR for the bugfix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
libweb3jsonrpc/JsonHelper.cpp
Outdated
@@ -360,8 +360,8 @@ TransactionSkeleton toTransactionSkeleton(Json::Value const& _json) | |||
|
|||
if (!_json["from"].empty()) | |||
ret.from = jsToAddress(_json["from"].asString()); | |||
if (!_json["to"].empty() && _json["to"].asString() != "0x") | |||
ret.to = jsToAddress(_json["to"].asString()); | |||
if (!_json["to"].empty() && _json["to"].asString() != "0x" && !_json["to"].asString().empty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be a bugfix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. because empty() is true if there is no "to" json section.
but the string in this section could be empty or 0x (looks like no consensus on format here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a PR for this bugfix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ask/bid price PR is yet not merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is that relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to track many PRs at the same time.
ok. I have now finished fixing the tests and could concentrate on PRs here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
libweb3jsonrpc/Test.cpp
Outdated
@@ -33,6 +38,107 @@ using namespace jsonrpc; | |||
|
|||
Test::Test(eth::Client& _eth): m_eth(_eth) {} | |||
|
|||
string prepareVersionString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to a generic place I'd assume. The client should also print this on start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also if not moved elsewhere, should be placed into the anonymous namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one copied from test namespace. however libwe3jsonrpc should not have deps on test namespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this is going to be ready for review? |
libweb3jsonrpc/test.json
Outdated
@@ -1,4 +1,7 @@ | |||
[ | |||
{ "name": "test_getClientInfo", "params": [], "order": [], "returns": ""}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this different to https://github.com/ethereum/wiki/wiki/JSON-RPC#web3_clientversion ? It seems you are just returning the version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in that case. no need to implement a new method.
I am removing it from the list of test RPC methods:
ethereum/retesteth#5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove it from this PR then?
@chfast seeing how a 1 line commit takes about 2 weeks to get merged. I guess this branch stays as experimental. and to merge changes from this branch I will open series of PRs with small changes. then just rebase this branch untill no changes left. |
If you refer to #4895, maybe it's time to fix the broken tests? |
2d5f915
to
d15c234
Compare
5515bfb
to
f6cdf0c
Compare
da73f6d
to
b81c41b
Compare
d436b7a
to
73ba70b
Compare
@@ -73,7 +73,8 @@ EVMSchedule const& ChainOperationParams::scheduleForBlockNumber(u256 const& _blo | |||
|
|||
u256 ChainOperationParams::blockReward(EVMSchedule const& _schedule) const | |||
{ | |||
if (_schedule.blockRewardOverwrite) | |||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no reward for StateTests.
for BlockchainTests reward should be enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. This change does not make sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not the change. its a work around. currently there is no way to set mining reward from the config option.
Andrey said that mining reward must present and be hardcoded by the fork rules.
I guess this means no StateTests support then. But we actually dont need state tests because if you run it with test_mineBlocks anyway. there would be no difference between state and blockchain test then.
so this line could be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still not clear, what do you want us to do with this PR now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to agree on genesis scheme ethereum/tests#469
introduced changes are merged |
experimenting with rpc methods that would make test generation possible via rpc.
ethereum/retesteth#5
ethereum/retesteth#4