[net] count mempool and extra pool matches correctly in PartiallyDownloadedBlock::InitData() #9591

jnewbery commented at 11:33 PM on January 19, 2017: member

This PR fixes an issue where the mempool_count and extra_count matches can be miscounted when reconstructing a compact block.

The first commit 4ac1845 is just code style fixes. The second commit 26ceb1e should fix the mempool_count and extra_count counting issues.

For @TheBlueMatt to review.

trivial: adhere to doc/developernotes.md coding style 4ac1845492

fanquake added the label Mempool on Jan 20, 2017

in src/blockencodings.cpp:None in 26ceb1e42c outdated

 147 | +            if (missing_txs == 0 && !shortid_collision) {
 148 | +                // We've found transactions matching all of the shortids and haven't yet
 149 | +                // found a shortid collision. We could continue scanning through the
 150 | +                // mempool and extra pool in case there's a shortid collision later,
 151 | +                // but running GetShortID() on the entire mempool and extra pool is an
 152 | +                // expensive operation.

TheBlueMatt commented at 4:04 PM on January 20, 2017:

Wouldn't call it "expensive" per se, but weighing a 1 in a million case where we use more bandwidth against something that takes 10ms...I'll take the 1 in a million.

TheBlueMatt commented at 4:04 PM on January 20, 2017: member

I believe we generally dont do commits which purely change code style to avoid needless code movement.

in src/blockencodings.cpp:None in 26ceb1e42c outdated

 134 |                      // This should be rare enough that the extra bandwidth doesn't matter,
 135 |                      // but eating a round-trip due to FillBlock failure would be annoying
 136 |                      if (txn_available[idit->second]) {
 137 |                          txn_available[idit->second].reset();
 138 | -                        mempool_count--;
 139 | +                        shortid_collision = true;

TheBlueMatt commented at 4:10 PM on January 20, 2017:

Why not just increment missing_txs here and drop shortid_collision?

jnewbery commented at 4:17 PM on January 20, 2017:

Because there's practically no performance impact of having a shortid_collision bool, and it makes the logic easier to understand.

TheBlueMatt commented at 5:10 PM on January 20, 2017:

It made it more complicated to me, but OK.

jock08chi2211 commented at 4:11 PM on January 20, 2017: none

Omg

On Thursday, January 19, 2017, John Newbery notifications@github.com wrote:

This PR fixes an issue where the mempool_count and extra_count matches can be miscounted when reconstructing a compact block.

The first commit 4ac1845 https://github.com/bitcoin/bitcoin/commit/4ac1845492c08ff8fb25f0c7bb94f8fdce2009db is just code style fixes. The second commit 26ceb1e https://github.com/bitcoin/bitcoin/commit/26ceb1e42c1e16497b5505ae8696f8b80e149cfb should fix the mempool_count and extra_count counting issues.

For @TheBlueMatt https://github.com/TheBlueMatt to review.

You can view, comment on, or merge this pull request online at:

#9591 Commit Summary

trivial: adhere to doc/developernotes.md coding style

Compact blocks - count mempool and extra pool matches correctly

File Changes

M src/blockencodings.cpp https://github.com/bitcoin/bitcoin/pull/9591/files#diff-0 (133)

Patch Links:

#9591.patch

#9591.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9591, or mute the thread https://github.com/notifications/unsubscribe-auth/AX275nyfOXLPtpCxR5V9_kYDc_myoudZks5rT_LKgaJpZM4Lova8 .

-- Jr

in src/blockencodings.cpp:None in 26ceb1e42c outdated

 240 | @@ -227,7 +241,7 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
 241 |          return READ_STATUS_CHECKBLOCK_FAILED;
 242 |      }
 243 |  
 244 | -    LogPrint("cmpctblock", "Successfully reconstructed block %s with %lu txn prefilled, %lu txn from mempool (incl at least %lu from extra pool) and %lu txn requested\n", hash.ToString(), prefilled_count, mempool_count, extra_count, vtx_missing.size());
 245 | +    LogPrint("cmpctblock", "Successfully reconstructed block %s with %lu txn prefilled, %lu txn from mempool, %lu from extra pool and %lu txn requested\n", hash.ToString(), prefilled_count, mempool_count, extra_count, vtx_missing.size());

TheBlueMatt commented at 4:13 PM on January 20, 2017:

I think you want an "up to %lu txn from mempool, up to %lu from extra pool and..."

jnewbery commented at 4:19 PM on January 20, 2017:

why? extra_count should now correctly count how many of the transactions were found in the extra pool.

TheBlueMatt commented at 5:09 PM on January 20, 2017:

Neither mempool_count or extra_count are decremented when we drop a transaction due to a collision, now, I believe.

jnewbery commented at 5:12 PM on January 20, 2017:

yes, you're right. I'll change the log.

TheBlueMatt commented at 4:14 PM on January 20, 2017: member

I suppose this should get an 0.14 tag - it fixes a super minor bug where debug log containing bogus information, though I dont believe there are any actual logic changes intended.

Compact blocks - count mempool and extra pool matches correctly c189da97dd

jnewbery force-pushed on Jan 20, 2017

jnewbery renamed this:
~~[WIP] count mempool and extra pool matches correctly in PartiallyDownloadedBlock::InitData()~~
[net] count mempool and extra pool matches correctly in PartiallyDownloadedBlock::InitData()
on Jan 20, 2017

jnewbery commented at 5:42 PM on January 20, 2017: member

Updated the log as suggested.

Removing WIP tag - open for general review.

morcos commented at 7:51 PM on January 20, 2017: member

Code review of c189da9 only Did not review code cleanup commit

I think it's slightly clearer this way, but just removing extra_count--; at line 156 may capture the same goal with much less code change.

jnewbery commented at 3:57 PM on January 23, 2017: member

As morcos points out, since this doesn't actually fix the count issue, it's a whole lot of code churn for not much gain. Closing this for now while I think about whether it's worth actually fixing it properly.

jnewbery closed this on Jan 23, 2017

MarcoFalke locked this on Sep 8, 2021