backport: merge bitcoin#25228, #24941, #25356, #25435, #23017, #25522, #26414, partial bitcoin#24587, #24637, #24623, #25445 (implement better tx capabilities in MiniWallet)

[kwvg]

Additional Information

• Dependency for backport: merge bitcoin#24587, #24637, #24623, #23075, #24817, #25087, #24839, #20456, #26640, #26892, #26280, #26657, #26923, partial bitcoin#19937 (test backports: part 3) #6726

• Since bitcoin#15891 (8ca90f3), we enforce standard transactions on regtest and regrettably, the logic introduced in bitcoin#24637 (*self_transfer*()) had a tendency to create transactions that were non-standard (see below)

  Test failure:

  dash@b90780fda2d1:/src/dash$ ./test/functional/mempool_package_onemore.py
  2025-06-18T17:50:19.492000Z TestFramework (INFO): PRNG seed is: 8344907287561424619
  2025-06-18T17:50:19.492000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_emt8bu6u
  2025-06-18T17:50:19.765000Z TestFramework (ERROR): JSONRPC error
  Traceback (most recent call last):
    File "/src/dash/test/functional/test_framework/test_framework.py", line 162, in main
      self.run_test()
    File "/src/dash/./test/functional/mempool_package_onemore.py", line 41, in run_test
      utxo, utxo2 = self.chain_tx([utxo], num_outputs=2)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/src/dash/./test/functional/mempool_package_onemore.py", line 28, in chain_tx
      return self.wallet.send_self_transfer_multi(
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/src/dash/test/functional/test_framework/wallet.py", line 207, in send_self_transfer_multi
      txid = self.sendrawtransaction(from_node=kwargs['from_node'], tx_hex=tx.serialize().hex())
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/src/dash/test/functional/test_framework/wallet.py", line 269, in sendrawtransaction
      txid = from_node.sendrawtransaction(hexstring=tx_hex, maxfeerate=maxfeerate, **kwargs)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/src/dash/test/functional/test_framework/coverage.py", line 49, in __call__
      return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/src/dash/test/functional/test_framework/authproxy.py", line 143, in __call__
      raise JSONRPCException(response['error'], status)
  test_framework.authproxy.JSONRPCException: bad-txns-nonstandard-inputs (-26)
  2025-06-18T17:50:20.266000Z TestFramework (INFO): Stopping nodes
  

  Two options presented themselves, either temporary relaxing fRequireStandard on regtest or backporting all the changes needed to make *self_transfer*() create standard transactions and then backporting MiniWallet conversions in a separate pull request. The latter approach was taken.

  • The correctness of this pull request can be validated by observing the CI results of dash#6726

• The backport for bitcoin#23017 does not include changes to rpc_blockchain.py as the relevant scan_blocks() call was removed in the partial backport of bitcoin#23371 (081897c).

Breaking Changes

None expected. Affects only functional tests.

Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas (note: N/A)
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation (note: N/A)
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[coderabbitai]

Walkthrough

The changes primarily update the test framework's MiniWallet class to improve UTXO management, transaction creation, and fee handling. New methods for multi-input/output transactions, balance calculation, and standardized UTXO creation were introduced. The send_self_transfer method was refactored to require an explicit from_node parameter, and new multi-transfer methods were added. Several test scripts were updated to remove now-unnecessary parameters such as mempool_valid and from_node in method calls, reflecting the refactored MiniWallet API. Transaction rehashing logic was revised in CSV activation tests, and wallet UTXO rescanning was streamlined in various mempool-related tests. No exported or public entity signatures were removed; some were added or updated for clarity and functionality.

✨ Finishing Touches

• 📝 Generate Docstrings

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Explain this complex logic.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (3)

test/functional/test_framework/wallet.py (2)

114-117: Consider using contextlib.suppress for cleaner exception handling.

The current implementation is correct, but could be more pythonic.

+from contextlib import suppress
 from copy import deepcopy
 from decimal import Decimal
 from enum import Enum

Then apply this change:

-            try:
-                self.get_utxo(txid=spent["txid"], vout=spent["vout"])
-            except StopIteration:
-                pass
+            with suppress(StopIteration):
+                self.get_utxo(txid=spent["txid"], vout=spent["vout"])

---

232-232: Use generator expression for better performance.

-        inputs_value_total = sum([int(COIN * utxo['value']) for utxo in utxos_to_spend])
+        inputs_value_total = sum(int(COIN * utxo['value']) for utxo in utxos_to_spend)

test/functional/mempool_spend_coinbase.py (1)

36-36: Consider using a regular function instead of lambda.

While functionally correct, using a def statement is more pythonic.

-        coinbase_txid = lambda h: self.nodes[0].getblock(self.nodes[0].getblockhash(h))['tx'][0]
+        def coinbase_txid(h):
+            return self.nodes[0].getblock(self.nodes[0].getblockhash(h))['tx'][0]

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ba93745 and cfa906e.

📒 Files selected for processing (7)

• test/functional/feature_csv_activation.py (1 hunks)
• test/functional/mempool_limit.py (2 hunks)
• test/functional/mempool_reorg.py (3 hunks)
• test/functional/mempool_spend_coinbase.py (1 hunks)
• test/functional/mempool_unbroadcast.py (1 hunks)
• test/functional/test_framework/util.py (1 hunks)
• test/functional/test_framework/wallet.py (6 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

🪛 Ruff (0.11.9)

test/functional/mempool_spend_coinbase.py

36-36: Do not assign a lambda expression, use a def

Rewrite coinbase_txid as a def

(E731)

test/functional/test_framework/wallet.py

114-117: Use contextlib.suppress(StopIteration) instead of try-except-pass

Replace with contextlib.suppress(StopIteration)

(SIM105)

---

251-251: Do not assert False (python -O removes these calls), raise AssertionError()

Replace assert False

(B011)

---

277-277: Do not assert False (python -O removes these calls), raise AssertionError()

Replace assert False

(B011)

🪛 Pylint (3.3.7)

test/functional/test_framework/wallet.py

[refactor] 212-212: Too many arguments (7/5)

(R0913)

---

[refactor] 232-232: Consider using a generator instead 'sum(int(COIN * utxo['value']) for utxo in utxos_to_spend)'

(R1728)

---

[refactor] 266-266: Too many arguments (6/5)

(R0913)

⏰ Context from checks skipped due to timeout of 90000ms (1)

• GitHub Check: linux64_multiprocess-test / Test source

🔇 Additional comments (13)

test/functional/test_framework/wallet.py (4)

93-95: Good abstraction for UTXO creation.

The helper method ensures consistent UTXO structure throughout the class.

---

136-136: Critical fix: rehashing after signing.

Correctly updates the transaction hash after adding the signature.

---

139-149: Excellent improvement to UTXO consistency.

The automatic rescan_utxos call ensures the wallet's UTXO set remains synchronized with the blockchain state after block generation.

---

181-186: Clean separation of concerns.

The refactoring properly separates transaction creation from broadcasting by requiring from_node only where needed.

test/functional/feature_csv_activation.py (1)

123-123: Correct placement of rehash calls.

Adding tx.rehash() after scriptSig modifications is necessary since these changes occur after signing. The removal of rehash calls after sign_tx is appropriate since it's now handled internally.

Also applies to: 131-131

test/functional/mempool_unbroadcast.py (1)

43-43: Correct API usage after MiniWallet refactoring.

The removal of from_node parameter aligns with the updated create_self_transfer signature.

test/functional/mempool_spend_coinbase.py (1)

31-31: Essential UTXO rescan after blockchain modification.

Correctly refreshes the wallet's UTXO set after invalidating blocks.

test/functional/test_framework/util.py (1)

645-645: Correct API usage after MiniWallet refactoring.

The simplified call without from_node and mempool_valid parameters aligns with the updated MiniWallet interface.

test/functional/mempool_limit.py (2)

34-34: LGTM: Parameter removal aligns with updated MiniWallet API.

The removal of mempool_valid=False is consistent with the MiniWallet API refactoring mentioned in the PR objectives. The transaction creation logic remains intact.

---

88-88: LGTM: Consistent API update maintains test functionality.

The removal of mempool_valid=False parameter aligns with the updated MiniWallet API while preserving the test's logic to verify that transactions with insufficient fees are rejected with the expected error.

test/functional/mempool_reorg.py (3)

34-34: LGTM: Improved UTXO scanning approach.

Replacing manual block scanning with wallet.rescan_utxos() is a more reliable and standardized approach for UTXO discovery, leveraging the enhanced capabilities introduced in the MiniWallet class.

---

56-56: LGTM: API parameter removal is consistent.

The removal of mempool_valid=False aligns with the MiniWallet API refactoring while maintaining the timelock transaction creation logic.

---

71-72: LGTM: Enhanced UTXO reference structure.

Using the ["new_utxo"] field from previous transaction results represents an improvement in UTXO management, providing more structured and reliable access to created UTXOs compared to manual transaction ID lookups.

[coderabbitai]

🛠️ Refactor suggestion

Replace assert False with explicit exception.

         else:
-            assert False
+            raise AssertionError("Invalid wallet mode")

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	assert False
	else:
	- assert False
	+ raise AssertionError("Invalid wallet mode")

🧰 Tools

🪛 Ruff (0.11.9)

277-277: Do not assert False (python -O removes these calls), raise AssertionError()

Replace assert False

(B011)

🤖 Prompt for AI Agents

In test/functional/test_framework/wallet.py at line 277, replace the generic
`assert False` statement with an explicit exception that clearly indicates the
error or unexpected condition. Use a specific exception type and provide a
descriptive error message to improve code clarity and debugging.

[coderabbitai]

🛠️ Refactor suggestion

Replace assert False with explicit exception.

Using assert False can be removed by Python optimization.

-        else:
-            assert False
+        else:
+            raise AssertionError("Invalid wallet mode")

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	assert False
	else:
	raise AssertionError("Invalid wallet mode")

🧰 Tools

🪛 Ruff (0.11.9)

251-251: Do not assert False (python -O removes these calls), raise AssertionError()

Replace assert False

(B011)

🤖 Prompt for AI Agents

In test/functional/test_framework/wallet.py at line 251, replace the use of
`assert False` with raising an explicit exception such as `raise RuntimeError`
or a more specific exception type. This ensures the failure is not bypassed by
Python optimizations that remove assert statements.

[PastaPastaPasta]

utACK cfa906e

[UdjinM6]

utACK cfa906e