Move external block import to separate thread #1880

sipa commented at 1:43 PM on September 28, 2012: member

Originally written for the ultraprune+leveldb demo at the London 2012 conference.

Diapolo commented at 2:49 PM on September 28, 2012: none

Seems to be a valuable addition, but as we have IMPLEMENT_RANDOMIZE_STACK() I would like to see that used or want to question if we need it anyway.

in src/main.cpp:None in 8f302d9a10 outdated

2682 | @@ -2648,9 +2683,10 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv)
2683 |              if (fDebug)
2684 |                  printf("  got inventory: %s  %s\n", inv.ToString().c_str(), fAlreadyHave ? "have" : "new");
2685 |  
2686 | -            if (!fAlreadyHave)
2687 | -                pfrom->AskFor(inv);
2688 | -            else if (inv.type == MSG_BLOCK && mapOrphanBlocks.count(inv.hash)) {
2689 | +            if (!fAlreadyHave) {

Diapolo commented at 2:51 PM on September 28, 2012:

Couln't this be a single if with an &&?

sipa commented at 2:54 PM on September 28, 2012:

AFAICS that way it would affect the following "else" branches.

Diapolo commented at 3:13 PM on September 28, 2012:

I'm not sure if I understand why, but it's not worth the discussion in the end.

laanwj commented at 3:49 AM on October 1, 2012:

@diapolo, I do think it's important that you understand: let's say you have the following code:

if (a) {
    if(b) {
       doActionAB();
    }
} else if (c) {
    doActionC();
}

If you make a truth table for that, you get:

a b c
0 0 0    N/A
0 0 1    doActionC
0 1 0    N/A
0 1 1    doActionC
1 0 0    N/A
1 0 1    N/A
1 1 0    doActionAB
1 1 1    doActionAB

Let's say you "optimize" it to

if (a && b) {
   doActionAB();
} else if (c) {
    doActionC();
}

Then the truth table becomes:

a b c
0 0 0    N/A
0 0 1    DoActionC
0 1 0    N/A
0 1 1    doActionC
1 0 0    N/A
1 0 1    doActionC
1 1 0    doActionAB
1 1 1    doActionAB

So, now, for a,!b,c you're also triggering doActionC.

In the case of this code, it would mean that a PushGetBlocks could be called when, during import, a node sends us a "inv" with a block that we already have. I'm not sure whether this is desired behavior or not, but I don't think so as Sipa's patch seems like a "ignore the inv request mechanism while importing".

sipa commented at 2:52 PM on September 28, 2012: member

@Diapolo I haven't checked lately, but if I recall correctly, IMPLEMENT_RANDOMIZE_STACK was effectively compiled away in recent GCCs. I've already argued for removing it, and still think we should - we have -fstack-protector to accomplish this now. I certainly won't implement it myself for new threads, though I wom't stop anyone from adding it.

Diapolo commented at 3:15 PM on September 28, 2012: none

@sipa I'm not an in depth security guru, so if we are safe and the macro doesn't add anything to our security hull I'm also fine with removing it entirely. Perhaps the person who introduced it can comment?

Edit: When GCC optimises it away, that would be also a fact pro removal IMO.

Edit 2: Other than that I like this patch :).

laanwj commented at 4:16 PM on September 28, 2012: member

I remember we had this discussion before in #bitcoin-dev and decided we're keeping the macro, but not adding it to newly introduced threads.

Diapolo commented at 4:59 PM on September 28, 2012: none

@laanwj Some decissions I don't understand ... the code could be a little easier to read without it and would be shorter. Did no one want to remove it (do the work)?

Edit: I like that we now remove that thing :).

BitcoinPullTester commented at 6:55 AM on September 29, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/8f302d9a105919b7ff2ca90b52014fd628ccb25f for binaries and test log.

in src/main.cpp:None in 8f302d9a10 outdated

2329 | +        uiInterface.InitMessage(_("Importing bootstrap blockchain data file."));
2330 | +
2331 | +        FILE *file = fopen(pathBootstrap.string().c_str(), "rb");
2332 | +        if (file) {
2333 | +            filesystem::path pathBootstrapOld = GetDataDir() / "bootstrap.dat.old";
2334 | +            LoadExternalBlockFile(file);

laanwj commented at 3:50 AM on October 1, 2012:

Shouldn't fImporting be set here?

sipa commented at 12:18 AM on October 5, 2012:

Yes, indeed.

sipa commented at 1:31 PM on October 7, 2012: member

@laanwj Updated; moved the management of fImporting to a RAII object.

laanwj commented at 9:25 AM on October 13, 2012: member

ACK

Move external block import to separate thread 66b02c93e6

BitcoinPullTester commented at 4:06 PM on October 20, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/a1902ea46f06b7fe7d57356fa9fe8bbd3d5aae02 for binaries and test log.

jgarzik commented at 5:54 PM on October 20, 2012: contributor

ACK

gavinandresen commented at 8:31 PM on October 20, 2012: contributor

ACK

jgarzik referenced this in commit 38ac953b9d on Oct 20, 2012

jgarzik merged this on Oct 20, 2012

jgarzik closed this on Oct 20, 2012

laudney referenced this in commit b2cc96fe27 on Mar 19, 2014

DrahtBot locked this on Sep 8, 2021