test: run mempool_expiry.py even with wallet disabled

mjdietzx commented at 0:23 am on October 31, 2020: contributor

Run the mempool expiry test even when the wallet was not compiled, as proposed in #20078.

fanquake added the label Tests on Oct 31, 2020

in test/functional/mempool_expiry.py:52 in 98796bba8f outdated

62+        half_expiry_time = entry_time + 3600 * timeout // 2
63         node.setmocktime(half_expiry_time)
64-        child_txid = node.sendrawtransaction(child_signed)
65+
66+        # Broadcast the child transaction spending the parent transaction.
67+        parent_utxo = wallet.get_utxo()

MarcoFalke commented at 8:21 am on October 31, 2020:

Pretty sure this is not the parent change. miniwallet should have two outputs at this point. One leftover coinbase and one change. Coin selection favors the largest output, so the coinbase one.

I might be wrong, but I think it could make sense to check that the child tx really has parent_txid as ancestor via the mempool rpc

mjdietzx commented at 10:39 am on October 31, 2020:

I added an assertion verifying this to make it clear https://github.com/bitcoin/bitcoin/pull/20276/commits/de4173bacd3a2ce6294515072cb725d5c1074cd8.

Originally I had:

0parent_utxo = next(filter(lambda utxo: utxo['txid'] == parent_txid, wallet._utxos))

but then I realized get_utxo() would do this for me: """Return the last utxo. Can be used to get the change output immediately after a send_self_transfer"""

MarcoFalke commented at 3:22 pm on October 31, 2020:

Oh, sorry my bad in that case. Though, the mempool check would still make sense.

mjdietzx commented at 5:36 pm on October 31, 2020: contributor

@MarcoFalke not to beat this to death, but I added another test case in https://github.com/bitcoin/bitcoin/pull/20276/commits/71081df19a816b5c243a6bcccab9fb11888732fa. While convincing myself that I was using miniWallet properly, and not getting lucky with any passing tests, I wrote this and figured it was useful enough to include in the PR

mjdietzx force-pushed on Nov 28, 2020

mjdietzx commented at 4:17 pm on November 28, 2020: contributor

Rebased and should be ready to merge

in test/functional/mempool_expiry.py:32 in 124c8ab5df outdated

25@@ -26,57 +26,62 @@
26 class MempoolExpiryTest(BitcoinTestFramework):
27     def set_test_params(self):
28         self.num_nodes = 1
29+        self.setup_clean_chain = True
30 
31-    def skip_test_if_missing_module(self):
32-        self.skip_if_no_wallet()
33+    def sync_mempool(self):
34+        """Expiry of mempool txs are only checked when a new tx is added to the mempool."""

0xB10C commented at 5:35 pm on November 28, 2020:

nit: I found this function name and doc string not to be super clear. sync_mempool() could be confused with something like self.sync_all(). If you happen to re-touch this then maybe something along the lines of the suggestion below would make it clearer.

0    def trigger_mempool_expiry(self):
1        """Broadcasts an unrelated transaction as the expiry of the transactions in the mempool is only checked when a new transaction is added to the mempool."""

mjdietzx commented at 6:21 pm on November 28, 2020:

Thanks for the suggestion, I made this change and squashed it into the commit

0xB10C commented at 6:01 pm on November 28, 2020: member

ACK 124c8ab5df85b606e00cfa666de6105e0620de00

Thank you @mjdietzx for this PR! Linking my original PR that added this test for context: https://github.com/bitcoin/bitcoin/pull/18172

mjdietzx force-pushed on Nov 28, 2020

0xB10C commented at 10:10 pm on November 28, 2020: member

re-ACK ec66561d87881f54d64c8d633e267efe1b18b4ad

Only changes are:

#20276 (review) addressed
formatting of long lines changed: removed line breaks (multiple lines -> one line)

mjdietzx commented at 8:46 pm on December 1, 2020: contributor

So since we don’t use the wallet in mempool_expiry.py after this PR, does this close #19448? Also, since #19450 is described as a temp workaround would we revert it after this is merged? cc @hebasto

Can someone explain why (only?) this test is triggering a data race though? Everything seems pretty simple so I’m confused why only this test would expose it. And I wonder if it’s not a hint at an underlying problem?

hebasto commented at 9:16 pm on December 1, 2020: member

So since we don’t use the wallet in mempool_expiry.py after this PR, does this close #19448? Also, since #19450 is described as a temp workaround would we revert it after this is merged? cc @hebasto

As noted in #19448 OP other similar issues are reported: #19211 and #19417. I think we could just wait until Berkley DB will no longer used in Bitcoin Core.

Can someone explain why (only?) this test is triggering a data race though? Everything seems pretty simple so I’m confused why only this test would expose it. And I wonder if it’s not a hint at an underlying problem?

I believe something is wrong with Berkeley DB code, but did not check it myself.

in test/functional/mempool_expiry.py:36 in ec66561d87 outdated

33+    def trigger_mempool_expiry(self):
34+        """Broadcasts an unrelated transaction as the expiry of the transactions in the mempool is only checked
35+        when a new transaction is added to the mempool."""
36+        try:
37+            self.wallet.send_self_transfer(from_node=self.nodes[0])
38+        except JSONRPCException:

MarcoFalke commented at 9:43 am on December 2, 2020:

can you explain why this is needed? I’d prefer not to call the method when it won’t do anything anyway

0xB10C commented at 10:25 am on December 2, 2020:

CTxMemPool::Expire(..) is only called in LimitMempoolSize(..) which is only called in UpdateMempoolForReorg(..) and MemPoolAccept::Finalize(..).

To trigger the expiry of a transaction in the mempool we send an unrelated transaction. For the unrelated transaction MemPoolAccept::Finalize(..) is called which calls LimitMempoolSize(..) which calls CTxMemPool::Expire(..) on all timed-out transactions. If we don’t send a new transaction, we can’t test if a transaction is really removed.

MarcoFalke commented at 10:58 am on December 2, 2020:

I meant the except: pass part, sorry for being unclear :sweat_smile:

mjdietzx commented at 6:32 pm on December 2, 2020:

There’s a minor problem here, re the unrelated transaction part of @0xB10C’s explanation. With MiniWallet if we don’t do any extra bookkeeping in this test, and just do self.wallet.send_self_transfer(from_node=node) to trigger the mempool update, the (now expired) utxo will be spent. So this send_self_transfer will throw test_framework.authproxy.JSONRPCException: mempool full (-26)

We’d need to ensure that we don’t spend one of the expired UTXOs when we want to trigger_mempool_expiry if we don’t catch the Exception. ie:

0node.setmocktime(expiry_time)
1# Broadcasts an unrelated transaction as the expiry of the transactions in the mempool is only checked when a new transaction is added to the mempool.
2new_utxo = self.wallet.get_utxo(txid=independent_txid)
3new_txid = self.wallet.send_self_transfer(from_node=self.nodes[0], utxo_to_spend=new_utxo)['txid']
4# parent_txid has now been evicted from the mempool.

Because the mempool is still updated if send_self_transfer fails, I thought it was cleaner to catch the Exception rather than have extra logic (that’s unrelated to what we’re testing) to ensure we’re not spending expired UTXOs in this trigger.

Maybe I need to add a comment along these lines in the except: pass to make this clear? I’m realizing this is more confusing than I thought

MarcoFalke commented at 7:54 am on December 3, 2020:

I don’t understand why spending an non-existent coin will throw ‘mempool full’ (not missing-inputs) and how that is going to trigger mempool expiry of other txs.

Anyway, I guess the options are:

Add a comment
Remember the previous utxo (not expired) and use that
Drop utxos that are about to expire from the wallet (might require fresh coins later on in the script)
Something else?

mjdietzx commented at 8:01 pm on December 8, 2020:

So I figured out why there were some weird errors that didn’t make sense as you point out. After the first subtest some UTXOs were hanging around in the mempool, that ended up being used in the second subtest because MiniWallet sorts for the largest UTXOs. That’s why I ensure the mempool is clear at the beginning of each subtest run now.

in test/functional/mempool_expiry.py:58 in ec66561d87 outdated

68-        child_signed = node.signrawtransactionwithwallet(child_raw)['hex']
69-
70-        # Let half of the timeout elapse and broadcast the child transaction.
71-        half_expiry_time = entry_time + int(60 * 60 * timeout/2)
72+        # Let half of the timeout elapse.
73+        half_expiry_time = entry_time + 3600 * timeout // 2

MarcoFalke commented at 9:45 am on December 2, 2020:

it looks like you are fixing up style in the same commit as refactors, cleanups and miniwallet changes

It would be easier to follow if the formatting changes, the trigger_mempool_expiry, and the miniwallet changes were all in separate commits, where each passed on its own and can be reviewed on its own

mjdietzx commented at 6:37 pm on December 2, 2020:

Ah yeah, I did too much extra stuff in this PR. I’m gonna undo all this unrelated style/refactor stuff. Will undo the string formatting changes as well

in test/functional/mempool_expiry.py:96 in ec66561d87 outdated

128+        assert_equal(half_expiry_time, node.getmempoolentry(independent_txid)['time'])
129 
130     def run_test(self):
131-        self.log.info('Test default mempool expiry timeout of %d hours.' %
132-                      DEFAULT_MEMPOOL_EXPIRY)
133+        self.log.info('Test default mempool expiry timeout of {} hours.'.format(DEFAULT_MEMPOOL_EXPIRY))

MarcoFalke commented at 9:46 am on December 2, 2020:

not sure about mixing all the formatting changes in with the other changes. In either case if you want to change this, you can use f-strings

MarcoFalke commented at 9:47 am on December 2, 2020: member

concept ACK, but the changes were a bit too much for me to properly review

mjdietzx force-pushed on Dec 8, 2020

mjdietzx commented at 7:59 pm on December 8, 2020: contributor

Alright, I cleaned this up and got rid of all the fluff that wasn’t related to the core of this PR. Should be ready to go now, will make sure I keep PRs focused in the future

in test/functional/mempool_expiry.py:38 in 7a05d47cdf outdated

36         children are removed as well."""
37         node = self.nodes[0]
38+        self.wallet = MiniWallet(node)
39+
40+        # Ensure the mempool starts empty.
41+        node.generate(1)

MarcoFalke commented at 9:19 am on December 9, 2020:

Why is this needed? You are generating a block in the next line, which cleares the mempool

mjdietzx commented at 9:12 pm on December 9, 2020:

TLDR; I made everything explicit in this commit https://github.com/bitcoin/bitcoin/pull/20276/commits/4b99dc5ef62fe1d1a6e23d71f55570dc14aac9fa because I realized this is all confusing. Thanks for pointing this out.

It was because fees from the transactions in the first test run (that are still in the mempool) self.test_transaction_expiry(DEFAULT_MEMPOOL_EXPIRY) get included in MiniWallet’s utxo set when this runs for the next sub-test self.test_transaction_expiry(CUSTOM_MEMPOOL_EXPIRY) if the first block is generated with self.wallet.generate

Then because of the way MiniWallet sorts UTXOs based on which is largest it means we end up using an expired transaction for the “trigger” transaction, and get a weird error in the 2nd test run, but not the first - which threw me for a loop

in test/functional/mempool_expiry.py:37 in 7a05d47cdf outdated

38+        self.wallet = MiniWallet(node)
39+
40+        # Ensure the mempool starts empty.
41+        node.generate(1)
42+        assert_equal(set(node.getrawmempool()), set())
43+        # Add enough mature utxos to the wallet so that all txs spend confirmed coins.

MarcoFalke commented at 9:20 am on December 9, 2020:

this comment is not true. The “trigger” txs are chained txs. More inputs are needed.

mjdietzx commented at 9:13 pm on December 9, 2020:

That’s a good point, addressed in most recent commit

MarcoFalke commented at 8:02 am on December 10, 2020: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

test: run mempool_expiry.py even with wallet disabled

Test coverage is also extended in this commit to:
Ensure that another transaction in the mempool is not evicted
when other transactions expire. Note: this other transaction does
not have any ancestors in the mempool that expired. Otherwise it
would be evicted from the mempool, and we already test this case.

3b064fcb9d

mjdietzx force-pushed on Dec 10, 2020

mjdietzx commented at 5:39 pm on December 15, 2020: contributor

This has been squashed/rebased and is ready to go

MarcoFalke commented at 1:51 pm on December 16, 2020: member

ACK 3b064fcb9dd3df6c438a440f0fea86e9cf7b5f57

MarcoFalke merged this on Dec 16, 2020

MarcoFalke closed this on Dec 16, 2020

sidhujag referenced this in commit 0432612bae on Dec 17, 2020

PastaPastaPasta referenced this in commit cbcc4edc46 on Jun 27, 2021

PastaPastaPasta referenced this in commit eb6ca5b7c2 on Jun 28, 2021

PastaPastaPasta referenced this in commit 52f6913d35 on Jun 29, 2021

PastaPastaPasta referenced this in commit 87745721bb on Jul 1, 2021

PastaPastaPasta referenced this in commit 0bdb90aaa0 on Jul 1, 2021

DrahtBot locked this on Feb 15, 2022

test: run mempool_expiry.py even with wallet disabled #20276