test: Refactor rpc_getblockstats.py to use MiniWallet #33184

enirox001 · 2025-08-13T14:52:22Z

Refactors rpc_getblockstats.py to use MiniWallet for transaction creation, removing legacy wallet dependencies and simplifying the test by eliminating data caching.

Fixes #31838

DrahtBot · 2025-08-13T14:52:27Z

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	Eunovo
Stale ACK	kannapoix

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33702 (contrib: Remove brittle, confusing and redundant UTF8 encoding from Python IO by maflcko)
#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.

test/functional/rpc_getblockstats.py

kannapoix

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!

test/functional/rpc_getblockstats.py

kannapoix · 2025-08-25T07:51:18Z

test/functional/rpc_getblockstats.py

+        external_address = self.nodes[0].get_deterministic_priv_key().address
+        external_script = address_to_scriptpubkey(external_address)
+
+        for i, (amount_btc, _) in enumerate(TRANSACTIONS):
+            amount_satoshis = int(amount_btc * COIN)
+            wallet.send_to(
+                from_node=self.nodes[0],
+                scriptPubKey=external_script,
+                amount=amount_satoshis,
+            )
+            if i == 0:
+                self.generate(wallet, 1)


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.

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.

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.

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.

kannapoix · 2025-08-26T00:34:32Z

test/functional/rpc_getblockstats.py

+                scriptPubKey=external_script,
+                amount=amount_satoshis,
+            )
+            if i == 0:


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:

bitcoin/test/functional/rpc_getblockstats.py

Lines 53 to 54 in 6ca6f3b

self.nodes[0].sendtoaddress(address=address, amount=10, subtractfeefromamount=True)

self.generate(self.nodes[0], 1)

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.

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().

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.

test/functional/rpc_getblockstats.py

DrahtBot · 2025-08-26T17:41:08Z

🚧 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.}

Hints

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.

kannapoix · 2025-08-28T04:52:38Z

Concept ACK

test/functional/rpc_getblockstats.py

achow101 · 2025-09-17T22:49:18Z

because it used legacy wallet operations

That's not why it fails, it does not use anything specific to legacy wallets at all. It fails initially because test nodes are started with -disablewallet if self.uses_wallet == False, and then fails again because of multiwallet, and lastly because settxfee is deprecated. But none of these are because of legacy wallet.

Ultimately, the issues stem from the fact that this code path is pretty much never executed, and if MiniWallet were to change in the future, we'd run into the same problem.

enirox001 · 2025-09-17T23:13:09Z

because it used legacy wallet operations

That's not why it fails, it does not use anything specific to legacy wallets at all. It fails initially because test nodes are started with -disablewallet if self.uses_wallet == False, and then fails again because of multiwallet, and lastly because settxfee is deprecated. But none of these are because of legacy wallet.

Ultimately, the issues stem from the fact that this code path is pretty much never executed, and if MiniWallet were to change in the future, we'd run into the same problem.

Thanks for the review @achow101

Originally assumed MiniWallet was the culprit based on the issue comments, but your explanation makes sense: the fixes here does not seem to be a proper solution to the problem.

However, I believe the MiniWallet refactor is still the right approach here. The fundamental issue is that the --gen-test-data code path uses outdated testing patterns that are prone to breaking with framework changes. By modernizing to MiniWallet, we ensure the test follows current best practices and stays compatible with future framework updates.

The alternative would be to just add self.add_wallet_options(parser) and replace deprecated RPCs, but that leaves the test using legacy patterns that could break again. What do you think would be the most effective long-term solution?

enirox001 · 2025-09-18T19:22:56Z

Looked into this @achow101 and you're right about the setup issues - they weren't legacy wallet specific but rather configuration problems.
I've addressed the three issues you identified:

Added self.uses_wallet = True to prevent -disablewallet
MiniWallet naturally avoids deprecated settxfee by using modern fee handling
MiniWallet sidesteps multiwallet complexity entirely

Regarding your concern about the --gen-test-data path being rarely executed: I've ensured the test runs with proper wallet configuration, making it more likely to be executed in CI and catching issues earlier.

While this PR may seem like overkill for the specific bug, MiniWallet does provide better maintainability and avoids the deprecated RPC patterns you mentioned. The changes are minimal and focused on the core issues.

maflcko · 2025-09-25T12:58:01Z

they weren't legacy wallet specific but rather configuration problems.

You'll have to adjust the pull description. Also, CI fails.

To ensure this is tested once, you can add --gen-test-data to one CI task config via TEST_RUNNER_EXTRA.

Eunovo

I think this test could be simplified to always generate data with the MiniWallet and test with generated data, instead of dumping it to a file and loading it later. I don't see the advantage of "generate" and "load" paths, and it's creating unnecessary complexity.

test/functional/rpc_getblockstats.py

test/functional/test_framework/test_framework.py

maflcko · 2025-09-30T06:35:16Z

I don't see the advantage of "generate" and "load" paths, and it's creating unnecessary complexity.

I guess it depends on how long it takes to generate, but if it is just a few seconds, it seems easier not to cache.

test/functional/rpc_getblockstats.py

enirox001 · 2025-10-29T17:09:29Z

In commit ac7f694

Fixed the no-op issue raised by @maflcko and @Eunovo. The test was calling getblockstats twice and comparing results against itself. Now it calls getblockstats once and validates consistency

test/functional/rpc_getblockstats.py

brunoerg · 2025-10-30T20:35:36Z

I was reading the code without having seen the discussions here, only based on PR title and description. With some context from the discussions, I think both of them should be updated to reflect what this PR is really doing. It was asked before (see #33184 (comment)) but maybe it still needs improvement.

- Replace legacy wallet usage with MiniWallet for better test isolation. - Keep mocktime to ensure deterministic block timestamps in stats, preventing CI failures due to varying 'time' fields in getblockstats. - Add logging for generated block data to aid debugging. - Fix test logic to avoid no-op conditions

enirox001 · 2025-11-03T15:57:55Z

With some context from the discussions, I think both of them should be updated to reflect what this PR is really doing.

I have updated the PR title and description based on the conversations made to better depict what the PR is doing @brunoerg .

enirox001 · 2025-11-03T16:02:36Z

In commit b89afc6

Changed the title and description of the PR to better depict the purpose of PR
Simplified the transaction data structure by replacing tuples with a list of amounts
Fixed a redundant assertion to properly validate key consistency across stats

kannapoix · 2025-11-27T08:20:16Z

test/functional/rpc_getblockstats.py

+            if i == 0:
+                self.generate(wallet, 1)

+        self.sync_all()


Do we need sync_all here? self.generate() already performs synchronization, and this test uses a single node. What are we syncing at this point?

kannapoix · 2025-11-27T08:31:15Z

test/functional/rpc_getblockstats.py

+        # Make sure all stats have the same set of keys
+        expected_keys = set(stats[0].keys())
+        for stat in stats[1:]:
+            assert_equal(set(stat.keys()), expected_keys)


Testing that all stats share the same set of keys is different from testing that the required keys are present. To preserve the original test’s intent, should we hard-code the expected keys? Other tests follow this approach as well :

bitcoin/test/functional/rpc_blockchain.py

Line 135 in 85d058d

def _test_getblockchaininfo(self):

kannapoix · 2025-11-27T09:09:55Z

test/functional/rpc_getblockstats.py


        for i in range(self.max_stat_pos+1):
            self.log.info('Checking block %d' % (i))
-            assert_equal(stats[i], self.expected_stats[i])


Doesn’t removing these assertions degrade the test? The original test compared RPC results against predefined expected stats.

kannapoix · 2025-11-27T09:14:46Z

test/functional/rpc_getblockstats.py

+            blockhash = stats[i]['blockhash']
            stats_by_hash = self.nodes[0].getblockstats(hash_or_height=blockhash)
-            assert_equal(stats_by_hash, self.expected_stats[i])
+            assert_equal(stats_by_hash, stats[i])


I think this also have a similar problem I pointed out before: https://github.com/bitcoin/bitcoin/pull/33184/files#r2567593821
This changes the test from testing against predefined expected data to cross-checking one getblockstats call with another (by hash). That’s a different test which may degrade the test.

kannapoix · 2025-11-27T09:16:29Z

test/functional/rpc_getblockstats.py


 if __name__ == '__main__':
    GetblockstatsTest(__file__).main()
+


Maybe these are added by mistake? Same lines are already exist above.

DrahtBot · 2025-12-03T10:28:35Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the Tests label Aug 13, 2025

DrahtBot mentioned this pull request Aug 14, 2025

coins,refactor: Reduce getblockstats RPC UTXO overhead estimation #31449

Open

brunoerg reviewed Aug 21, 2025

View reviewed changes

test/functional/rpc_getblockstats.py Outdated Show resolved Hide resolved

brunoerg reviewed Aug 21, 2025

View reviewed changes

test/functional/rpc_getblockstats.py Outdated Show resolved Hide resolved

enirox001 force-pushed the 2025_08_refactor_getblockstats branch from 545aafb to 10da636 Compare August 22, 2025 16:30

kannapoix reviewed Aug 26, 2025

View reviewed changes

enirox001 force-pushed the 2025_08_refactor_getblockstats branch from 16ca1aa to 3321cad Compare August 26, 2025 17:40

DrahtBot added the CI failed label Aug 26, 2025

enirox001 closed this Aug 26, 2025

enirox001 deleted the 2025_08_refactor_getblockstats branch August 26, 2025 17:45

enirox001 restored the 2025_08_refactor_getblockstats branch August 26, 2025 17:45

enirox001 reopened this Aug 26, 2025

enirox001 force-pushed the 2025_08_refactor_getblockstats branch from 3321cad to 5544dae Compare August 26, 2025 17:52

DrahtBot mentioned this pull request Aug 27, 2025

wallet, rpc: remove settxfee and paytxfee #32138

Open

maflcko reviewed Aug 28, 2025

View reviewed changes

test/functional/rpc_getblockstats.py Outdated Show resolved Hide resolved

enirox001 force-pushed the 2025_08_refactor_getblockstats branch 2 times, most recently from 0d06794 to 2e1836d Compare August 28, 2025 19:43

DrahtBot removed the CI failed label Aug 28, 2025

enirox001 changed the title ~~test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py~~ test: Fix rpc_getblockstats for no-wallet environments Sep 29, 2025

enirox001 force-pushed the 2025_08_refactor_getblockstats branch 2 times, most recently from 09df27a to 316cf41 Compare September 29, 2025 20:52

Eunovo reviewed Sep 30, 2025

View reviewed changes

test/functional/rpc_getblockstats.py Outdated Show resolved Hide resolved

test/functional/test_framework/test_framework.py Outdated Show resolved Hide resolved

maflcko reviewed Oct 25, 2025

View reviewed changes

test/functional/rpc_getblockstats.py Outdated Show resolved Hide resolved

enirox001 force-pushed the 2025_08_refactor_getblockstats branch 15 times, most recently from a367a89 to ac7f694 Compare October 29, 2025 17:01

enirox001 force-pushed the 2025_08_refactor_getblockstats branch 2 times, most recently from 863da27 to aeb18a1 Compare October 29, 2025 19:19

brunoerg reviewed Oct 30, 2025

View reviewed changes

test/functional/rpc_getblockstats.py Outdated Show resolved Hide resolved

brunoerg reviewed Oct 30, 2025

View reviewed changes

test/functional/rpc_getblockstats.py Outdated Show resolved Hide resolved

enirox001 changed the title ~~test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py~~ test: Refactor rpc_getblockstats.py to use MiniWallet Nov 3, 2025

enirox001 force-pushed the 2025_08_refactor_getblockstats branch from aeb18a1 to b89afc6 Compare November 3, 2025 15:55

kannapoix reviewed Nov 27, 2025

View reviewed changes

DrahtBot added the Needs rebase label Dec 3, 2025

	self.nodes[0].sendtoaddress(address=address, amount=10, subtractfeefromamount=True)
	self.generate(self.nodes[0], 1)


		if __name__ == '__main__':
		GetblockstatsTest(__file__).main()

test: Refactor rpc_getblockstats.py to use MiniWallet #33184

Are you sure you want to change the base?

test: Refactor rpc_getblockstats.py to use MiniWallet #33184

Conversation

enirox001 commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

kannapoix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Aug 26, 2025

Uh oh!

kannapoix commented Aug 28, 2025

Uh oh!

Uh oh!

achow101 commented Sep 17, 2025

Uh oh!

enirox001 commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enirox001 commented Sep 18, 2025

Uh oh!

maflcko commented Sep 25, 2025

Uh oh!

Eunovo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Sep 30, 2025

Uh oh!

Uh oh!

enirox001 commented Oct 29, 2025

Uh oh!

Uh oh!

Uh oh!

brunoerg commented Oct 30, 2025

Uh oh!

enirox001 commented Nov 3, 2025

Uh oh!

enirox001 commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Dec 3, 2025

enirox001 commented Aug 13, 2025 •

edited

Loading

DrahtBot commented Aug 13, 2025 •

edited

Loading

enirox001 commented Sep 17, 2025 •

edited

Loading

enirox001 commented Nov 3, 2025 •

edited

Loading