test: Replace legacy wallet with MiniWallet in rpc

enirox001 commented at 2:52 pm on August 13, 2025: contributor

The rpc_getblockstats.py test was failing with --gen-test-data because it used legacy wallet operations that could be affected by wallet changes, making the test data inconsistent.

I replaced the legacy wallet with MiniWallet to fix this. This makes the test data generation deterministic and reliable.

Changes:

Switched from wallet_importprivkey/sendtoaddress to MiniWallet.send_to()
Added helper methods for OP_RETURN creation and block retrieval
Made assertions dynamic instead of hardcoded
Updated the test data JSON to match MiniWallet’s SegWit transactions

The test now passes consistently with both --gen-test-data and existing data.

Fixes #31838

DrahtBot added the label Tests on Aug 13, 2025

DrahtBot commented at 2:52 pm on August 13, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33184.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	kannapoix

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32138 (wallet, rpc: remove settxfee and paytxfee by polespinasa)
#31449 (coins,refactor: Reduce getblockstats RPC UTXO overhead estimation by l0rinc)

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.

in test/functional/rpc_getblockstats.py:66 in 545aafb902 outdated

79+        self.generate(wallet, COINBASE_MATURITY + 1)
80+
81+        external_address = self.nodes[0].get_deterministic_priv_key().address
82+        external_script = address_to_scriptpubkey(external_address)
83+
84+        for i, (amount_btc, description) in enumerate(TRANSACTIONS):

brunoerg commented at 6:45 pm on August 21, 2025:

0        for i, (amount_btc, _) in enumerate(TRANSACTIONS):

enirox001 commented at 4:33 pm on August 22, 2025:

Done

in test/functional/rpc_getblockstats.py:49 in 545aafb902 outdated

62-        self.nodes[0].settxfee(amount=0.003)
63-        self.nodes[0].sendtoaddress(address=address, amount=1, subtractfeefromamount=True)
64-        # Send to OP_RETURN output to test its exclusion from statistics
65-        self.nodes[0].send(outputs={"data": "21"})
66+        MOCKTIME = 1525107225
67+        FEE_SATOSHIS = 31200

brunoerg commented at 6:49 pm on August 21, 2025:

545aafb902cfe16194f90f7eb11cd5221b30015e: Do we need this FEE_SATOSHIS? Couldn’t we use the default value from send_to?

enirox001 commented at 4:31 pm on August 22, 2025:

Good catch! Removed FEE_SATOSHIS and now using the default fee. The original settxfee(0.003) was arbitrary - since getblockstats measures UTXOs (not fees), the default fee works fine. Test still passes and code is cleaner.

enirox001 force-pushed on Aug 22, 2025

in test/functional/rpc_getblockstats.py:93 in 10da636b15 outdated

107+        with open(filename, 'w', encoding="utf8") as f:
108+            json.dump(to_dump, f, sort_keys=True, indent=2)
109+
110+    def _create_op_return_transaction(self, wallet):
111+        """Create a transaction with OP_RETURN output to test statistics exclusion."""
112+        tx = wallet.create_self_transfer(fee_rate=Decimal("0.003"))

kannapoix commented at 7:19 am on August 25, 2025:

Is there a reason we need to explicitly specify fee_rate here? From what I can see, the test seems to pass even without setting this value. I understand that in the original test, the transaction with the OP_RETURN was sent with a fee rate of 0.003 implicitly. However, since we are not testing the fee rate in this case, I’m concerned that explicitly setting it here might be confusing for future readers.

enirox001 commented at 5:56 pm on August 26, 2025:

Good catch! You’re right - since create_self_transfer() defaults to fee_rate=0.003, explicitly setting it here is redundant and potentially confusing. I’ve removed the parameter to use the default.

in test/functional/rpc_getblockstats.py:72 in 10da636b15 outdated

86+                from_node=self.nodes[0],
87+                scriptPubKey=external_script,
88+                amount=amount_satoshis,
89+            )
90+            if i == 0:
91+                self.generate(wallet, 1)

kannapoix commented at 7:51 am on August 25, 2025:

If we are not specifically testing address type or scripts here, perhaps we could use wallet.send_self_transfer(from_node=self.nodes) instead? I think this might make the code simpler than generating scripts and using wallet.send_to.

enirox001 commented at 5:57 pm on August 26, 2025:

Good idea, but send_self_transfer() only creates self-transfers (sending back to the wallet’s own address). For this test, we need external outputs to properly test getblockstats UTXO statistics. The send_to() method is necessary here to create transactions with external recipients.

kannapoix commented at 2:37 am on August 28, 2025:

Sorry, I don’t quite understand why specifically external outputs are necessary. In getblockstats, there are four UTXO-related statistics: utxo_increase, utxo_size_inc, utxo_increase_actual, and utxo_size_inc_actual. As far as I can see, none of these depend on whether a UTXO is sent to an external or internal address.

That said, using send_self_transfer would significantly change the generated transactions compared to the original test, so I agree that it’s better to try to keep the generated transactions as close to the original as possible.

enirox001 commented at 7:43 pm on August 28, 2025:

You’re correct that UTXO statistics don’t depend on output destination. However, I’m keeping external outputs to maintain consistency with the original test’s transaction structure. Changing to send_self_transfer would alter the generated test data and expected statistics values.

Since the goal is to replace the wallet implementation while preserving test behavior, send_to with external scripts is the most faithful translation of the original sendtoaddress calls.

in test/functional/rpc_getblockstats.py:71 in 10da636b15 outdated

85+            wallet.send_to(
86+                from_node=self.nodes[0],
87+                scriptPubKey=external_script,
88+                amount=amount_satoshis,
89+            )
90+            if i == 0:

kannapoix commented at 0:34 am on August 26, 2025:

May I ask why we need to mine here after the first transaction? I noticed that the original test also mines at this point, but I’m not sure about the reason behind it: https://github.com/bitcoin/bitcoin/blob/6ca6f3b37b992591726bd13b494369bee3bd6468/test/functional/rpc_getblockstats.py#L53-L54

enirox001 commented at 5:58 pm on August 26, 2025:

This mining pattern is intentional - it creates a specific block structure for testing:

Block 1: 1 transaction (first 10 BTC send)
Block 2: 4 transactions (remaining sends + OP_RETURN)

This allows testing getblockstats across different block scenarios (single vs multi-transaction blocks) rather than having all transactions in one block.

kannapoix commented at 4:39 am on August 28, 2025:

Thank you!

It appears that we are no longer testing the case of send 10 BTC with fee subtracted, nor the case of send 1 BTC with a higher fee rate, since we are not specifying a fee in send_to().

enirox001 commented at 7:43 pm on August 28, 2025:

You’re correct - the comments were misleading. I’ve updated them to accurately reflect that MiniWallet uses consistent fee behavior for all transactions. The test still validates getblockstats functionality across different block structures, which is the primary goal.

in test/functional/rpc_getblockstats.py:96 in 10da636b15 outdated

110+    def _create_op_return_transaction(self, wallet):
111+        """Create a transaction with OP_RETURN output to test statistics exclusion."""
112+        tx = wallet.create_self_transfer(fee_rate=Decimal("0.003"))
113+        op_return_data = b'\x21'
114+        tx["tx"].vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN, op_return_data])))
115+        wallet.sendrawtransaction(from_node=self.nodes[0], tx_hex=tx["hex"])

kannapoix commented at 1:27 am on August 26, 2025:

Just to confirm, it seems we need to re-serialize the transaction after changing the output. Is that correct?

0        wallet.sendrawtransaction(from_node=self.nodes[0], tx_hex=tx["tx"].serialize().hex())

enirox001 commented at 6:00 pm on August 26, 2025:

You’re right - we need to re-serialize after modifying the transaction. The tx["hex"] contains the original transaction, but we’re modifying tx["tx"] by adding the OP_RETURN output. Without re-serializing, the OP_RETURN wouldn’t actually be included in the sent transaction.

I’ve applied your suggested fix.

kannapoix commented at 3:06 am on August 26, 2025: none

I’m new to this code base, so I might have missed something or made some mistakes in my review. Please feel free to correct me if that’s the case. Thanks!

enirox001 force-pushed on Aug 26, 2025

DrahtBot added the label CI failed on Aug 26, 2025

DrahtBot commented at 5:41 pm on August 26, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/48935536189 LLM reason (✨ experimental): Lint check failed due to a style error detected by rust’s ruff, specifically an unused import in a Python test file.

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.

enirox001 closed this on Aug 26, 2025

enirox001 deleted the branch on Aug 26, 2025

enirox001 restored the branch on Aug 26, 2025

enirox001 reopened this on Aug 26, 2025

enirox001 force-pushed on Aug 26, 2025

kannapoix commented at 4:52 am on August 28, 2025: none

Concept ACK

in test/functional/rpc_getblockstats.py:22 in 5544dae670 outdated

18+from test_framework.script import OP_RETURN, CScript
19+from test_framework.messages import CTxOut, COIN
20+from test_framework.address import address_to_scriptpubkey
21 import json
22 import os
23+from decimal import Decimal

maflcko commented at 7:23 am on August 28, 2025:

[17:54:00.577] = help: Remove unused import: decimal.Decimal

enirox001 commented at 7:43 pm on August 28, 2025:

Done

enirox001 force-pushed on Aug 28, 2025

test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py 2e1836d744

enirox001 force-pushed on Aug 28, 2025

DrahtBot removed the label CI failed on Aug 28, 2025

test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py #33184

Code Coverage & Benchmarks

Reviews

Conflicts