Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Remove the redundant signature recovery and hash #4951

Merged

Conversation

wanderingbort
Copy link
Contributor

Before applying blocks, we add them to fork-db. Only if fork-db signals better header-validated fork do we bother to apply the block. As part of header-validation we validate the signature therefore, the additional check in ::apply_block is wasteful.

@wanderingbort wanderingbort requested a review from arhag July 31, 2018 22:09
arhag
arhag previously approved these changes Aug 1, 2018
@arhag arhag dismissed their stale review August 1, 2018 17:45

Actually, we need at least a check to make sure the pending block header is identical to the signed block header, otherwise a producer could falsify the action merkle root, for example.

Copy link
Contributor

@heifner heifner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to update tests:

/data/job/unittests/forked_tests.cpp(131): �[4;31;49mfatal error: in "forked_tests/fork_with_bad_block": exception fc::exception expected but not raised

Failure occurred in a following context:

    Testing Fork: 0�[0;39;49m

�[33m0 exception: unspecified

rethrow

    {}

    thread-0  forked_tests.cpp:142 test_method�[0m

/data/job/unittests/main.cpp(15): �[4;31;49mfatal error: in "forked_tests/fork_with_bad_block": Caught Unexpected Exception�[0;39;49m

@wanderingbort
Copy link
Contributor Author

wanderingbort commented Aug 1, 2018

@heifner I think those were legit regressions that should now be fixed (without changing the tests)

hopefully resolved with d2699c0

@heifner
Copy link
Contributor

heifner commented Aug 1, 2018

@wanderingbort Tests doing their jobs, nice.

@heifner heifner merged commit 1bdd4be into EOSIO:release/1.1.x Aug 1, 2018
@wanderingbort wanderingbort deleted the feature/fix-double-sig-check branch August 1, 2018 20:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants