New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Continuing port of java comparison tool #8141

Merged
merged 4 commits into from Jun 13, 2016

Conversation

Projects
None yet
6 participants
@mrbandrews
Contributor

mrbandrews commented Jun 2, 2016

This continues the port of the java comparison tool to python.

Changes to the blockstore and a small change in main.cpp are in separate commits.

This includes and runs both the "inexpensive" and "barely expensive" tests. The "barely expensive" test (which consists of a few re-orgs) take a minute or two on my machine.

Java test is here:
https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 2, 2016

Member

Nice!
Concept ACK.

Member

jonasschnelli commented Jun 2, 2016

Nice!
Concept ACK.

@jonasschnelli jonasschnelli added the Tests label Jun 2, 2016

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 2, 2016

Member

Why is it necessary to accept/ignore overly long encoded varints?

Member

sipa commented Jun 2, 2016

Why is it necessary to accept/ignore overly long encoded varints?

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jun 3, 2016

Member

@sipa I think the right test would be to make sure that receiving a block with an overly long encoded varint doesn't cause the block to be marked permanently bad. The test included here is overly prescriptive by requiring that the overly long encoding be rejected; we should just loosen that and still check that the normally encoded block be accepted.

Member

sdaftuar commented Jun 3, 2016

@sipa I think the right test would be to make sure that receiving a block with an overly long encoded varint doesn't cause the block to be marked permanently bad. The test included here is overly prescriptive by requiring that the overly long encoding be rejected; we should just loosen that and still check that the normally encoded block be accepted.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 7, 2016

Member

Awesome! Concept ACK.

Member

laanwj commented Jun 7, 2016

Awesome! Concept ACK.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 10, 2016

Member

Anything holding back merging this?

Member

laanwj commented Jun 10, 2016

Anything holding back merging this?

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jun 10, 2016

Member

It'd be helpful to know what of the Java test remains to be ported.

Member

theuni commented Jun 10, 2016

It'd be helpful to know what of the Java test remains to be ported.

@mrbandrews

This comment has been minimized.

Show comment
Hide comment
@mrbandrews

mrbandrews Jun 10, 2016

Contributor

Just the "expensive test," which consists of: "Test massive reorgs (in terms of tx count)"

Contributor

mrbandrews commented Jun 10, 2016

Just the "expensive test," which consists of: "Test massive reorgs (in terms of tx count)"

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jun 10, 2016

Member

ACK

I've looked through this test and the java test, and I believe this faithfully implements all the consensus tests from the java comparison tool that we run in travis.

I think before we turn off running the java tool in travis, we should first do more careful experiments to make sure that the java tool wasn't catching things that this python test would miss, say by introducing intentional bugs to bitcoind and verifying that this test fails. I haven't done this at all yet.

Member

sdaftuar commented Jun 10, 2016

ACK

I've looked through this test and the java test, and I believe this faithfully implements all the consensus tests from the java comparison tool that we run in travis.

I think before we turn off running the java tool in travis, we should first do more careful experiments to make sure that the java tool wasn't catching things that this python test would miss, say by introducing intentional bugs to bitcoind and verifying that this test fails. I haven't done this at all yet.

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jun 10, 2016

Member

@sdaftuar agreed.

@mrbandrews To my knowledge, the "expensive" test has been broken for ages and has not been used in any automated testing.

Member

theuni commented Jun 10, 2016

@sdaftuar agreed.

@mrbandrews To my knowledge, the "expensive" test has been broken for ages and has not been used in any automated testing.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 11, 2016

Member

Concept ACK

Member

sipa commented Jun 11, 2016

Concept ACK

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 11, 2016

Member

Agree with @sdaftuar. Some tests with intentional bugs (including historical actual bugs) would be nice.

Member

sipa commented Jun 11, 2016

Agree with @sdaftuar. Some tests with intentional bugs (including historical actual bugs) would be nice.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 13, 2016

Member

It'd be helpful to know what of the Java test remains to be ported.

But not a requirement for merging this, as this doesn't disable the java tester. Improving the python tests is always good, independent from anything else.

Although the deadline for that is approaching too.

Member

laanwj commented Jun 13, 2016

It'd be helpful to know what of the Java test remains to be ported.

But not a requirement for merging this, as this doesn't disable the java tester. Improving the python tests is always good, independent from anything else.

Although the deadline for that is approaching too.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 13, 2016

Member

utACK ff2dcf2

Member

laanwj commented Jun 13, 2016

utACK ff2dcf2

@laanwj laanwj merged commit ff2dcf2 into bitcoin:master Jun 13, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 13, 2016

Merge #8141: Continuing port of java comparison tool
ff2dcf2 Tests: Edit bloated varint test and add option for 'barely expensive' tests (mrbandrews)
12c5a16 Catch exceptions from non-canonical encoding and print only to log (mrbandrews)
291f8aa Continuing port of java comptool (mrbandrews)
8c9e681 Tests: Rework blockstore to avoid re-serialization. (mrbandrews)

@mrbandrews mrbandrews deleted the mrbandrews:ba-comptool branch Jun 14, 2016

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #8141: Continuing port of java comparison tool
ff2dcf2 Tests: Edit bloated varint test and add option for 'barely expensive' tests (mrbandrews)
12c5a16 Catch exceptions from non-canonical encoding and print only to log (mrbandrews)
291f8aa Continuing port of java comptool (mrbandrews)
8c9e681 Tests: Rework blockstore to avoid re-serialization. (mrbandrews)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #8141: Continuing port of java comparison tool
ff2dcf2 Tests: Edit bloated varint test and add option for 'barely expensive' tests (mrbandrews)
12c5a16 Catch exceptions from non-canonical encoding and print only to log (mrbandrews)
291f8aa Continuing port of java comptool (mrbandrews)
8c9e681 Tests: Rework blockstore to avoid re-serialization. (mrbandrews)

@daira daira referenced this pull request Dec 15, 2017

Merged

Build system improvements #2786

codablock added a commit to codablock/dash that referenced this pull request Dec 22, 2017

Merge #8141: Continuing port of java comparison tool
ff2dcf2 Tests: Edit bloated varint test and add option for 'barely expensive' tests (mrbandrews)
12c5a16 Catch exceptions from non-canonical encoding and print only to log (mrbandrews)
291f8aa Continuing port of java comptool (mrbandrews)
8c9e681 Tests: Rework blockstore to avoid re-serialization. (mrbandrews)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment