return importBlockFailed ImportResult in test_importRawBlock #5868

winsvega · 2019-12-10T00:26:18Z

Export exception information on test_improtRawBlock

gumb0

The related unit tests crash now. Maybe the exception doesn't really contain errinfo_importResult?

Also please add an entry to CHANGELOG.md

libweb3jsonrpc/Test.cpp

halfalicious · 2019-12-10T17:53:05Z

Unit test failures:


760/769 Test #761: JsonRpcSuite/test_importRawBlockFailsForInvalidHeader .................................................................***Failed    0.22 sec
Running tests using path: "/home/builder/project/test/jsontests"
Running 1 test case...
Test Case "test_importRawBlockFailsForInvalidHeader": 
unknown location(0): fatal error: in "JsonRpcSuite/test_importRawBlockFailsForInvalidHeader": memory access violation at address: 0x00000000: no mapping at fault address
/home/builder/project/test/unittests/libweb3jsonrpc/jsonrpc.cpp(804): last checkpoint

*** 1 failure is detected (6 failures are expected) in the test module "Master Test Suite"


761/769 Test #762: JsonRpcSuite/test_importRawBlockFailsForInvalidBlockBody ..............................................................***Failed    0.22 sec
Running tests using path: "/home/builder/project/test/jsontests"
Running 1 test case...
Test Case "test_importRawBlockFailsForInvalidBlockBody": 
unknown location(0): fatal error: in "JsonRpcSuite/test_importRawBlockFailsForInvalidBlockBody": memory access violation at address: 0x00000000: no mapping at fault address
/home/builder/project/test/unittests/libweb3jsonrpc/jsonrpc.cpp(828): last checkpoint

*** 1 failure is detected (6 failures are expected) in the test module "Master Test Suite"


762/769 Test #760: JsonRpcSuite/test_importRawBlock ......................................................................................   Passed    0.32 sec
        Start 765: ClientBase.1
        Start 766: ClientBase.2
        Start 767: ClientBase.3
        Start 768: ClientBase.4
        Start 769: ClientBase.5
763/769 Test #768: ClientBase.4 ..........................................................................................................   Passed    0.10 sec
764/769 Test #769: ClientBase.5 ..........................................................................................................   Passed    0.10 sec
765/769 Test #767: ClientBase.3 ..........................................................................................................   Passed    0.10 sec
766/769 Test #766: ClientBase.2 ..........................................................................................................   Passed    0.11 sec
767/769 Test #765: ClientBase.1 ..........................................................................................................   Passed    0.21 sec
768/769 Test #735: ClientTests/Personal ..................................................................................................   Passed    8.36 sec
769/769 Test #313: LegacyTests/Constantinople/BlockchainTests/ValidBlocks/bcExploitTest ..................................................   Passed  145.04 sec

99% tests passed, 2 tests failed out of 769

Total Test time (real) = 180.73 sec

The following tests FAILED:
	761 - JsonRpcSuite/test_importRawBlockFailsForInvalidHeader (Failed)
	762 - JsonRpcSuite/test_importRawBlockFailsForInvalidBlockBody (Failed)
Errors while running CTest

winsvega · 2019-12-10T18:25:55Z

The tests are failing because of this line

(fixed code ClientTest.cpp :: 149)

// check that it was really imported and not rejected as invalid
    if (!bc().isKnown(blockHash))
        BOOST_THROW_EXCEPTION(ImportBlockFailed() << errinfo_importResult(ImportResult::BadChain));

the block was successfully imported into blockqueue but then dropped from the chain.
this is the case I was talking about. in this particular tests there should be an error code Invalid Difficulty.

gumb0 · 2019-12-10T18:40:51Z

Your exception handler should better check boost::get_error_info<dev::eth::errinfo_importResult>(e) != nullptr before dereferencing it

gumb0 · 2019-12-10T18:41:57Z

As for Invalid Difficulty - I'm not sure, it seems to me from the code, that it should be checked on the very first stage during adding to Block Queue.

gumb0 · 2019-12-10T18:44:16Z

But as a general comment I'm not sure that the tests should require the exact error message to be returned from the client.

The way to check that invalid difficulty is rejected is just to construct the block that has all other fields right except for the difficulty, and check whether the client rejects it. Without relying on error messages, error messages are not part of the consensus.

winsvega · 2019-12-10T20:38:14Z

"The way to check that invalid difficulty is rejected is just to construct the block that has all other fields right except for the difficulty, and check whether the client rejects it. Without relying on error messages, error messages are not part of the consensus."

that's how the tests do. But when I generate the test I must verify the exception to be safe that the exact exception is checked. I am not talking about exact error message. but the exception upon block import.

gumb0 · 2019-12-11T11:32:24Z

that's how the tests do.

Well the tests contain things like

                "expectExceptionALL" : "InvalidGasLimit",

So it looks they try to check the exact exception.

gumb0 · 2019-12-11T11:33:21Z

@winsvega Could you provide me the chain config and block data with invalid difficulty? I'll try to check what's happening with it

winsvega · 2019-12-11T15:31:48Z

try running
./retesteth -t BlockchainTests/InvalidBlocks/bcInvalidHeaderTest -- --clients "aleth" --verbosity 6 --singletest wrongDifficulty

   "expectExceptionALL" : "InvalidGasLimit",

this is old format. I am customizing this so that clients could report any kind of message.

gumb0 · 2019-12-11T16:22:31Z

Yeah this block doesn't pass the first checks in BlockQueue, but BlockQueue::import returns ImportResult::Malformed for all such checks.

gumb0 · 2019-12-11T16:38:00Z

this is old format. I am customizing this so that clients could report any kind of message.

I still don't get, how do you change it? If the test won't contain different errors, then what's the point of checking the error during generation? (the result test is the same anyway?)

libweb3jsonrpc/Test.cpp

winsvega · 2019-12-11T17:32:19Z

The generated test is the same but I need a confirmation from the client that a specific exception was hit to be sure (upon generation)

gumb0 · 2019-12-11T17:51:16Z

To be sure of what?
You create this invalid header yourself from the valid one, then you create a test saying "this block should be rejected", what's the purpose of these additional checks, to verify that your own invalid header generation code works correctly?

So the test fillers still contain different exception names? Why not get rid of them there, too, if you remove them from filled test?

winsvega · 2019-12-11T19:26:00Z

To be sure that a client that generates the test reject the block with required exception. And avoid the case where client reject the block but for a different reason. And yes to check that a test created correctly. (Typos/code issues)

gumb0 · 2019-12-11T20:04:26Z

why do you need that?

winsvega · 2019-12-11T20:06:34Z

This is my standard of quality for the tests

gumb0 · 2019-12-12T11:08:34Z

Ok I have some ideas how to return the exact error

winsvega · 2019-12-12T11:19:10Z

Btw geth return exact exception

gumb0 · 2019-12-13T16:01:25Z

In #5871 I'm adding a nested exception with the exact header validation error to ImportBlockFailed thrown from ClientTest::importRawBlock, see exact spec here https://github.com/ethereum/aleth/pull/5871/files#diff-4c08ffb035d5be4408f58a82ddff192bR35-R39

When it's merged, you would need to

rebase this
Add function to Test.cpp to convert exceptions to strings like this

string exceptionToErrorMessage(boost::exception_ptr _e)
{
    string ret;
    try
    {
        boost::rethrow_exception(_e);
    }
    catch (InvalidDifficulty const&)
    {
        ret = "Invalid Difficulty.";
    }
    catch (...)
    {
        ret = "Unknown error.";
    }
    return ret;
}

Handle ImportBlockFailed like this

    catch (ImportBlockFailed const& e)
    {
        // ...
        auto importResult = boost::get_error_info<errinfo_importResult>(e);
        std::string detailedError;
        if (importResult && *importResult == ImportResult::Malformed)
        {
            auto nested = boost::get_error_info<errinfo_nestedException>(e);
            if (nested)
                detailedError = exceptionToErrorMessage(*nested);
        }
        // ...
    }

winsvega · 2019-12-15T16:17:37Z

rebased on top of #5871
it works! thanks!

chfast · 2019-12-16T08:46:18Z

Will you be able to finish it today so I can include it in the 1.8.0 release?

winsvega · 2019-12-16T10:20:11Z

No. I checked yesterday this hack doesn't detect some of the block validation exceptions. Such as Invalid Timestamp, Invalid Number

gumb0 · 2019-12-16T12:38:23Z

Please rebase on master

codecov-io · 2019-12-19T09:31:33Z

Codecov Report

Merging #5868 into master will decrease coverage by 0.05%.
The diff coverage is 19.29%.

@@            Coverage Diff             @@
##           master    #5868      +/-   ##
==========================================
- Coverage   64.04%   63.98%   -0.06%     
==========================================
  Files         365      365              
  Lines       31037    31093      +56     
  Branches     3440     3441       +1     
==========================================
+ Hits        19877    19895      +18     
- Misses       9929     9968      +39     
+ Partials     1231     1230       -1

libethereum/ClientTest.cpp

libweb3jsonrpc/Test.cpp

winsvega requested a review from gumb0 December 10, 2019 00:26

winsvega changed the title ~~return importBlocFailed ImportResult in test_importRawBlock~~ return importBlockFailed ImportResult in test_importRawBlock Dec 10, 2019

winsvega force-pushed the importres branch from 1c92880 to e0ddc1e Compare December 10, 2019 00:27

gumb0 suggested changes Dec 10, 2019

View reviewed changes

libweb3jsonrpc/Test.cpp Outdated Show resolved Hide resolved

gumb0 reviewed Dec 11, 2019

View reviewed changes

libweb3jsonrpc/Test.cpp Outdated Show resolved Hide resolved

winsvega force-pushed the importres branch from e0ddc1e to ff4e23c Compare December 15, 2019 16:16

winsvega mentioned this pull request Dec 15, 2019

new format of expected block exceptions in InvalidBlockChain tests (WIP) ethereum/tests#670

Closed

return importBlockFailed ImportResult in test_importRawBlock

139a4fa

winsvega force-pushed the importres branch from ff4e23c to 89086d0 Compare December 19, 2019 09:31

winsvega force-pushed the importres branch from 89086d0 to 6a89e72 Compare December 19, 2019 11:05

gumb0 suggested changes Dec 19, 2019

View reviewed changes

libethereum/ClientTest.cpp Outdated Show resolved Hide resolved

libweb3jsonrpc/Test.cpp Outdated Show resolved Hide resolved

rebase on top of nested exception pr

2b9d4b5

winsvega force-pushed the importres branch from 6a89e72 to 2b9d4b5 Compare December 20, 2019 11:47

update tests repo

4c8a35a

winsvega requested a review from gumb0 December 20, 2019 13:31

Add changelog

f2869fc

gumb0 approved these changes Dec 20, 2019

View reviewed changes

winsvega merged commit 31db6db into master Dec 23, 2019

winsvega mentioned this pull request Jan 22, 2020

update blockchain test fillers ethereum/tests#672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

return importBlockFailed ImportResult in test_importRawBlock #5868

return importBlockFailed ImportResult in test_importRawBlock #5868

winsvega commented Dec 10, 2019 •

edited

Loading

gumb0 left a comment

halfalicious commented Dec 10, 2019

winsvega commented Dec 10, 2019 •

edited

Loading

gumb0 commented Dec 10, 2019

gumb0 commented Dec 10, 2019

gumb0 commented Dec 10, 2019

winsvega commented Dec 10, 2019 •

edited

Loading

gumb0 commented Dec 11, 2019

gumb0 commented Dec 11, 2019

winsvega commented Dec 11, 2019

gumb0 commented Dec 11, 2019

gumb0 commented Dec 11, 2019

winsvega commented Dec 11, 2019 •

edited

Loading

gumb0 commented Dec 11, 2019

winsvega commented Dec 11, 2019 •

edited

Loading

gumb0 commented Dec 11, 2019

winsvega commented Dec 11, 2019 •

edited

Loading

gumb0 commented Dec 12, 2019

winsvega commented Dec 12, 2019

gumb0 commented Dec 13, 2019

winsvega commented Dec 15, 2019

chfast commented Dec 16, 2019

winsvega commented Dec 16, 2019

gumb0 commented Dec 16, 2019

codecov-io commented Dec 19, 2019 •

edited

Loading

return importBlockFailed ImportResult in test_importRawBlock #5868

return importBlockFailed ImportResult in test_importRawBlock #5868

Conversation

winsvega commented Dec 10, 2019 • edited Loading

gumb0 left a comment

Choose a reason for hiding this comment

halfalicious commented Dec 10, 2019

winsvega commented Dec 10, 2019 • edited Loading

gumb0 commented Dec 10, 2019

gumb0 commented Dec 10, 2019

gumb0 commented Dec 10, 2019

winsvega commented Dec 10, 2019 • edited Loading

gumb0 commented Dec 11, 2019

gumb0 commented Dec 11, 2019

winsvega commented Dec 11, 2019

gumb0 commented Dec 11, 2019

gumb0 commented Dec 11, 2019

winsvega commented Dec 11, 2019 • edited Loading

gumb0 commented Dec 11, 2019

winsvega commented Dec 11, 2019 • edited Loading

gumb0 commented Dec 11, 2019

winsvega commented Dec 11, 2019 • edited Loading

gumb0 commented Dec 12, 2019

winsvega commented Dec 12, 2019

gumb0 commented Dec 13, 2019

winsvega commented Dec 15, 2019

chfast commented Dec 16, 2019

winsvega commented Dec 16, 2019

gumb0 commented Dec 16, 2019

codecov-io commented Dec 19, 2019 • edited Loading

Codecov Report

winsvega commented Dec 10, 2019 •

edited

Loading

winsvega commented Dec 10, 2019 •

edited

Loading

winsvega commented Dec 10, 2019 •

edited

Loading

winsvega commented Dec 11, 2019 •

edited

Loading

winsvega commented Dec 11, 2019 •

edited

Loading

winsvega commented Dec 11, 2019 •

edited

Loading

codecov-io commented Dec 19, 2019 •

edited

Loading