Merge bitcoin/bitcoin#27224: refactor: Remove CAddressBookData::destdata

[DashCoreAutoGuix]

Backport of Bitcoin PR bitcoin#27224

This is a backport of Bitcoin Core PR bitcoin#27224

Original PR Description

This is cleanup that doesn't change external behavior. Benefits of the cleanup are:

• Removes awkward StringMap intermediate representation for wallet address metadata.
• Simplifies CWallet, deals with used address and received request serialization in walletdb.cpp instead of higher level wallet code
• Adds test coverage and documentation

This PR doesn't change externally observable behavior. Internally, the only change in behavior is that EraseDestData deletes rows directly from the database because they are no longer stored in memory. This is more direct and efficient because it uses a single lookup and scan instead of multiple lookups.

Dash-Specific Adaptations

1. DataStream vs CDataStream: Bitcoin migrated from CDataStream to DataStream in a separate PR. Dash still uses CDataStream, so all method signatures have been adapted accordingly.

2. ErasePrefix Implementation: Added the ErasePrefix method to DatabaseBatch interface and implemented it in:

   • DummyBatch: Returns true (no-op)
   • BerkeleyBatch: Uses cursor-based deletion with transaction support
   • SQLiteBatch: Uses SQL DELETE FROM main WHERE instr(key, ?) = 1 query

3. Address Reuse Tracking: Adapted IsAddressUsed to IsAddressPreviouslySpent without witness address types since Dash doesn't support SegWit.

4. DESTDATA Handling: Updated DESTDATA deserialization to parse:

   • "used" key for previously_spent flag
   • "rr##" keys for receive requests

5. HDCHAIN Handling: Preserved Dash's CRYPTED_HDCHAIN variant alongside HDCHAIN.

Changes Summary

• Replaced generic CAddressBookData::destdata map with specific fields:

  • previously_spent: bool flag for address reuse tracking
  • receive_requests: map for payment requests

• Added DatabaseBatch::ErasePrefix() for efficient deletion of key ranges

• Refactored wallet database operations to handle address metadata more directly

• Updated wallet loading logic to properly deserialize new address data format

Bitcoin commit: 5325a61

Summary by CodeRabbit

• New Features

  • Per-address receive requests: create, persist, and delete requests, with proper loading across wallet formats.
  • Replaces “Used” with “Previously spent” for addresses, improving avoid-reuse behavior and spent detection.
  • Faster cleanup when removing all data tied to an address.

• Bug Fixes

  • Deleting an empty receive request from the UI now correctly erases it.

• Tests

  • Added cross-backend tests for loading, writing, and erasing receive requests and previously-spent flags.

[coderabbitai]

Walkthrough

Adds prefix-deletion support to wallet database batches (new ErasePrefix API) with Berkeley DB and SQLite implementations. Refactors wallet address metadata: replaces “used” with previously_spent, introduces receive_requests with per-id storage, updates read/write/erase paths, adjusts interfaces, and adds tests covering multiple DB backends.

Changes

Cohort / File(s)	Summary of changes
DB batch interface: add prefix erase src/wallet/db.h	Introduces pure virtual DatabaseBatch::ErasePrefix; DummyBatch override returns true.
Berkeley DB batch: implement prefix erase, expose txn src/wallet/bdb.h, src/wallet/bdb.cpp	Adds BerkeleyBatch::ErasePrefix using cursor iteration within a transaction; adds txn() accessor; wires override.
SQLite batch: implement prefix erase and helpers src/wallet/sqlite.h, src/wallet/sqlite.cpp	Adds m_delete_prefix_stmt, ExecStatement helper, ErasePrefix implementation using DELETE with instr(key, ?)=1; integrates into statement setup/close; refactors deletion path.
Wallet core: rename/expand address metadata src/wallet/wallet.h, src/wallet/wallet.cpp	Replaces destdata “used” with CAddressBookData.previously_spent and receive_requests map; adds getters/setters/loaders and erase for receive requests; updates IsSpentKey and related APIs to PreviouslySpent.
Wallet DB API: address-focused read/write/erase src/wallet/walletdb.h, src/wallet/walletdb.cpp	Removes WriteDestData/EraseDestData; adds WriteAddressPreviouslySpent, WriteAddressReceiveRequest, EraseAddressReceiveRequest, EraseAddressData (prefix-based); updates DESTDATA parsing to load “used” and “rr”.
GUI interface glue src/wallet/interfaces.cpp	setAddressReceiveRequest now erases on empty value; otherwise sets; adds explanatory comments.
Tests: multi-backend coverage and failure hooks src/wallet/test/wallet_tests.cpp	Adds LoadReceiveRequests test across formats; simulates prefix erasure in FailBatch via ErasePrefix override; validates write/erase and previously_spent handling.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 22.22% 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 succinctly describes the primary change—removing the CAddressBookData::destdata field during a refactor—and accurately reflects the core intent of the pull request.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch backport-0.25-batch-415-pr-27224

---

📜 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 54e2588 and d943c55.

📒 Files selected for processing (11)

• src/wallet/bdb.cpp (1 hunks)
• src/wallet/bdb.h (2 hunks)
• src/wallet/db.h (2 hunks)
• src/wallet/interfaces.cpp (1 hunks)
• src/wallet/sqlite.cpp (3 hunks)
• src/wallet/sqlite.h (1 hunks)
• src/wallet/test/wallet_tests.cpp (2 hunks)
• src/wallet/wallet.cpp (4 hunks)
• src/wallet/wallet.h (3 hunks)
• src/wallet/walletdb.cpp (2 hunks)
• src/wallet/walletdb.h (2 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

C++20 codebase should be placed under src/

Files:

• src/wallet/interfaces.cpp
• src/wallet/bdb.cpp
• src/wallet/bdb.h
• src/wallet/walletdb.h
• src/wallet/walletdb.cpp
• src/wallet/test/wallet_tests.cpp
• src/wallet/db.h
• src/wallet/sqlite.cpp
• src/wallet/sqlite.h
• src/wallet/wallet.h
• src/wallet/wallet.cpp

**

⚙️ 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

• bitcoin → dash in strings/paths
• Bitcoin → Dash 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:

• src/wallet/interfaces.cpp
• src/wallet/bdb.cpp
• src/wallet/bdb.h
• src/wallet/walletdb.h
• src/wallet/walletdb.cpp
• src/wallet/test/wallet_tests.cpp
• src/wallet/db.h
• src/wallet/sqlite.cpp
• src/wallet/sqlite.h
• src/wallet/wallet.h
• src/wallet/wallet.cpp

src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests

Files:

• src/wallet/test/wallet_tests.cpp

🧠 Learnings (1)

📓 Common learnings

Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#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
PR: DashCoreAutoGuix/dash#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
PR: DashCoreAutoGuix/dash#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.

🧬 Code graph analysis (10)

src/wallet/bdb.cpp (4)

src/wallet/db.h (3)

• ErasePrefix (170-170)
• TxnBegin (179-179)
• TxnCommit (180-180)

src/wallet/sqlite.cpp (6)

• ErasePrefix (498-501)
• ErasePrefix (498-498)
• TxnBegin (555-563)
• TxnBegin (555-555)
• TxnCommit (565-573)
• TxnCommit (565-565)

src/wallet/walletdb.cpp (5)

• prefix (1121-1121)
• TxnBegin (1151-1154)
• TxnBegin (1151-1151)
• TxnCommit (1156-1159)
• TxnCommit (1156-1156)

src/wallet/bdb.h (1)

• TxnBegin (74-81)

src/wallet/bdb.h (4)

src/wallet/bdb.cpp (2)

• ErasePrefix (819-836)
• ErasePrefix (819-819)

src/wallet/db.h (1)

• ErasePrefix (170-170)

src/wallet/sqlite.cpp (2)

• ErasePrefix (498-501)
• ErasePrefix (498-498)

src/wallet/test/wallet_tests.cpp (2)

• prefix (1535-1535)
• prefix (1535-1535)

src/wallet/walletdb.h (4)

src/wallet/walletdb.cpp (8)

• WriteAddressPreviouslySpent (1103-1107)
• WriteAddressPreviouslySpent (1103-1103)
• WriteAddressReceiveRequest (1109-1112)
• WriteAddressReceiveRequest (1109-1109)
• EraseAddressReceiveRequest (1114-1117)
• EraseAddressReceiveRequest (1114-1114)
• EraseAddressData (1119-1124)
• EraseAddressData (1119-1119)

src/wallet/interfaces.cpp (10)

• dest (210-214)
• dest (210-210)
• dest (223-226)
• dest (223-223)
• dest (227-230)
• dest (227-227)
• dest (231-249)
• dest (231-234)
• dest (264-281)
• dest (264-264)

src/wallet/wallet.h (1)

• previously_spent (233-250)

src/wallet/wallet.cpp (2)

• EraseAddressReceiveRequest (2741-2746)
• EraseAddressReceiveRequest (2741-2741)

src/wallet/walletdb.cpp (3)

src/wallet/interfaces.cpp (10)

• dest (210-214)
• dest (210-210)
• dest (223-226)
• dest (223-223)
• dest (227-230)
• dest (227-227)
• dest (231-249)
• dest (231-234)
• dest (264-281)
• dest (264-264)

src/wallet/wallet.h (1)

• previously_spent (233-250)

src/wallet/wallet.cpp (2)

• EraseAddressReceiveRequest (2741-2746)
• EraseAddressReceiveRequest (2741-2741)

src/wallet/test/wallet_tests.cpp (2)

src/node/interfaces.cpp (12)

• name (1090-1093)
• name (1090-1090)
• name (1094-1097)
• name (1094-1094)
• name (1098-1101)
• name (1098-1098)
• name (1102-1111)
• name (1102-1102)
• name (1112-1122)
• name (1112-1112)
• MakeChain (1143-1143)
• MakeChain (1143-1143)

src/wallet/wallet.cpp (2)

• MakeWalletDatabase (2748-2771)
• MakeWalletDatabase (2748-2748)

src/wallet/db.h (3)

src/wallet/bdb.cpp (2)

• ErasePrefix (819-836)
• ErasePrefix (819-819)

src/wallet/sqlite.cpp (2)

• ErasePrefix (498-501)
• ErasePrefix (498-498)

src/wallet/test/wallet_tests.cpp (2)

• prefix (1535-1535)
• prefix (1535-1535)

src/wallet/sqlite.cpp (3)

src/wallet/bdb.cpp (4)

• EraseKey (795-806)
• EraseKey (795-795)
• ErasePrefix (819-836)
• ErasePrefix (819-819)

src/wallet/db.h (2)

• EraseKey (168-168)
• ErasePrefix (170-170)

src/wallet/test/wallet_tests.cpp (10)

• key (1531-1531)
• key (1531-1531)
• key (1532-1532)
• key (1532-1532)
• key (1533-1533)
• key (1533-1533)
• key (1534-1534)
• key (1534-1534)
• prefix (1535-1535)
• prefix (1535-1535)

src/wallet/sqlite.h (1)

src/wallet/sqlite.cpp (8)

• SetupSQLStatements (142-162)
• SetupSQLStatements (142-142)
• ExecStatement (475-491)
• ExecStatement (475-475)
• EraseKey (493-496)
• EraseKey (493-493)
• ErasePrefix (498-501)
• ErasePrefix (498-498)

src/wallet/wallet.h (4)

src/interfaces/wallet.h (1)

• std (146-346)

src/wallet/wallet.cpp (30)

• LoadAddressPreviouslySpent (2707-2710)
• LoadAddressPreviouslySpent (2707-2707)
• LoadAddressReceiveRequest (2712-2715)
• LoadAddressReceiveRequest (2712-2712)
• IsAddressPreviouslySpent (2717-2721)
• IsAddressPreviouslySpent (2717-2717)
• SetAddressPreviouslySpent (2693-2705)
• SetAddressPreviouslySpent (2693-2693)
• batch (533-533)
• batch (798-798)
• batch (922-922)
• batch (1162-1162)
• batch (1241-1241)
• batch (1297-1297)
• batch (1342-1342)
• batch (1354-1354)
• batch (1375-1375)
• batch (1499-1499)
• batch (1644-1644)
• batch (1728-1728)
• batch (1777-1777)
• batch (2223-2223)
• batch (2284-2284)
• batch (2291-2291)
• GetAddressReceiveRequests (2723-2732)
• GetAddressReceiveRequests (2723-2723)
• SetAddressReceiveRequest (2734-2739)
• SetAddressReceiveRequest (2734-2734)
• EraseAddressReceiveRequest (2741-2746)
• EraseAddressReceiveRequest (2741-2741)

src/wallet/interfaces.cpp (10)

• dest (210-214)
• dest (210-210)
• dest (223-226)
• dest (223-223)
• dest (227-230)
• dest (227-227)
• dest (231-249)
• dest (231-234)
• dest (264-281)
• dest (264-264)

src/wallet/walletdb.cpp (2)

• EraseAddressReceiveRequest (1114-1117)
• EraseAddressReceiveRequest (1114-1114)

src/wallet/wallet.cpp (3)

src/wallet/interfaces.cpp (10)

• dest (210-214)
• dest (210-210)
• dest (223-226)
• dest (223-223)
• dest (227-230)
• dest (227-227)
• dest (231-249)
• dest (231-234)
• dest (264-281)
• dest (264-264)

src/wallet/scriptpubkeyman.cpp (3)

• GetAffectedKeys (1638-1648)
• GetAffectedKeys (1638-1638)
• id (1671-1671)

src/wallet/walletdb.cpp (2)

• EraseAddressReceiveRequest (1114-1117)
• EraseAddressReceiveRequest (1114-1114)

🔇 Additional comments (21)

src/wallet/sqlite.h (1)

31-41: SQLite: Prefix‑delete plumbing matches upstream intent; verify finalize and param conversions

• Addition of m_delete_prefix_stmt, ExecStatement helper, and ErasePrefix override is consistent with the new API.
• Please double‑check Close() finalizes m_delete_prefix_stmt to avoid a prepared‑statement leak, and that SetupSQLStatements prepares it on open (looks correct based on sqlite.cpp snippet).
• ExecStatement takes Span; call sites also pass CDataStream (e.g., EraseKey). Ensure implicit conversion (or an overload) exists in this tree, otherwise add a local conversion at call sites.

src/wallet/bdb.h (2)

197-198: BerkeleyBatch::ErasePrefix override added — OK

Matches the new DatabaseBatch contract and pairs with the bdb.cpp implementation. No issues.

---

225-226: Expose active transaction accessor cautiously

txn() const { return activeTxn; } is fine for internal helpers (e.g., cursor helpers). Please confirm no new external users beyond wallet DB internals to avoid expanding the public surface area unnecessarily.

src/wallet/interfaces.cpp (1)

265-281: setAddressReceiveRequest now erases on empty value — matches upstream semantics

Conditional erase vs set aligns with the refactor to per‑address receive‑requests. Comment block mirrors upstream rationale. LGTM.

Please confirm the GUI path actually passes an empty string to erase (so this code deletes, not stores an empty value).

src/wallet/db.h (2)

94-95: API change: DatabaseBatch gains pure virtual ErasePrefix

Good addition; forces concrete backends to implement prefix deletion. Compile‑time enforcement is desired here.

---

170-171: DummyBatch override present

Returning true is consistent with Dummy semantics used by tests.

src/wallet/bdb.cpp (1)

819-836: Berkeley prefix erase logic looks correct and transactional

• Uses DB_SET_RANGE + forward scan, deletes while keys start with prefix, then commits. Returns true on success/DB_NOTFOUND.
• The const_cast commentary is accurate for BDB usage without DB_DBT_USERMEM.
• Note: ErasePrefix starts its own transaction; it will return false if a txn is already active. That matches typical usage here, but keep in mind it cannot be composed into a larger caller‑managed txn.

If composition into an existing WalletBatch transaction is needed anywhere in Dash, consider a fast path when activeTxn != nullptr (not required if upstream behavior is preserved).

src/wallet/sqlite.cpp (5)

149-149: LGTM! SQL statement correctly implements prefix matching.

The SQL DELETE FROM main WHERE instr(key, ?) = 1 correctly deletes rows where the key starts with the provided prefix, as instr() returns 1 when the substring is found at the beginning.

---

408-408: LGTM! Proper cleanup of the new statement.

The m_delete_prefix_stmt is correctly added to the cleanup list in Close(), ensuring proper resource management.

---

475-491: LGTM! Well-designed helper method reduces duplication.

The ExecStatement helper correctly generalizes the bind-execute-cleanup pattern, improving maintainability. The assertion on the statement handle is appropriate for a private helper.

---

493-496: LGTM! EraseKey correctly refactored to use the helper.

The refactoring maintains the original behavior while eliminating code duplication by delegating to ExecStatement.

---

498-501: LGTM! ErasePrefix correctly implemented.

The ErasePrefix method properly delegates to ExecStatement with the prefix-delete statement, maintaining consistency with the EraseKey implementation.

src/wallet/test/wallet_tests.cpp (4)

443-450: LGTM! Multi-format testing array correctly defined.

The DATABASE_FORMATS array correctly uses conditional compilation to include only the database formats that are built, enabling comprehensive testing across available backends.

---

452-465: LGTM! Well-designed test helper for format-specific wallet testing.

The TestLoadWallet helper correctly creates a wallet with the specified database format and invokes the test callback with proper locking, enabling clean multi-format test cases.

---

467-500: LGTM! Comprehensive test of address data management across database formats.

The LoadReceiveRequests test thoroughly exercises the new address data APIs (previously_spent, receive requests, and bulk erase) across all available database backends, ensuring consistent behavior.

---

1535-1535: LGTM! FailBatch correctly implements the new interface method.

The ErasePrefix override in FailBatch maintains consistency with other test double methods, enabling failure scenario testing for prefix-based erasure.

src/wallet/walletdb.cpp (5)

566-584: LGTM! DESTDATA reading correctly refactored to parse specific keys.

The modified DESTDATA reading logic correctly identifies "used" entries as previously_spent flags and "rr##" entries as per-address receive requests, aligning with the new explicit address metadata model.

---

1103-1107: LGTM! WriteAddressPreviouslySpent correctly implements the write-or-erase pattern.

The method correctly writes the "used" DESTDATA entry when previously_spent is true and erases it when false, maintaining consistency with the legacy format.

---

1109-1112: LGTM! WriteAddressReceiveRequest correctly stores per-id receive requests.

The method properly constructs the DESTDATA key with "rr" prefix and id suffix, enabling per-address, per-request storage that matches the read path.

---

1114-1117: LGTM! EraseAddressReceiveRequest correctly erases individual receive requests.

The method properly constructs the same DESTDATA key as the write path and erases the specific receive request entry.

---

1119-1124: LGTM! EraseAddressData correctly implements bulk erasure using prefix deletion.

The method properly constructs a DESTDATA prefix for the destination and uses ErasePrefix to efficiently delete all address-related entries (previously_spent and all receive requests) in a single operation.

---

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

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