DisconnectedBlockTransactions::LimitMemoryUsage() has unit test coverage, but the default value end to end doesn’t have coverage.
This test adds exercised coverage of memory limiting of the disconnect pool, and some basic behavior sanity checks.
Another test added is making sure chainlimits are being respected on reorg, and the expected transactions pruned.
Lastly, fix the existing test case which was using a deficient test via directly inducing reorgs with invalidateblock
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32516.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | maflcko, TheCharlatan |
| Stale ACK | mzumsande, i-am-yuvi |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Possible typos and grammar issues:
drahtbot_id_4_m
97@@ -97,6 +98,53 @@ def transaction_graph_test(self, size, n_tx_to_mine=None, fee=100_000):
98 assert_equal(entry['ancestorcount'], k + 1)
99 assert_equal(entry['ancestorsize'], sum(tx_size[0:(k + 1)]))
100
101+ self.generate(self.nodes[0], 1)
102+ assert_equal(self.nodes[0].getrawmempool(), [])
103+ wallet.rescan_utxos()
104+
105+ # Make 200 outputs to spend via wallet
106+ wallet.send_self_transfer_multi(from_node=self.nodes[0], num_outputs=200)
Not exactly sure if this is what you are asking, but: new blocks with coinbase transaction outputs for MiniWallet can be generated by passing an instance of the latter to self.generate (instead of the node object), i.e. in this case:
0 self.generate(wallet, 300)
(added +100 to let the coins mature)
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/42311652764
LLM reason (✨ experimental): The CI failure is due to lint errors identified by ruff.
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.
128+ large_std_txs = []
129+ # Add children to ensure they're recursively removed if disconnectpool trimming of parent occurs
130+ small_child_txs = []
131+ aggregate_serialized_size = 0
132+ while aggregate_serialized_size < MAX_DISCONNECTED_TX_POOL_BYTES:
133+ # Mine parents in FIFO order via priority (priority map wiped during reorg)
196@@ -190,7 +197,7 @@ def test_chainlimits_exceeded(self):
197
198 def run_test(self):
199 # Use batch size limited by DEFAULT_ANCESTOR_LIMIT = 25 to not fire "too many unconfirmed parents" error.
This comment is outdated, because DEFAULT_ANCESTOR_LIMIT = 25 is no longer after the last commit. Not sure about the reason why it was used before, is it no longer valid?
Also, changing DEFAULT_ANCESTOR_LIMIT to 100 could be mentioned in the commit message, it also affects the other subtests.
tACK 2b202e9db56487e658fc41089178f31ef4259a0d
DisconnectTip() & reconnect via invalidateblock and reconsiderblockLimitMemoryUsage(): exercised by flooding > 20 MBtest_chainlimits_exceededLogs
02025-05-21T16:57:42.023000Z TestFramework (INFO): PRNG seed is: 4085491027579870526
12025-05-21T16:57:42.024000Z TestFramework (INFO): Initializing test directory /var/folders/jb/wlbrz0t95vl58wzxjt75wqmh0000gn/T/bitcoin_func_test_jb27si_k
22025-05-21T16:57:42.337000Z TestFramework (INFO): Creating 100 transactions...
32025-05-21T16:57:42.717000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
42025-05-21T16:57:42.722000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
52025-05-21T16:57:44.161000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
62025-05-21T16:57:44.166000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
72025-05-21T16:57:46.466000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
82025-05-21T16:57:46.472000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
92025-05-21T16:57:48.423000Z TestFramework (INFO): The last batch of 25 transactions has been accepted into the mempool.
102025-05-21T16:57:48.571000Z TestFramework (INFO): All of the recently mined transactions have been re-added into the mempool in 0.1478879451751709 seconds.
112025-05-21T16:57:48.571000Z TestFramework (INFO): Checking descendants/ancestors properties of all of the in-mempool transactions...
122025-05-21T16:57:48.633000Z TestFramework (INFO): Creating independent transactions to test MAX_DISCONNECTED_TX_POOL_BYTES limit during reorg
132025-05-21T16:57:53.509000Z TestFramework (INFO): Check that too long chains on reorg are handled
142025-05-21T16:57:55.389000Z TestFramework (INFO): Stopping nodes
152025-05-21T16:57:55.494000Z TestFramework (INFO): Cleaning up /var/folders/jb/wlbrz0t95vl58wzxjt75wqmh0000gn/T/bitcoin_func_test_jb27si_k on exit
162025-05-21T16:57:55.494000Z TestFramework (INFO): Tests successful
90+ self.generate(self.nodes[0], 1)[0]
91 assert_equal(len(self.nodes[0].getrawmempool()), 0)
92 self.log.info('All of the transactions from the current batch have been mined into a block.')
93 elif tx_count == size:
94- # At the end all of the mined blocks are invalidated, and all of the created
95+ # At the end all of the mined the old fork is revalidated to cause reorg, and all of the created
0 # At the end the old fork is revalidated to cause reorg, and all of the created
114+ assert_equal(self.nodes[0].getrawmempool(), [])
115+
116+ # Set up empty fork blocks ahead of time, needs to be longer than full fork made later
117+ fork_length = 60
118+ fork_blocks = self.generate(self.nodes[0], fork_length)
119+ self.nodes[0].invalidateblock(fork_blocks[0])
ActiveBestChainStep, since the two methods do not share the same object. I think to test the mempool in general it is fine to do, both methods at least use the same object at runtime.
146+
147+ # Reorg back before the first block in the series, should drop something
148+ # but not all, and any time parent is dropped, child is also removed
149+ self.nodes[0].reconsiderblock(fork_blocks[0])
150+ expected_parent_count = len(large_std_txs) - 2
151+ assert_equal(len(self.nodes[0].getrawmempool()), expected_parent_count * 2)
mempool from the next line?
154@@ -155,11 +155,46 @@ def test_max_disconnect_pool_bytes(self):
155 assert_equal([tx["txid"] in mempool for tx in large_std_txs], [tx["txid"] in mempool for tx in small_child_txs])
156 assert_equal([tx["txid"] in mempool for tx in large_std_txs], [True] * expected_parent_count + [False] * 2)
157
158+ def test_chainlimits_exceeded(self):
159+ self.log.info('Check that too long chains on reorg are handled')
160+
161+ # Generate coins test
26- self.extra_args = [['-limitdescendantsize=1000', '-limitancestorsize=1000', '-limitancestorcount=100', '-datacarriersize=100000']]
27+ # Ancestor and descendant limits depend on transaction_graph_test requirements
28+ self.extra_args = [['-limitdescendantsize=1000', '-limitancestorsize=1000', f'-limitancestorcount={CUSTOM_ANCESTOR_COUNT}', f'-limitdescendantcount={CUSTOM_DESCENDANT_COUNT}', '-datacarriersize=100000']]
29
30- def transaction_graph_test(self, size, n_tx_to_mine=None, fee=100_000):
31+ def transaction_graph_test(self, size, n_tx_to_mine, fee=100_000):
nit in 2b202e9db56487e658fc41089178f31ef4259a0d: Could force named args, while touching this?
0... size, *, n_tx_to...
lgtm ACK 2b202e9db56487e658fc41089178f31ef4259a0d 🛤
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK 2b202e9db56487e658fc41089178f31ef4259a0d 🛤
3SBccUIAZKcRyVW63zp8B0dLkfguSYFAfLhxM+uFjRVeT01zjRQ9onvrSXYmRRKWtG0RZDIGQjc6YaDks3w3JDw==
181+ chain = wallet.create_self_transfer_chain(chain_length=CUSTOM_DESCENDANT_COUNT + 2)
182 for tx in chain[:-2]:
183 self.nodes[0].sendrawtransaction(tx["hex"])
184
185- assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants for tx", self.nodes[0].sendrawtransaction, chain[-2]["hex"])
186+ assert_raises_rpc_error(-26, "too-long-mempool-chain, too many", self.nodes[0].sendrawtransaction, chain[-2]["hex"])
nit in https://github.com/bitcoin/bitcoin/commit/2b202e9db56487e658fc41089178f31ef4259a0d: could clarify the exact error string?
0too-long-mempool-chain, too many unconfirmed ancestors [limit: 100]
The current test directly uses invalidatblock to trigger
mempool re-entry of transactions. Unfortunately, the
behavior doesn't match what a real reorg would look like. As
a result you get surprising behavior such as the mempool
descendant chain limits being exceeded, or if a fork is
greater than 10 blocks deep, evicted block transactions stop
being submitted back into in the mempool.
Fix this by preparing an empty fork chain, and then
continuing with the logic, finally submitting the fork chain
once the rest of the test is prepared. This triggers a more
typical codepath.
Also, extend the descendant limit to 100, like ancestor
limit.
209+ assert_equal(self.nodes[0].getrawmempool(), [])
210+
211+ # Last tx fits now
212+ self.nodes[0].sendrawtransaction(chain[-1]["hex"])
213+
214+ # Finally, reorg to empty chain kick everything back into mempool
style nit: either to kick or kicks or will kick
(feel free to ignore)
re-ACK 84aa484d45e2fb3c1149941ef23779e4adb983d9 🚋
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 84aa484d45e2fb3c1149941ef23779e4adb983d9 🚋
3O4vg4KJB7fyQgaqyIEXZN4vN/M1IJ1RGjEYbRriXHFjjAAucXCr+GECIWTscWQkia3xx30XE6hi6vtHbKfZZDQ==
Thanks for entertaining my suggestion :)
ACK 84aa484d45e2fb3c1149941ef23779e4adb983d9