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
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32516.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Possible typos and grammar issues:
<sup>drahtbot_id_4_m</sup>
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)
MiniWallet doesn't appear to support more than 49 utxos at a time? Unclear to me what's going on so I did this instead.
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:
self.generate(wallet, 300)
(added +100 to let the coins mature)
Oh, mine to wallet, not to node which has attached wallet :facepalm:
I don't think I'm actually hitting that particular limit, getting weird results when looking closer, draft for now while I look
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/42311652764</sub>
<sub>LLM reason (✨ experimental): The CI failure is due to lint errors identified by ruff.
</sub>
<details><summary>Hints</summary>
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.
</details>
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)
I don't understand this comment. Did you use prioritisetransaction in an older version, but dropped that?
oops, yep, removed
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.
Kept the test setup as is, since it tests a meaningful 3 block reorg. Changed the comment because the motivation is different.
Concept ACK
Code Review ACK 2b202e9db56487e658fc41089178f31ef4259a0d
Concept ACK
tACK 2b202e9db56487e658fc41089178f31ef4259a0d
DisconnectTip() & reconnect via invalidateblock and reconsiderblockLimitMemoryUsage(): exercised by flooding > 20 MBtest_chainlimits_exceededLogs
2025-05-21T16:57:42.023000Z TestFramework (INFO): PRNG seed is: 4085491027579870526
2025-05-21T16:57:42.024000Z TestFramework (INFO): Initializing test directory /var/folders/jb/wlbrz0t95vl58wzxjt75wqmh0000gn/T/bitcoin_func_test_jb27si_k
2025-05-21T16:57:42.337000Z TestFramework (INFO): Creating 100 transactions...
2025-05-21T16:57:42.717000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2025-05-21T16:57:42.722000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2025-05-21T16:57:44.161000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2025-05-21T16:57:44.166000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2025-05-21T16:57:46.466000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2025-05-21T16:57:46.472000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2025-05-21T16:57:48.423000Z TestFramework (INFO): The last batch of 25 transactions has been accepted into the mempool.
2025-05-21T16:57:48.571000Z TestFramework (INFO): All of the recently mined transactions have been re-added into the mempool in 0.1478879451751709 seconds.
2025-05-21T16:57:48.571000Z TestFramework (INFO): Checking descendants/ancestors properties of all of the in-mempool transactions...
2025-05-21T16:57:48.633000Z TestFramework (INFO): Creating independent transactions to test MAX_DISCONNECTED_TX_POOL_BYTES limit during reorg
2025-05-21T16:57:53.509000Z TestFramework (INFO): Check that too long chains on reorg are handled
2025-05-21T16:57:55.389000Z TestFramework (INFO): Stopping nodes
2025-05-21T16:57:55.494000Z TestFramework (INFO): Cleaning up /var/folders/jb/wlbrz0t95vl58wzxjt75wqmh0000gn/T/bitcoin_func_test_jb27si_k on exit
2025-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
# At the end the old fork is revalidated to cause reorg, and all of the created
done
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])
Just a comment, I think it is kind of fine to do the reorgs through invalidateblock instead of connecting and disconnecting nodes from each other, but the disconnectpool behaviour here is kind of on the line where I would tend to prefer exercising the actual code through 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.
did a proper reorg using blocktools to prep the fork.
Concept ACK
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)
nit in 1aeb1838cabf63507a2885e50499d94588f0fd83: Can drop the call here and use the mempool from the next line?
done
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
nit in 05abcc86998f6649b48ed3a2eaf2a024ba42af5c: Remove the comment, or add "coins to test", or "coins for test"?
deleted
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?
... size, *, n_tx_to...
done
lgtm ACK 2b202e9db56487e658fc41089178f31ef4259a0d 🛤
<details><summary>Show signature</summary>
Signature:
untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 2b202e9db56487e658fc41089178f31ef4259a0d 🛤
SBccUIAZKcRyVW63zp8B0dLkfguSYFAfLhxM+uFjRVeT01zjRQ9onvrSXYmRRKWtG0RZDIGQjc6YaDks3w3JDw==
</details>
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?
too-long-mempool-chain, too many unconfirmed ancestors [limit: 100]
done
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 đźš‹
<details><summary>Show signature</summary>
Signature:
untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 84aa484d45e2fb3c1149941ef23779e4adb983d9 đźš‹
O4vg4KJB7fyQgaqyIEXZN4vN/M1IJ1RGjEYbRriXHFjjAAucXCr+GECIWTscWQkia3xx30XE6hi6vtHbKfZZDQ==
</details>
Thanks for entertaining my suggestion :)
ACK 84aa484d45e2fb3c1149941ef23779e4adb983d9
Post-Merge ACK 84aa484d45e2fb3c1149941ef23779e4adb983d9