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

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

Force-pushed addressing @MarcoFalke's suggestion. Also, just updated the description.

I assume you meant immature coinbases, rather than unconfirmed ones. Which tests currently rely on those? Can't really imagine a case where an unspendable UTXO would be of any use (only if mining of new blocks happen between fetching the UTXO and actually spending them, but then we can probably change those tests to fetch the UTXO later?).

One of the examples I found:

In p2p_segwit.py:

diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py
index b0900e49b..c502bac8c 100755
--- a/test/functional/p2p_segwit.py
+++ b/test/functional/p2p_segwit.py
@@ -2006,6 +2006,7 @@ class SegWitTest(BitcoinTestFramework):
             def serialize(self):
                 return serialize_with_bogus_witness(self.tx)
 
+        self.log.info(self.wallet.get_utxos(include_immature_coinbase=True))
         tx = self.wallet.create_self_transfer()['tx']
         assert_raises_rpc_error(-22, "TX decode failed", self.nodes[0].decoderawtransaction, hexstring=serialize_with_bogus_witness(tx).hex(), iswitness=True)
         with self.nodes[0].assert_debug_log(['Unknown transaction optional data']):

It will print a lot of utxos, but if you set include_immature_coinbase=False, it will return an empty list.

Force-pushed changing the approach.

The logic:

• if you don't specify a txid, it will return the largest utxo desconsidering immature coinbase. In case the only ones available are immature ones, it will return the largest of them.
  Why? - We can't filter the immature coinbase ones by default because in some tests they're the only available ones.

• Added a flag avoid_immature_coinbase to not break some mempool tests, in that case, they need to fetch it considering the immature ones even having other ("normal") utxos.

The filtering logic looks very nice and clean now! But, so, are there only those two mempool tests that need immature spends? It looks like you had to add lots of argument-forwarding to get there. I'm wondering if you've looked into the possibility of just adding more block confirmations in those two tests? And then you perhaps wouldn't need avoid_immature_coinbase at all which I feel should just be an implied default anyway.

@brunoerg are you following up here?

@brunoerg are you following up here?

Yes.

Concept ACK

Regarding the approach, I think we should always apply the filter or allow the argument but have it default to True and only let you change it to false if you are calling get_utxo directly (no argument forwarding).

I'm also a little confused by the mempool_package_limit.py, because it looks like the test assumes it is already working with mature UTXOs:

https://github.com/bitcoin/bitcoin/blob/master/test/functional/mempool_package_limits.py#L26-L29

Using immature UTXOs in the mempool test feels like a hack, where we could just add more confirmations or figure out why the test is failing when there are supposed to already be mature UTXOs

EDIT: if you do end up needing to touch mempool_package_limits.py with a fix, I'd also suggest adding it as its own commit

I'm gonna change the approach and touch mempool_package_limits.py.

Thanks everyone for reviewing it! Here's what have changed:

• get_utxo will not return immature coinbase utxos anymore for any case.
• We won't use the confirmation count from utxo object since it may have outdated. We will use current block height + utxo's height for it.
• changed get_utxos to use current block height to compute the confirmation count instead of using the value from utxo object, same logic from get_utxo.

Looks like the tests fail in CI, even on a re-run?

Looks like the tests fail in CI, even on a re-run?

Yes, needs to fix mempool_compatibility.

Force-pushed:

• Fixed mempool_compatibility to stop node1 after calling send_self_transfer, not before:

--- a/test/functional/mempool_compatibility.py
+++ b/test/functional/mempool_compatibility.py
@@ -47,12 +47,12 @@ class MempoolCompatibilityTest(BitcoinTestFramework):
         # unbroadcasted_tx won't pass old_node's `MemPoolAccept::PreChecks`.
         self.connect_nodes(0, 1)
         self.sync_blocks()
-        self.stop_node(1)
 
         self.log.info("Add a transaction to mempool on old node and shutdown")
         old_tx_hash = new_wallet.send_self_transfer(from_node=old_node)["txid"]
         assert old_tx_hash in old_node.getrawmempool()
         self.stop_node(0)
+        self.stop_node(1)

Before I re-ACK, are you gonna squash any commits?

Before I re-ACK, are you gonna squash any commits?

Done!

ACK 272eb55