test: Add functional tests for blockreconstructionextratxn and extra pool (compactblocks)

bigshiny90 commented at 4:40 pm on July 20, 2025: none

This tests the -blockreconstructionextratxn parameter and extra pool memory (ring buffer) behavior used specifically for compact block reconstruction. The “extra transaction pool” stores transactions that were rejected from the mempool for policy reasons (dust, low fees, non-standard scripts), txs replaced via rbf, etc…

There is no test coverage for this. (no unit tests either as the code being tested - in PeerManagerImpl - isn’t publicly accessible in the net_processing.h file)

The code being tested is in src/net_processing.cpp - look for vExtraTxnForCompact, AddToCompactExtraTransactions and -blockreconstructionextratxn start arg.

This tests

Policy-rejected transactions are stored in the extra pool: Transactions rejected for being dust, having low fees, or using non-standard scripts are kept in the extra pool Pool capacity limits work correctly: Tests various pool sizes. Default is 100. Eviction behavior: When the extra pool is full, the oldest transactions are evicted (FIFO) Wraparound behavior: The extra pool correctly wraps around when adding new transactions Compact block reconstruction: These extra transactions are actually used during block reconstruction

Uses policy rejected transactions to populate the extra pool for tests.

(Updated 08/29/25: rebased to currrent master and fixed tests to accommodate new low-fee rate policy)

DrahtBot added the label Tests on Jul 20, 2025

DrahtBot commented at 4:40 pm on July 20, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33023.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

so we can eg wait until a particular block is announced. -> # so we can e.g. wait until a particular block is announced. [Uses standard abbreviation punctuation for “for example”, improving clarity]

drahtbot_id_5_m

DrahtBot added the label CI failed on Jul 20, 2025

DrahtBot commented at 6:35 pm on July 20, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/46342861746 LLM reason (✨ experimental): The CI failure is primarily caused by lint errors detected by ruff, including issues with trailing whitespace and possible spelling errors.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

bigshiny90 force-pushed on Jul 23, 2025

fanquake marked this as a draft on Jul 24, 2025

bigshiny90 force-pushed on Jul 24, 2025

bigshiny90 commented at 11:31 am on July 24, 2025: none

updated to fix CI Lint errors

(also had to remove an unused param)

fanquake marked this as ready for review on Jul 24, 2025

bigshiny90 force-pushed on Jul 24, 2025

DrahtBot removed the label CI failed on Jul 24, 2025

brunoerg commented at 7:48 pm on July 24, 2025: contributor

I think the description is not clear about what you’re trying to cover/achieve with the test. Can you explain it better? Also, corecheck doesn’t show any related new coverage.

bigshiny90 commented at 8:11 pm on July 24, 2025: none

I think the description is not clear about what you’re trying to cover/achieve with the test. Can you explain it better? Also, corecheck doesn’t show any related new coverage.

Added better description (hopefully) to PR

I’m not sure about corecheck, but i’ll hazard a guess:

There are other tests that cover the p2p_compactblocks - so these tests seem to cover the same ground, though in reality they are testing something new. (also, the fact that most of what is being tested is not publicly exposed). Just a guess, as I don’t know how corecheck determines things

Hopefully answers the questions?

bigshiny90 renamed this:
~~test: Add functional tests for blockreconstructionextratxn parameter~~
test: Add functional tests for blockreconstructionextratxn and extra pool (compactblocks)
on Jul 25, 2025

in test/functional/p2p_compactblocks_extratxs.py:411 in 4985235dd2 outdated

406+        self.make_utxos()
407+
408+        # Ensure segwit is active
409+        assert softfork_active(self.nodes[0], "segwit")
410+
411+       # Test policy rejection types first

luke-jr commented at 9:18 pm on July 29, 2025:

Missing a space

bigshiny90 commented at 10:05 pm on July 29, 2025:

fixed. Thanks!

luke-jr changes_requested

bigshiny90 force-pushed on Jul 29, 2025

bigshiny90 requested review from brunoerg on Jul 29, 2025

bigshiny90 requested review from luke-jr on Jul 29, 2025

bigshiny90 force-pushed on Jul 30, 2025

luke-jr referenced this in commit d78bbe5707 on Aug 4, 2025

in test/functional/p2p_compactblocks_extratxs.py:390 in bd1c6b4ea9 outdated

385+        self.restart_node_with_limit(count=100000)  # Very large but reasonable
386+
387+        # Add some transactions - should work but be clamped
388+        rejected_txs = self.populate_extra_pool(10)
389+        result = self.send_compact_block(rejected_txs, list(range(10)))
390+        assert_equal(result["missing_indices"], [])

kevkevinpal commented at 9:55 pm on August 25, 2025:

This seems misplaced for this test test_extratxn_invalid_parameters

bigshiny90 commented at 11:41 pm on August 25, 2025:

removed. now just tests for invalid capacity -1

in test/functional/p2p_compactblocks_extratxs.py:367 in bd1c6b4ea9 outdated

362+        # Try to reconstruct with all 10
363+        indices = list(range(num_txs))
364+        result = self.send_compact_block(rejected_txs, indices)
365+
366+        # First 8 should be missing (evicted)
367+        expected_missing = list(range(8))

kevkevinpal commented at 9:58 pm on August 25, 2025:

instead of 8 can you make this num_txs - count, it makes sense to not have magic numbers

bigshiny90 commented at 11:43 pm on August 25, 2025:

done

in test/functional/p2p_compactblocks_extratxs.py:351 in bd1c6b4ea9 outdated

346+        # Verify wraparound worked correctly - first new_tx_count should be evicted
347+        expected_missing = list(range(new_tx_count))
348+        assert_equal(result2['missing_indices'], expected_missing)
349+        self.log.info(f"✓ Wraparound worked correctly! Transactions {expected_missing} were evicted as expected")
350+
351+    def test_extratxn_minimal_capacity_eviction(self):

kevkevinpal commented at 10:03 pm on August 25, 2025:

what is the point of testing with 2 when you have another test, testing with 1?

bigshiny90 commented at 11:43 pm on August 25, 2025:

yes, removed redundant test - test_extratxn_minimal_capacity_eviction

in test/functional/p2p_compactblocks_extratxs.py:259 in bd1c6b4ea9 outdated

254+        indices = list(range(buffersize))
255+        result = self.send_compact_block(rejected_txs, indices)
256+        assert_equal(result["missing_indices"], indices)
257+        self.log.info(f"✓ All {buffersize} transactions are missing (extra txn pool disabled)")
258+
259+    def test_extratxnpool_capacity(self):

kevkevinpal commented at 10:07 pm on August 25, 2025:

If you could why not set buffersize to 400, and then after checking the capacity test the wrap around?

Then you can drop test_extratxn_large_capacity and test_extratxn_buffer_wraparound seems redundant to do the setup multiple times when they can be done in the same test.

bigshiny90 commented at 11:46 pm on August 25, 2025:

I combined the 3 separate tests - test_extratxnpool_capacity, test_extratxn_large_capacity, test_extratxn_buffer_wraparound - and created one test test_extratxnpool_capacity_and_wraparound, which tests capacity 400 and wraparound behavior

kevkevinpal commented at 10:09 pm on August 25, 2025: contributor

I get the idea of testing the extra pool. I think some of the tests can be consolidated.

I did run the test locally, and it passes for me. I haven’t checked code coverage either to see if it covers anything extra,

but grep -nri "blockreconstructionextratxn" ./test/functional/ does not have any matches

bigshiny90 force-pushed on Aug 25, 2025

bigshiny90 requested review from kevkevinpal on Aug 25, 2025

DrahtBot added the label CI failed on Aug 26, 2025

luke-jr referenced this in commit 1af7b8a326 on Aug 28, 2025

DrahtBot commented at 7:51 am on August 29, 2025: contributor

Could turn into draft while CI is red?

bigshiny90 commented at 1:29 pm on August 29, 2025: none

investigating why tests fail on github CI… but runs fine on a local build

test: Add functional tests for blockreconstructionextratxn parameter

This adds comprehensive functional tests for the extra transaction pool
used in compact block reconstruction, controlled by the
-blockreconstructionextratxn parameter.

841b3c2e96

bigshiny90 force-pushed on Aug 29, 2025

bigshiny90 commented at 2:02 pm on August 29, 2025: none

needed to rebase to current master and fix tests for the new lower fee-rate policy.

DrahtBot removed the label CI failed on Aug 29, 2025

test: Add functional tests for blockreconstructionextratxn and extra pool (compactblocks) #33023

Code Coverage & Benchmarks

Reviews

LLM Linter (✨ experimental)

so we can eg wait until a particular block is announced. -> # so we can e.g. wait until a particular block is announced. [Uses standard abbreviation punctuation for “for example”, improving clarity]