This PR enables a non-wallet functional test mempool_updatefromblock.py to be run without wallet compiled, by using the MiniWallet proposed in #20078. This picks up from where #21999 left off.
Commit 1 of 2: MiniWallet changes only to minimize diffs for easier reviewing Commit 2 of 2: Refactor (renaming a function and variables)
The test run time decreases from 16.5s to 4.3s.
131+ self.transaction_list_test(size=100, n_tx_to_mine=[25, 50, 75])
132
133
134 if __name__ == '__main__':
135 MempoolUpdateFromBlockTest().main()
136+
Thanks. Fixed. Don’t know why the linter now complains about the spelling errors in unchanged files.
^—- failure generated from test/lint/lint-python.sh src/random.h:92: occuring ==> occurring src/rpc/blockchain.cpp:794: nWe ==> new src/util/syscall_sandbox.cpp:128: creat ==> create test/functional/data/rpc_decodescript.json:81: ba ==> by, be test/functional/data/rpc_decodescript.json:84: ba ==> by, be ^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt
Are you still working on this?
Yes. Read mzumsande’s comment too quickly and didn’t realize it was a request to change. Will fix soon.
tACK 203a3e065ecd0ec8a9fb84f4deb512ef2b66cdf7.
Tested on NixOS 22.05 64 bits with --disable-wallet.
I verified that the migration to MiniWallet does not alter what is being tested. It takes 3s to run on my machine 🚀
80@@ -109,7 +81,7 @@ def transaction_graph_test(self, size, n_tx_to_mine=None, start_input_txid='', e
81 self.log.info('Checking descendants/ancestors properties of all of the in-mempool transactions...')
82 for k, tx in enumerate(tx_id):
83 self.log.debug('Check transaction #{}.'.format(k))
84- entry = self.nodes[0].getmempoolentry(tx)
85+ entry = self.nodes[0].getrawmempool(True)[tx['txid']]
Changed back to getmempoolentry.
I chose getrawmempool over getmempoolentry to follow some existing functional test files. After a closer look at those two RPCs, I agree the existing getmempoolentry here is better as it indicates the “query using a transaction id” intention.
78- outputs = {end_address: output_value}
79+ parent_utxo = miniwallet.get_utxo(txid=tx_id[i-1]['txid'])
80
81- self.log.debug('output_value={}'.format(output_value))
82- self.log.debug('outputs={}'.format(outputs))
83+ tx = miniwallet.send_self_transfer(from_node=self.nodes[0], utxo_to_spend=parent_utxo, sequence=SEQUENCE_FINAL)
SEQUENCE_FINAL?
Without SEQUENCE_FINAL, the test broke at assert_equal(len(node.getrawmempool()), size) after the first block is invalidated, as the mempool has 26 transactions instead of the expected 100.
In a simpler example with fewer transactions without SEQUENCE_FINAL, I can see after the block is invalidated, transactions are removed from the mempool because of this place: https://github.com/bitcoin/bitcoin/blob/f05cf59d91eb03857dd9bdcc77607764da0349d2/src/consensus/tx_verify.cpp#L53
86- # Create a new transaction.
87- unsigned_raw_tx = self.nodes[0].createrawtransaction(inputs, outputs)
88- signed_raw_tx = self.nodes[0].signrawtransactionwithwallet(unsigned_raw_tx)
89- tx_id.append(self.nodes[0].sendrawtransaction(signed_raw_tx['hex']))
90- tx_size.append(self.nodes[0].getmempoolentry(tx_id[-1])['vsize'])
91+ vsize = self.nodes[0].getrawmempool(True)[tx['txid']]['vsize']
getmempoolentry.
38
39 If all of the N created transactions tx[0]..tx[N-1] reside in the mempool,
40 the following holds:
41 the tx[K] transaction:
42 - has N-K descendants (including this one), and
43 - has K+1 ancestors (including this one)
I think this PR introduces an unwanted behavior change. It looks like now you have a simple straight line of spends tx0->tx1->…->txN.
Before this test created a huge, complicated “cyclic tournament” graph of transactions where:
That’s kind of hard to visualize. But the first transaction tx0 has 99 outputs, and tx1, … tx100 all spend one of tx0’s outputs. And tx100 has 99 inputs, an input from tx1, … tx99. Looking at L51-L80 (before this PRs changes) you can try to imagine what tx4 looks like and that’s kinda hard!
It’s easy to see by the runtime of this test. Before this PR running this test in our CI took ~5 minutes. Now this test runs in ~1s.
I’ve found this test to be extremely valuable bc it highlights a possible denial of service attack vector. Imagine someone creates a graph of transactions like this and gets them all mined into a single block (let’s just say they mine the block or collude w/ a miner). Even though the mempool has DEFAULT_ANCESTOR_LIMIT and DEFAULT_DESCENDANT_LIMIT of 25, this test just shows how large and complicated a web of dependent transactions you can make within those limits.
Now imagine that huge graph takes up a single block that’s mined. And then somehow (maybe on purpose) that block gets reorged. All of a sudden this entire block of transactions enters the mempool again. And all the nodes need to validate every single transaction in this graph before adding them back to their mempool. Imagine if we did something stupid (or a lot of extra looping work) in the mempool validation logic. When that block gets reorged it could potentially crash or stall a bunch of nodes.
So I think this test is a really good stress test as-is. If you do any looping or weird stuff in the mempool validation logic this test can very easily timeout. I learned this the hard way when trying out some RBF changes, where I saw this test timing out. And I started to really appreciate the value of this specific test at that point
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.