[p2p] getblock for 1-block reorgs in response to compact block message #13045

instagibbs commented at 5:26 PM on April 20, 2018: member

Our headers announcement logic will download any blocks that match the total work: https://github.com/bitcoin/bitcoin/blob/8b262eb2d80bfa27ae8501078ce47bc1407e9c55/src/net_processing.cpp#L1443

Compact block responses are asymmetric with this: We only intentionally download the block when the new chain has more work, or when it's already marked as "in flight".

A wrinkle here is that the new behavior will only affect high bandwidth compact block peers: currently low bandwidth compact block peers have already marked this particular block as "in flight", which allows the full block to be downloaded with this catch-all "some reason" logic. For high bandwidth compact block peers this message is the first the block has been heard of it, and previous behavior is to simply ignore the compact block announcement.

This change synchronizes the treatment of new block announcements.

fanquake added the label P2P on Apr 20, 2018

in src/net_processing.cpp:2409 in 64025e11d5 outdated

2405 | @@ -2406,10 +2406,11 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2406 |          if (pindex->nStatus & BLOCK_HAVE_DATA) // Nothing to do here
2407 |              return true;
2408 |  
2409 | -        if (pindex->nChainWork <= chainActive.Tip()->nChainWork || // We know something better
2410 | +        if (pindex->nChainWork <= chainActive.Tip()->nChainWork || // We know same or better better

practicalswift commented at 11:03 AM on April 21, 2018:

Nit: "better better" should be something else? :-)

instagibbs force-pushed on Apr 21, 2018

instagibbs commented at 12:43 PM on April 21, 2018: member

@practicalswift fixed

rebroad commented at 10:16 PM on April 21, 2018: contributor

why not simply request the compact block rather than a full block?

TheBlueMatt commented at 10:55 PM on April 21, 2018: member

Should mark the block as in flight if it wasn't already and we requested it. Probably should be checking if it's in flight from the same peer too (which would be required if you mark it, and should probably have been done either way). @promag because the mempool reconstruction will be useless so it'll be worse than a normal request.

On April 21, 2018 10:16:43 PM UTC, Rebroad notifications@github.com wrote:

why not simply request the compact block rather than a full block?

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/13045#issuecomment-383335960

instagibbs force-pushed on Apr 23, 2018

instagibbs commented at 2:09 PM on April 23, 2018: member

@TheBlueMatt Added the filter-by-sender and marking block as in flight.

in src/net_processing.cpp:2411 in a61ea427d5 outdated

2405 | @@ -2406,11 +2406,15 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2406 |          if (pindex->nStatus & BLOCK_HAVE_DATA) // Nothing to do here
2407 |              return true;
2408 |  
2409 | -        if (pindex->nChainWork <= chainActive.Tip()->nChainWork || // We know something better
2410 | +        if (pindex->nChainWork <= chainActive.Tip()->nChainWork || // We know same or better
2411 |                  pindex->nTx != 0) { // We had this block at some point, but pruned it
2412 | -            if (fAlreadyInFlight) {
2413 | -                // We requested this block for some reason, but our mempool will probably be useless
2414 | +            if ((fAlreadyInFlight && blockInFlightIt->second.first == pfrom->GetId())

Empact commented at 9:31 PM on April 23, 2018:

nit: how about unpacking blockInFlightIt->second.first to a local like inFlightNodeId or the like? It's used one other place in the file and is relatively inscrutable.

instagibbs commented at 1:38 PM on April 24, 2018:

I cached the "from same peer" logic instead since it's used twice.

instagibbs force-pushed on Apr 24, 2018

rebroad commented at 2:50 PM on April 25, 2018: contributor

would it be worth keeping the txs that went into the latest block to hand for this situation so that compact blocks can be used? what if the previous compact block received had only one transaction in it, whereas this competing block has many transactions? by requesting a full block we would be favouring the empty block when it would be better for the network to favour the full block. therefore I'd still argue it's better to request a compact block so to not give empty blocks an unfair advantage.

On Sat, 21 Apr 2018, 23:56 Matt Corallo, notifications@github.com wrote:

because the mempool reconstruction will be useless so it'll be worse than a normal request.

instagibbs commented at 2:54 PM on April 25, 2018: member

@rebroad I think this goes a bit off-topic to incentives. As-is it would likely take a lot of work to support it, and I'd have no idea how to do it safely.

in src/net_processing.cpp:2413 in b45a45672c outdated

2411 | +        if (pindex->nChainWork <= chainActive.Tip()->nChainWork || // We know same or better
2412 |                  pindex->nTx != 0) { // We had this block at some point, but pruned it
2413 | -            if (fAlreadyInFlight) {
2414 | -                // We requested this block for some reason, but our mempool will probably be useless
2415 | +            if ((fAlreadyInFlight && in_flight_from_same_peer)
2416 | +                    || pindex->nChainWork == chainActive.Tip()->nChainWork) {

instagibbs commented at 1:25 PM on April 26, 2018:

I think this will actually request the full block that is the known chaintip. Probably need to check it isn't in chainActive first. It already is being checked that it connects, so this is all that is likely needed.

instagibbs commented at 1:52 PM on April 26, 2018:

oh, maybe not. two lines or so above we have this check:

if (pindex->nStatus & BLOCK_HAVE_DATA) // Nothing to do here
            return true;

TheBlueMatt commented at 2:06 AM on April 28, 2018:

I dont think we want to request the block if its already in flight from another peer, even if it has equal work with our tip.

in src/net_processing.cpp:2417 in b45a45672c outdated

2415 | +            if ((fAlreadyInFlight && in_flight_from_same_peer)
2416 | +                    || pindex->nChainWork == chainActive.Tip()->nChainWork) {
2417 | +                // We requested this block for some reason from the same peer
2418 | +                // or just want same height block, but our mempool will probably be useless
2419 |                  // so we just grab the block via normal getdata
2420 | +                std::list<QueuedBlock>::iterator* queuedBlockIt = nullptr;

TheBlueMatt commented at 2:12 AM on April 28, 2018:

This appears to be unused. That said, I think we should consider handling if peer accidentally sends us the block twice, thus queuedBlockIt->partialBlock is set after we call MarkBlockAsInFlight (probably by just wiping the partialBlock).

instagibbs commented at 1:10 PM on April 30, 2018:

Ok, replicated the logic from a few lines below covering the same case.

TheBlueMatt commented at 2:13 AM on April 28, 2018: member

Indeed, that is what I meant wrt in_flight_from_same_peer. It appears to be indentical to what I did in #10984 at https://github.com/bitcoin/bitcoin/pull/10984/commits/a63474546943ef857554adb68c8c0b23e3a24f88 so it must be right! :p.

instagibbs force-pushed on Apr 30, 2018

instagibbs commented at 1:10 PM on April 30, 2018: member

Comments addressed

in src/net_processing.cpp:2420 in b4a3207f3f outdated

2418 | +                // or just want same height block, but our mempool will probably be useless
2419 |                  // so we just grab the block via normal getdata
2420 | +                std::list<QueuedBlock>::iterator* queuedBlockIt = nullptr;
2421 | +                if (!MarkBlockAsInFlight(pfrom->GetId(), pindex->GetBlockHash(), pindex, &queuedBlockIt)) {
2422 | +                    if (!(*queuedBlockIt)->partialBlock) {
2423 | +                        (*queuedBlockIt)->partialBlock.reset(new PartiallyDownloadedBlock(&mempool));

TheBlueMatt commented at 10:04 PM on May 14, 2018:

You want to reset to nullptr here, as we're about to fetch via non-compact-blocks.

instagibbs commented at 1:09 AM on May 16, 2018:

done

instagibbs force-pushed on May 15, 2018

in src/net_processing.cpp:2405 in 8f8eea2e3d outdated

2401 | @@ -2402,15 +2402,28 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2402 |  
2403 |          std::map<uint256, std::pair<NodeId, std::list<QueuedBlock>::iterator> >::iterator blockInFlightIt = mapBlocksInFlight.find(pindex->GetBlockHash());
2404 |          bool fAlreadyInFlight = blockInFlightIt != mapBlocksInFlight.end();
2405 | +        bool in_flight_from_same_peer = fAlreadyInFlight ? blockInFlightIt->second.first == pfrom->GetId() : false;

Empact commented at 3:01 AM on May 16, 2018:

&& over ? here

in src/net_processing.cpp:2412 in 8f8eea2e3d outdated

2410 | -        if (pindex->nChainWork <= chainActive.Tip()->nChainWork || // We know something better
2411 | +        if (pindex->nChainWork <= chainActive.Tip()->nChainWork || // We know same or better
2412 |                  pindex->nTx != 0) { // We had this block at some point, but pruned it
2413 | -            if (fAlreadyInFlight) {
2414 | -                // We requested this block for some reason, but our mempool will probably be useless
2415 | +            if ((fAlreadyInFlight && in_flight_from_same_peer)

Empact commented at 3:03 AM on May 16, 2018:

Redundant fAlreadyInFlight check

in src/net_processing.cpp:2417 in 8f8eea2e3d outdated

2415 | +            if ((fAlreadyInFlight && in_flight_from_same_peer)
2416 | +                    || (!fAlreadyInFlight && pindex->nChainWork == chainActive.Tip()->nChainWork)) {
2417 | +                // We requested this block for some reason from the same peer
2418 | +                // or just want same height block, but our mempool will probably be useless
2419 |                  // so we just grab the block via normal getdata
2420 | +                std::list<QueuedBlock>::iterator* queuedBlockIt = nullptr;

Empact commented at 3:04 AM on May 16, 2018:

nit: snake_case for new vars

in src/net_processing.cpp:2448 in 8f8eea2e3d outdated

2444 | @@ -2432,7 +2445,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2445 |          // possibilities in compact block processing...
2446 |          if (pindex->nHeight <= chainActive.Height() + 2) {
2447 |              if ((!fAlreadyInFlight && nodestate->nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) ||
2448 | -                 (fAlreadyInFlight && blockInFlightIt->second.first == pfrom->GetId())) {
2449 | +                 (fAlreadyInFlight && in_flight_from_same_peer)) {

Empact commented at 3:05 AM on May 16, 2018:

Redundant fAlreadyInFlight check

instagibbs force-pushed on May 21, 2018

instagibbs commented at 3:37 PM on May 21, 2018: member

@Empact comments addressed

getblock for 1-block reorgs in response to compact block message

filter already in flight requests to same peer only

8d3fe87523

in src/net_processing.cpp:2412 in 9fc77a9d8f outdated

2410 | -        if (pindex->nChainWork <= chainActive.Tip()->nChainWork || // We know something better
2411 | +        if (pindex->nChainWork <= chainActive.Tip()->nChainWork || // We know same or better
2412 |                  pindex->nTx != 0) { // We had this block at some point, but pruned it
2413 | -            if (fAlreadyInFlight) {
2414 | -                // We requested this block for some reason, but our mempool will probably be useless
2415 | +            if ((in_flight_from_same_peer)

Empact commented at 6:19 PM on May 21, 2018:

nit: unnecessary parens

instagibbs commented at 4:07 PM on May 22, 2018:

done

instagibbs force-pushed on May 22, 2018

DrahtBot closed this on Jul 22, 2018

DrahtBot reopened this on Jul 22, 2018

DrahtBot commented at 5:52 PM on March 15, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15948 (refactor: rename chainActive by jamesob)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot closed this on Apr 28, 2019

DrahtBot commented at 7:11 PM on April 28, 2019: member

The last travis run for this pull request was 279 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

DrahtBot reopened this on Apr 28, 2019

DrahtBot commented at 4:08 PM on May 7, 2019: member

Needs rebase

DrahtBot added the label Needs rebase on May 7, 2019

instagibbs commented at 4:12 PM on May 7, 2019: member

I don't think anyone remaining who would review cares about this, closing.

instagibbs closed this on May 7, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021