net: optimize compact block extra tx iteration #35670

Probably leave this line as is since otherwise we are relying on this clamp always existing:

https://github.com/bitcoin/bitcoin/blob/32941e131479b4aec1eb606d68a9bcc3f5d7cf3d/src/node/peerman_args.cpp#L20

And there are no tests that enforce this is how -blockreconstructionextratxn works AFAICT.

max_extra_txs is a u32, so it can't can't be negative: https://github.com/bitcoin/bitcoin/blob/32941e131479b4aec1eb606d68a9bcc3f5d7cf3d/src/net_processing.h#L88

I don’t think keeping <= 0 adds protection here, since max_extra_txs is already unsigned. If the args clamp were removed, a negative value would wrap before reaching this guard, so both <= 0 and == 0 would miss it. The local invariant here is just the disabled/zero case before the modulo below. Let me know if I misunderstood what you meant.

Ah thanks, missed that it was unsigned 🤦

non-blocking-nit:

        vExtraTxnForCompact.at(vExtraTxnForCompactIt) = std::make_pair(tx->GetWitnessHash(), tx);

This is a bit safer I thinks.

Thanks, I prefer leaving that as-is to simplify review.

non-blocking-observation-feel-free-to-disregard:

It seems a little awkward that we are tying together size and capacity in this way, this feels like something that should be happening somewhere in a constructor instead of happening inside of the Add function the first time it's invoked with a check to see if this is the first time Add is being called, but given what already exists I think this is a reasonable solution.

should be happening somewhere in a constructor

That would make the memory allocation eager, this is meant to do lazy init.

given what already exists I think this is a reasonable solution

Yeah, the review is simpler if this PR only focuses on one problem at the time: which is uninitialized iteration here.

I agree that this approach is lazy with respect to startup, it is "eager" with respect to the extrapool's use in the sense that the first time you add something to the extrapool you allocate memory for the whole pool even if it might take you much more time before you need that memory to be allocated.

Maybe the right approach here is actually to drop the reserve, it lets us delete a line that doesn't really gain us much, and lets us get even lazier with the allocation.

it lets us delete a line that doesn't really gain us much

Not sure I follow. The reserve is needed to avoid copying the data as we populate it until it fills (in which case it will likely contain more elements than needed). For 100 elements it doesn't matter that much, but as https://github.com/bitcoinknots/bitcoin/blob/f41f01e1e6de7025d52a865bef97f2a67277f0f3/src/net_processing.h#L33 shows, that default isn't always the case. I don't see what advantage removing it would bring, especially in relation to this PR.

The reserve is needed to avoid copying the data as we populate it until it fills (in which case it will likely contain more elements than needed).

This is minimal, even if someone were using an extrapool sized at 32,768 as you linked, and every tx was 10KiB, the final copy of 160MiB would take 12ms on an RPi4.

I don't see what advantage removing it would bring, especially in relation to this PR.

Since this line is being added in this PR, it seems like there should be a justification for what it is doing that std::vector doesn't already do well enough.

Since this line is being added in this PR

I see it more as replacing the old first-use allocation shape than adding a new one:

• previously resize(m_opts.max_extra_txs) allocated storage and made all configured slots part of the vector, creating default entries that reconstruction would later iterate.
• now reserve(m_opts.max_extra_txs) keeps the same first-use capacity reservation, but leaves size() tracking only entries that were actually added.