Skip to content

Conversation

@DashCoreAutoGuix
Copy link
Owner

@DashCoreAutoGuix DashCoreAutoGuix commented Dec 6, 2025

Backports bitcoin#26349

Original commit: 551c8e9

Summary

  • Makes the address field optional in listtransactions and listsinceblock RPC responses
  • Adds tests to verify OP_RETURN outputs (which have no address) are displayed correctly

Changes

  • Updated RPC documentation to mark address field as optional with explanation
  • Added test_op_return() test to both wallet_listsinceblock.py and wallet_listtransactions.py

🤖 Generated with Claude Code

Summary by CodeRabbit

  • API Changes

    • The "address" field in listtransactions and listsinceblock RPC responses is now optional and will be omitted for outputs lacking an address, such as OP_RETURN data.
  • Tests

    • Added tests validating OP_RETURN transaction handling for both RPC methods.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 6, 2025

Walkthrough

Updated RPC methods listtransactions and listsinceblock to mark the "address" field as optional with updated descriptions indicating it is omitted for OP_RETURN outputs. Added corresponding functional tests to verify OP_RETURN transactions correctly exclude the address field from RPC results.

Changes

Cohort / File(s) Summary
RPC method modifications
src/wallet/rpc/transactions.cpp
Made the "address" field optional in listtransactions and listsinceblock RPC methods. Updated field descriptions to indicate the address is not returned when the output lacks an address (e.g., OP_RETURN null data).
Functional test coverage
test/functional/wallet_listtransactions.py, test/functional/wallet_listsinceblock.py
Added test_op_return() methods to both test classes. Each test constructs an OP_RETURN transaction, broadcasts it, and verifies that the corresponding RPC output correctly omits the address field.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Verify the /*optional=*/true syntax is correctly applied in both RPC method definitions
  • Confirm the test assertions properly validate the absence of the address field in OP_RETURN transaction entries
  • Check that updated field descriptions accurately reflect the new behavior

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: making the address field optional in listtransactions and listsinceblock RPC responses, which is the primary modification across all changed files.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch backport-0.25-batch-480-pr-26349

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9cf919c and c1087d9.

📒 Files selected for processing (3)
  • src/wallet/rpc/transactions.cpp (2 hunks)
  • test/functional/wallet_listsinceblock.py (2 hunks)
  • test/functional/wallet_listtransactions.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
test/functional/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Functional tests should be placed in test/functional/ and written in Python

Files:

  • test/functional/wallet_listtransactions.py
  • test/functional/wallet_listsinceblock.py
**

⚙️ CodeRabbit configuration file

**: # CodeRabbit AI Review Instructions for Dash Backports

Your Role

You are reviewing Bitcoin Core backports to Dash Core. Your ONLY job is to validate that the Dash commit faithfully represents the original Bitcoin commit with minimal, necessary adaptations.

Critical Validation Rules

1. File Operations Must Match (AUTO-REJECT if violated)

  • If Bitcoin modifies an existing file → Dash MUST modify (not create new)
  • If Bitcoin creates a new file → Dash creates
  • If Bitcoin deletes a file → Dash deletes
  • Common failure: Bitcoin modifies keys.txt, Dash creates new file with 58 keys

2. Size Ratio Check (80-150% of Bitcoin)

  • Count functional lines changed (exclude comments/whitespace)
  • Dash changes should be 80-150% of Bitcoin's size
  • Red flag: 2-line Bitcoin fix becoming 150+ lines in Dash

3. No Scope Creep

  • Reject if you see: "TODO:", "FIXME:", "while we're here", "also fix"
  • No unrelated refactoring or style changes
  • Only Bitcoin's intended changes + minimal Dash adaptations

4. Bitcoin-Specific Code Detection

  • Auto-reject witness/segwit code: msg_wtxidrelay, MSG_WTX, witness imports
  • Auto-reject RBF (replace-by-fee) functionality
  • Note: PSBT is supported in Dash (don't flag)

5. Mandatory Adaptations Only

  • bitcoindash in strings/paths
  • BitcoinDash in user-facing text
  • Port numbers: 8332→9998 (RPC), 8333→9999 (P2P)
  • Hardcoded test values specific to Dash
  • No other changes unless absolutely required

6. Completeness Check

  • All files changed in Bitcoin must be present
  • Extra files need clear justification (Dash-specific compatibility)
  • Missing files = incomplete backport

Review Process

  1. First: Check file operations match exactly
  2. Second: Calculate size ratio
  3. Third: Scan for scope creep patterns
  4. Fourth: Detect Bitcoin-specific code
  5. Fifth: Verify all changes are minimal adaptations

Output Format

VALIDATION: [PASS/FAIL]

File Operatio...

Files:

  • test/functional/wallet_listtransactions.py
  • test/functional/wallet_listsinceblock.py
  • src/wallet/rpc/transactions.cpp
src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

C++20 codebase should be placed under src/

Files:

  • src/wallet/rpc/transactions.cpp
🧠 Learnings (5)
📓 Common learnings
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: In Dash backports from Bitcoin Core test files, the `address_to_scriptpubkey` function should be imported from `test_framework.wallet` instead of `test_framework.address` as the import location differs between Bitcoin and Dash test frameworks.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T21:29:32.827Z
Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T19:54:21.426Z
Learning: In Dash backports from Bitcoin Core, including necessary compilation fixes (such as API compatibility changes like UniValue get_int() → getInt<int>()) alongside the core backport is standard and expected practice. These compatibility fixes ensure the backported code compiles in Dash's evolved codebase while preserving Bitcoin's original functionality and intent.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T17:13:35.087Z
Learning: When backporting Bitcoin Core commits that use Python's textwrap.dedent() function in test files, the textwrap import statement needs to be explicitly added if it's missing in the Dash test file.
📚 Learning: 2025-07-30T14:45:15.700Z
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: The `add_wallet_options` method is not available in Dash's test framework and should be removed when backporting Bitcoin Core tests that use this method, as demonstrated in wallet_rescan_unconfirmed.py.

Applied to files:

  • test/functional/wallet_listtransactions.py
  • test/functional/wallet_listsinceblock.py
📚 Learning: 2025-07-30T14:45:15.700Z
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: In Dash backports from Bitcoin Core test files, the `address_to_scriptpubkey` function should be imported from `test_framework.wallet` instead of `test_framework.address` as the import location differs between Bitcoin and Dash test frameworks.

Applied to files:

  • test/functional/wallet_listtransactions.py
  • test/functional/wallet_listsinceblock.py
  • src/wallet/rpc/transactions.cpp
📚 Learning: 2025-07-29T22:13:54.797Z
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T22:13:54.797Z
Learning: In Dash backports from Bitcoin Core, witness transaction-related code (MSG_WTX, wtxid) should be replaced with regular transaction handling (MSG_TX, txid) for compatibility, as demonstrated in the p2p_filter.py test fix where MSG_WTX was replaced with MSG_TX and irr_wtxid usage was replaced with irr_txid.

Applied to files:

  • src/wallet/rpc/transactions.cpp
📚 Learning: 2025-07-28T20:34:29.061Z
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.

Applied to files:

  • src/wallet/rpc/transactions.cpp
🧬 Code graph analysis (1)
test/functional/wallet_listtransactions.py (2)
test/functional/wallet_listsinceblock.py (1)
  • test_op_return (403-414)
src/wallet/rpc/transactions.cpp (2)
  • listtransactions (442-551)
  • listtransactions (442-442)
🪛 Ruff (0.14.7)
test/functional/wallet_listtransactions.py

109-109: Prefer next(...) over single element slice

Replace with next(...)

(RUF015)

test/functional/wallet_listsinceblock.py

412-412: Prefer next(...) over single element slice

Replace with next(...)

(RUF015)

🔇 Additional comments (3)
src/wallet/rpc/transactions.cpp (1)

457-461: Address field marked optional exactly as in upstream Bitcoin commit

The only changes here are to the RPC result schema docs for listtransactions and listsinceblock: the address field is now explicitly marked optional and its help text notes omission for outputs without an address (e.g. OP_RETURN data). This matches Bitcoin commit 551c8e9 one-for-one, with the expected branding change to “Dash address”, and does not alter runtime behavior (which already omitted the field in these cases).

Also applies to: 571-575

test/functional/wallet_listsinceblock.py (1)

44-45: OP_RETURN listsinceblock test is a faithful backport and correctly targets the new optional address behavior

run_test now invokes test_op_return, and the new test creates an OP_RETURN-only transaction, then verifies that its listsinceblock entry lacks an address field. This matches the upstream Bitcoin test exactly and aligns with the updated RPC docs that mark address as optional when no address exists. No extra logic or Dash-specific behavior is introduced beyond the upstream change.

Also applies to: 403-415

test/functional/wallet_listtransactions.py (1)

100-101: OP_RETURN listtransactions test mirrors upstream and validates optional address field behavior

The added test_op_return and its invocation in run_test are a direct backport of Bitcoin’s test: they generate an OP_RETURN transaction and assert that its listtransactions entry does not contain an address field. This cleanly exercises the newly documented optional address result without introducing any Dash-specific deviations or extra behavior.

Also applies to: 102-112


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

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants