Fixes #2643 by restoring previous behaviour #2661

rdponticelli commented at 1:46 PM on May 17, 2013: contributor

No description provided.

BitcoinPullTester commented at 2:14 PM on May 17, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/212d4a02bb2700bbc18e26e0f9f80a5281053128 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

BitcoinPullTester commented at 8:15 PM on May 17, 2013: none

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/f48c7eb63ae43c55e6c70450757b33fdce8c58f3 for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in contrib/test-scripts)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

rdponticelli commented at 9:37 PM on May 17, 2013: contributor

@TheBlueMatt: The error on pulltester doesn't seems to be related to these changes, right?

in src/qt/bitcoingui.cpp:None in f48c7eb63a outdated

 510 | @@ -511,6 +511,8 @@ void BitcoinGUI::setNumBlocks(int count, int nTotalBlocks)
 511 |      switch (blockSource) {
 512 |          case BLOCK_SOURCE_NETWORK:
 513 |              progressBarLabel->setText(tr("Synchronizing with network..."));
 514 | +            if(clientModel->getNumConnections() == 0)

Diapolo commented at 10:52 AM on May 18, 2013:

This can't be needed here, as clientModel->getBlockSource() returns BLOCK_SOURCE_NETWORK only for getNumConnections() > 0 anyway.

rdponticelli commented at 2:17 PM on May 18, 2013:

This isn't needed for my case either, but I added it latter to try to mimic better the previous behavior, because it might trigger the same problem on other cases. Anyway, I'm trying to find the real cause.

in src/qt/bitcoingui.cpp:None in f48c7eb63a outdated

 522 | @@ -521,7 +523,7 @@ void BitcoinGUI::setNumBlocks(int count, int nTotalBlocks)
 523 |          case BLOCK_SOURCE_NONE:
 524 |              // Case: not Importing, not Reindexing and no network connection
 525 |              progressBarLabel->setText(tr("No block source available..."));
 526 | -            break;
 527 | +            return;

Diapolo commented at 10:56 AM on May 18, 2013:

This indeed restores previous behaviour (or a similar behaviour), but prevents displaying the label and statusbar for the case BLOCK_SOURCE_NONE. I intended to display e.g. No block source available... with progressbar showing x days behind for that case. IMHO we should try to find what is causing the bug instead of just preventing it :).

That said I would consider this as work-around hotfix but not as fix for the real problem that causes #2643. But perhaps you are able to debug further? Can you tell if clientModel is set for you when the bug occurs?

laanwj commented at 11:33 AM on May 18, 2013: member

I'm a bit wary of merging fixes that I don't understand, although it would be ok for 0.8.2.

Can you debug where it crashes when the premature returns are not done?

Candidates are:

clientModel->getLastBlockDate()
clientModel->getVerificationProgress()

If clientModel would not be set at all it would crash with a SIGSEGV so I don't think that could be it. For the rest the function only updates Qt controls, a very unlikely cause of crashes.

rdponticelli commented at 2:14 PM on May 18, 2013: contributor

Yes, I'm trying to debug this further. But at least if 0.8.2 have to come out quick, this hotfix might be better than nothing...

Fixes #2643 af338a7890

BitcoinPullTester commented at 5:25 PM on May 18, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/af338a7890c3ab6a8530518c52c3b833af89c3d8 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

rdponticelli commented at 6:47 PM on May 18, 2013: contributor

Meh, it doesn't fix it. It just bypasses it in that special case...

Looks like in that environment it always crashes when it tries to show the progress bar...

Now it hangs when I launch it with reindex, and it does it too in 0.8.1...

rdponticelli closed this on May 18, 2013

rdponticelli commented at 6:53 PM on May 18, 2013: contributor

Adding a -noprogressbar option would be overkill, right?

Diapolo commented at 1:14 AM on May 19, 2013: none

So are we facing a Qt bug here? What Qt version is your version of Bitcoin-Qt using?

laanwj commented at 8:14 AM on May 19, 2013: member

I really doubt it's a Qt bug. That kind of isolates it to clientModel->getVerificationProgress() (as this is new in 0.8.2) Should be pretty easy to find out. Can you try commenting it out and replacing it with a fixed number and see if it still hangs? Edit: and yes, a -noprogressbar option is out if the question :)

Diapolo commented at 11:53 AM on May 19, 2013: none

@laanwj What is that magic number here telling? progressBar->setMaximum(1000000000); @rdponticelli Did you try yet what laanwj suggested above?

rdponticelli commented at 1:56 PM on May 19, 2013: contributor

Replacing clientModel->getVerificationProgress() by a constant it works.

Might be the problem that on this setup the only block available is the genesis block?

laanwj commented at 4:13 PM on May 19, 2013: member

@Diapolo just an arbitrary number AFAIK @rdponticelli Good, that at least isolates the issue. So that function has a bug that makes it crash with only the genesis block. At least on your setup. I have not noticed this myself, when starting with an empty data directory. I'm unable to reproduce it.

rdponticelli commented at 10:42 PM on May 19, 2013: contributor

@laanwj and/or @Diapolo: Is it intended and correct that BitcoinGUI::setNumBlocks is called twice at startup?

rdponticelli deleted the branch on Jul 14, 2014

DrahtBot locked this on Sep 8, 2021