Build tool failures due to use of un-maintained script #6119

@sdaftuar Noticed trouble with his builds, and it looks like its due to an unmaintained file in bitcoinj (which is used in pull-testing)

For accurate build results, I think this should be either updated (the tool*) or deprecated until it is maintained. @theuni do you have any preference ? I ask since it seems you had your hands in the travis yml last, I can look at the bitcoinj side to see what it would take, seems it might just need an update to handle 0.10?

failing build: https://travis-ci.org/bitcoin/bitcoin/jobs/61800593

passing build (throws exceptions all over - Ctrl-F "Exception" ):https://travis-ci.org/bitcoin/bitcoin/jobs/61800595

*tool hasn't seen a commit since december, and likely needs 0.10 updates: https://github.com/bitcoinj/bitcoinj/blob/master/core/src/test/java/org/bitcoinj/core/BitcoindComparisonTool.java

edit: link to erroneous build command https://github.com/bitcoin/bitcoin/blob/master/Makefile.am#L195-L197

@faizkhan00 This tool has a long history. It's not pulled from bitcoinj, but rather from @TheBlueMatt's forked version. See depends/packages/native_comparisontool.mk.

We rely on it for behavioral tests and it can't be deprecated until all tests have been recreated. See the recent python test framework from @sdaftuar which may be able to achieve that once it's filled out more.

Thanks @theuni, this is exactly what I was looking for before. I'm still trying to find out, where that comparisontool is built from (makefile-wise)... I didn't notice any references to TheBlueMatt/bitcoinj, but perhaps I'm not looking in the right place.

(As an aside, it also looks like native_comparisontool.mk is pointing to an empty ref (404 for me :/): https://github.com/TheBlueMatt/test-scripts/raw/38b490a2599d422b12d5ce8f165792f63fd8f54f , not sure if this is aggravating the buildbot issues or not, though.)

More to the point, if the builds are breaking, where would the fixes go, and are they valuable to update? (versus pushing that off or expanding @sdaftuar's testing framework, instead -* I think having accurate travis builds would provide lots of value)

@faizkhan00 Travis uses that process for each build. It contains no state (other than a download cache) so if the url is bad, builds will eventually fail because they can't fetch it. The full url is download_path + file_name: https://github.com/TheBlueMatt/test-scripts/raw/38b490a2599d422b12d5ce8f165792f63fd8f54f/pull-tests-0f7b5d8.jar

As for fixing broken builds, the current tool is built from here: https://github.com/TheBlueMatt/bitcoinj/commits/0f7b5d89b5f70c0803d36e690b702b5050771381 . That commit doesn't appear in any public branches, it looks like it's been rebased into https://github.com/TheBlueMatt/bitcoinj/tree/blocktester. Notice that the package_version in native_comparisontool.mk points to the commit that the jar is built from.

I suggested at one point that we build the jar from source rather than pulling down some random blob, but I was in the minority and I didn't push it. I'd still like to see it work that way, but at this point, it makes more sense to spend the effort get the python tool up to feature parity.

If you'd like to take a stab at fixing, I suggest you checkout the code at 0f7b5d89b5f70c0803d36e690b702b5050771381, track down the issue, build a jar, then submit a PR to bump the version we pull. But again, I'm sure @sdaftuar could come up with a list of ways you could help out with the python framework instead.

Travis uses that process for each build. It contains no state [...] The full url is download_path + file_name: ...

Ah! I see how it works now. That was a strange moment, thanks for clarifying that.

I also see what you mean about building from source, it seems the jar in TheBlueMatt/test-scripts was compiled in a native environment and then distributed using Github, which doesn't feel very good from a deployment perspective.

It looks like you're spot-on regarding hash-branch correlations too, I see the tip of TheBlueMatt/blocktester is the same hash as whats in the tip of TheBlueMatt/test-scripts (the jar, at least); and I think my preference is to try to load the BitcoindComparisonTool tests into the python framework as well.

Since it likely means debugging the comparison tool, I'll probably start there, and seeing as how its probably not a light fix I'd like to ask @sdaftuar: What are your thoughts about expanding the python framework a bit to replace the current tooling? It looks like I'll be debugging things in parallel, so I'm curious if you see any benefit from adding to your code.

@faizkhan00 I think the next step for the python work is to start recreating the tests from the existing bitcoinj comparison tool in a new python test. I think there are two main hurdles for this work, one is to just carefully understand the existing tests, and the other is to implement whatever additional python infrastructure might be needed to make for well-written tests (ie that are easy to read, write, update, understand, etc). Not sure yet exactly what those infrastructure changes will look like, but I'm guessing we'll need to refine the BlockStore object (in blockstore.py) and maybe the TestInstance in comptool.py, and likely need to add some additional code to support signing transactions.

I guess a third hurdle is that I think there may be bugs in the existing bitcoinj tests. For instance one issue I've been meaning to track down but haven't yet is this apparent bug mentioned in an old PR, #2884.

Anyway I am planning to start working on a python reimplementation, but I expect it to take a while before all the tests are rewritten and before everyone would be comfortable cutting over from the existing tests to a new one. So if you do spend time debugging/patching the bitcoinj version, which actually also needs to be patched for #5966, that should be generally helpful I think.

Also if you're interested in diving in and writing python tests please feel free, though unfortunately I haven't written up documentation yet to explain how the code is structured and how to write new tests (it's on my list of things to do once the interface feels more settled, probably after the next round of changes) -- for now I'd suggest looking at the tests added in #5981 for examples to follow and reading the comments in the code. And feel free to ping me with any specific questions in the meantime.

So if you do spend time debugging/patching the bitcoinj version, which actually also needs to be patched for #5966, that should be generally helpful I think.

So this is where I'll start this week, hopefully the problems aren't nearly as bad as they look. If along the way the documentation for the python code ends up getting written, I might switchover, but I think in either case it will be good to figure out what the Java tool does so we can port that code to Python eventually. I'll update here letting everyone know how it goes when I get a chance to look at it this week.

So, I was able to get Bluematt's tool running against -master, seeing lots of failures; I'll start running them down but its certainly not the 'quick fix' i was expecting... my hope is after all is said and done i'll know what the tests are for , and it will make for an easy port onto @sdaftuar's framework. Interestingly, the commit on test-scripts/master (from Dec) just exits after the 87th block test... Not sure why that is yet, though.

Full failure listing:

org.bitcoinj.core.VerificationException: Block had too many Signature Operations
org.bitcoinj.core.VerificationException: Block larger than MAX_BLOCK_SIZE
org.bitcoinj.core.VerificationException$CoinbaseScriptSizeOutOfRange: Coinbase script size out of range
org.bitcoinj.core.VerificationException$CoinbaseScriptSizeOutOfRange: Coinbase script size out of range
org.bitcoinj.core.VerificationException: Block had too many Signature Operations
org.bitcoinj.core.VerificationException: Block had too many Signature Operations
org.bitcoinj.core.VerificationException: Block had too many Signature Operations
org.bitcoinj.core.VerificationException: First tx is not coinbase
org.bitcoinj.core.VerificationException: Block had no transactions
org.bitcoinj.core.VerificationException: Hash is higher than target: 8c271620ac810a0c7f4d82d91d04a34e2dd88158b77e865090433ee42b14c090 vs 7fffff0000000000000000000000000000000000000000000000000000000000
org.bitcoinj.core.VerificationException: Block too far in future: 1431574959 vs 1431571359
org.bitcoinj.core.VerificationException: Merkle hashes do not match: 0780c7e4e4499ecd763784f516db0bcb5fb10b0b696566d63fb496c73c117241 vs 5a3cef1e627ddb1a329f2838e7520876c7c57721d77930d54f7fde14890cd8ef
org.bitcoinj.core.VerificationException: TX 2 is coinbase when it should not be.
org.bitcoinj.core.VerificationException: Block had too many Signature Operations
org.bitcoinj.core.VerificationException: Block had too many Signature Operations

What do you mean by "running against -master"? Travis runs the tool runs against every bitcoin pull request on any branch.

Do you mean that you've rebased his changes into upstream bitcoinj master?

What do you mean by "running against -master"? 

Thats true, but, running the jar on the tip of TheBlueMatt/test-scripts means we have the latest code (which differs against the hashes in native-comparisontool.mk) and also offers a quick path to start making changes and building the tool from source (which might entail rebasing into upstream bitcoinj master actually- since I've been seeing errors while building the jarfile), while also staying on top of bitcoin/bitcoin changes in case new features break more tests.

My thought right now is, by getting what's on TheBlueMatt/test-scripts/master up-to-date with bitcoin/bitcoin/master, then we'll at least have a starting point to start seeing which tests can be ported over while still serving up travis builds.

Another thing, I'm not entirely sure if those Exceptions are part of the suite or actual regressions yet, because my thought is those would cause many more build failures across the pull-tester, but they don't seem to cause much of a problem as they're caught when thrown in the pull-tester.

I believe the Travis builds are passing now, and I think the exceptions above are erroneous (supposed to be thrown) so closing this