-
Notifications
You must be signed in to change notification settings - Fork 36.5k
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
build: comparison tool swap #6305
Conversation
While there are no other builds in the queue, I'll force-push a few times to try to trigger the NPE |
e158c04
to
a4d9f95
Compare
This should be functionally identical to what's in place now. It was built from theuni/bitcoinj@be0eef7 That commit is the same as this pruned commit in TheBlueMatt's repo: TheBlueMatt/bitcoinj@0f7b5d8 Now we'll be able to trust the line numbers in the stack traces.
Grr, 7 forced rebuilds and no errors. I'm not sure if that's a good thing or not :) |
Well it tends to happen in droves, as if it depends on the load on the Travis server. |
Going ahead and merging this for troubleshooting improvement. |
a4d9f95 build: comparison tool swap (Cory Fields)
The issue is still present. I looped it locally, until it failed (took around 30 min):
debug.log:
It's caused by BitcoindComparisonTool.java#L311: bitcoind.ping().get(); More specifically, because /**
* Sends the peer a ping message and returns a future that will be invoked when the pong is received back.
* The future provides a number which is the number of milliseconds elapsed between the ping and the pong.
* Once the pong is received the value returned by {@link com.google.bitcoin.core.Peer#getLastPingTime()} is
* updated.
* @throws ProtocolException if the peer version is too low to support measurable pings.
*/
public ListenableFuture<Long> ping() |
@dexX7 Thanks for having a look. The line numbers didn't change, so it's the same as before. I came to the same conclusion when I looked into it last time, but @mikehearn was confident that the issue couldn't be here. My guess was that it could be caused by a non-so-random PRNG causing ping lookups to race. In that case, replacing the bitcoind.ping().get() with bitcoind.ping(counter++).get() would avoid the issue I think. @mikehearn Mind having another look, now that we're sure the line#s are good? Even better, since @dexX7 can reproduce locally, maybe he'll be able to test proposed changes more easily. |
Ah, just to clarify: I'm not sure, how it's caused, and this was just a guess. But regarding the nonce lookup "collision": I gave it a try and it looks like the nonce can be anything, there always seems to be only one ping in the pending pings list (at least in these tests, for what I saw). I quickly created a stripped ping-pong only version of the tool with additional log output, and when running via:
Then the flipped created/clearing log entries further indicate the pong was indeed processed, before ping() finished:
|
@dexX7 Ah, great work. Looks like we have an answer :) |
@dexX7 Thanks! The race is fixed in this commit: Running "mvn package" inside a git master checkout of bitcoinj should create a file core/target/pull-tests.jar that contains a fresh pull tester. |
This should fix the spurious comparison tool failures. See discussion here: bitcoin#6305 The race fix was cherry-picked on top of the version we're currently using, so it should be functionally identical otherwise.
As suggested here: #6278 (comment)
This should be functionally identical to what's in place now. It was built from
theuni/bitcoinj@be0eef7
That commit is the same as this pruned commit in TheBlueMatt's repo:
TheBlueMatt/bitcoinj@0f7b5d8
Now we'll be able to trust the line numbers in the stack traces.