Skip to content

backport: auto 20251202#7028

Merged
PastaPastaPasta merged 3 commits intodashpay:developfrom
PastaPastaPasta:backport/auto-20251202
Dec 2, 2025
Merged

backport: auto 20251202#7028
PastaPastaPasta merged 3 commits intodashpay:developfrom
PastaPastaPasta:backport/auto-20251202

Conversation

@PastaPastaPasta
Copy link
Member

@PastaPastaPasta PastaPastaPasta commented Dec 2, 2025

Issue being fixed or feature implemented

batch of automated backports

What was done?

Small batch of auto backports

How Has This Been Tested?

CI

Breaking Changes

Checklist:

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

DashCoreAutoGuix and others added 3 commits December 2, 2025 07:38
Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: fanquake <fanquake@gmail.com>
…, gettransaction and getwalletinfo (dashpay#1182)

Co-authored-by: Andrew Chow <github@achow101.com>
@PastaPastaPasta PastaPastaPasta added this to the 23.1 milestone Dec 2, 2025
@github-actions
Copy link

github-actions bot commented Dec 2, 2025

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link

coderabbitai bot commented Dec 2, 2025

Walkthrough

This pull request introduces a "lastprocessedblock" field containing the last processed block hash and height to three wallet RPC calls (getbalances, gettransaction, getwalletinfo). A new utility function AppendLastProcessedBlock is added to populate this field consistently. Additionally, the systemd service file is updated to use Type=notify instead of Type=forking with corresponding startup and shutdown notification directives, and minor documentation updates are made to protocol.h describing MEMPOOL and NODE_BLOOM flags.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Thread safety validation: The AppendLastProcessedBlock function is annotated with EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet). Verify the locking context at all call sites (getbalances, gettransaction, getwalletinfo) is correct.
  • Systemd service change: The Type=notify transition with NotifyAccess=all and -startupnotify/-shutdownnotify flags alters daemon lifecycle behavior. Confirm this is compatible with existing deployment environments and systemd versions.
  • Test coverage: Verify that the new lastprocessedblock tests in wallet_balance.py and wallet_basic.py adequately cover edge cases (empty wallets, multiple transactions, key imports).
  • RPC response schema consistency: Ensure RESULT_LAST_PROCESSED_BLOCK placement and structure (hash, height fields) is consistent across all three modified RPC calls.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'backport: auto 20251202' is vague and generic, using non-descriptive terms that don't convey meaningful information about the specific changes in the changeset. Consider a more descriptive title that summarizes the main changes, such as 'Add lastprocessedblock field to wallet RPCs' or 'Update systemd service notification and add wallet RPC enhancements'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description mentions 'batch of automated backports' and 'small batch of auto backports' which relates to the nature of the PR but lacks specific details about what changes are included.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 566fd2d and 4e43e6b.

📒 Files selected for processing (10)
  • contrib/init/dashd.service (2 hunks)
  • doc/release-notes-26094.md (1 hunks)
  • src/protocol.h (1 hunks)
  • src/wallet/rpc/coins.cpp (2 hunks)
  • src/wallet/rpc/transactions.cpp (2 hunks)
  • src/wallet/rpc/util.cpp (1 hunks)
  • src/wallet/rpc/util.h (3 hunks)
  • src/wallet/rpc/wallet.cpp (2 hunks)
  • test/functional/wallet_balance.py (3 hunks)
  • test/functional/wallet_basic.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
{guix-build*,releases,**/guix-build*,releases/**,.github/**,depends/**,ci/**,contrib/**,doc/**}

📄 CodeRabbit inference engine (CLAUDE.md)

Do not make changes to build system files (guix-build*), release artifacts, or avoid changes to .github, depends, ci, contrib, and doc directories unless specifically prompted

Files:

  • doc/release-notes-26094.md
  • contrib/init/dashd.service
src/**/*.{cpp,h,hpp,cc}

📄 CodeRabbit inference engine (CLAUDE.md)

Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1

Files:

  • src/wallet/rpc/util.h
  • src/protocol.h
  • src/wallet/rpc/util.cpp
  • src/wallet/rpc/wallet.cpp
  • src/wallet/rpc/coins.cpp
  • src/wallet/rpc/transactions.cpp
src/wallet/**/*.{cpp,h}

📄 CodeRabbit inference engine (CLAUDE.md)

Wallet implementation must use Berkeley DB and SQLite

Files:

  • src/wallet/rpc/util.h
  • src/wallet/rpc/util.cpp
  • src/wallet/rpc/wallet.cpp
  • src/wallet/rpc/coins.cpp
  • src/wallet/rpc/transactions.cpp
test/functional/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Functional tests in test/functional/ must be written in Python (minimum version specified in .python-version) and depend on dashd and dash-node

Files:

  • test/functional/wallet_basic.py
  • test/functional/wallet_balance.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 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.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.

Applied to files:

  • doc/release-notes-26094.md
  • src/wallet/rpc/util.h
  • src/wallet/rpc/util.cpp
  • src/wallet/rpc/wallet.cpp
  • src/wallet/rpc/coins.cpp
  • src/wallet/rpc/transactions.cpp
  • test/functional/wallet_balance.py
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay

Applied to files:

  • src/wallet/rpc/util.h
  • src/protocol.h
  • src/wallet/rpc/transactions.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite

Applied to files:

  • src/wallet/rpc/util.h
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.

Applied to files:

  • src/wallet/rpc/util.h
  • src/wallet/rpc/wallet.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.

Applied to files:

  • test/functional/wallet_balance.py
📚 Learning: 2025-06-20T23:32:16.225Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6726
File: test/functional/rpc_createmultisig.py:120-120
Timestamp: 2025-06-20T23:32:16.225Z
Learning: In the rpc_createmultisig.py test, the checkbalances() function correctly excludes 9 blocks from the balance calculation: 8 blocks from do_multisig() calls (2 blocks per call × 4 calls) plus 1 block from checkbalances() itself.

Applied to files:

  • test/functional/wallet_balance.py
🧬 Code graph analysis (4)
src/wallet/rpc/util.h (1)
src/wallet/rpc/util.cpp (1)
  • AppendLastProcessedBlock (168-168)
src/wallet/rpc/wallet.cpp (2)
src/wallet/rpc/util.h (1)
  • RESULT_LAST_PROCESSED_BLOCK (27-53)
src/wallet/rpc/util.cpp (1)
  • AppendLastProcessedBlock (168-168)
src/wallet/rpc/coins.cpp (2)
src/wallet/rpc/util.h (1)
  • RESULT_LAST_PROCESSED_BLOCK (27-53)
src/wallet/rpc/util.cpp (1)
  • AppendLastProcessedBlock (168-168)
test/functional/wallet_balance.py (1)
src/wallet/rpc/coins.cpp (2)
  • getbalances (433-503)
  • getbalances (433-433)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: linux64_multiprocess-build / Build source
  • GitHub Check: mac-build / Build source
  • GitHub Check: linux64_tsan-build / Build source
  • GitHub Check: win64-build / Build source
  • GitHub Check: linux64_ubsan-build / Build source
  • GitHub Check: linux64_sqlite-build / Build source
  • GitHub Check: linux64_fuzz-build / Build source
  • GitHub Check: linux64-build / Build source
  • GitHub Check: linux64_nowallet-build / Build source
  • GitHub Check: arm-linux-build / Build source
🔇 Additional comments (17)
contrib/init/dashd.service (1)

21-37: LGTM! Systemd service configuration updated correctly.

The change from Type=forking to Type=notify with corresponding startup/shutdown notifications is a standard pattern for better systemd integration. The use of systemd-notify via command-line arguments is a valid approach.

src/wallet/rpc/wallet.cpp (2)

195-195: LGTM! RPC result field added correctly.

The RESULT_LAST_PROCESSED_BLOCK field is properly declared in the RPC help and matches the implementation in util.h.


274-275: LGTM! Last processed block appended correctly.

The call to AppendLastProcessedBlock is made within the scope of LOCK(pwallet->cs_wallet) (line 211), satisfying the EXCLUSIVE_LOCKS_REQUIRED annotation on the function.

src/protocol.h (1)

137-139: LGTM! Documentation improved.

The updated documentation correctly clarifies that the MEMPOOL message is defined in BIP35 and is only available with the NODE_BLOOM service bit per BIP111.

test/functional/wallet_basic.py (1)

711-713: LGTM! Test expectations updated for new field.

The lastprocessedblock field is correctly added to the expected fields set for gettransaction response validation.

src/wallet/rpc/util.cpp (1)

168-175: LGTM! Implementation is correct and thread-safe.

The function properly:

  • Enforces lock requirements via EXCLUSIVE_LOCKS_REQUIRED annotation
  • Validates lock is held with AssertLockHeld
  • Retrieves last block hash and height from wallet
  • Constructs the JSON object correctly
doc/release-notes-26094.md (1)

1-6: LGTM! Release notes are clear and comprehensive.

The documentation correctly describes the new lastprocessedblock field in all three affected RPCs and appropriately warns about caching invalidation when keys are imported or generated.

src/wallet/rpc/coins.cpp (2)

455-455: LGTM! RPC result field declared correctly.

The RESULT_LAST_PROCESSED_BLOCK is properly added to the getbalances RPC result schema.


498-500: LGTM! Last processed block appended with proper locking.

The call to AppendLastProcessedBlock is made within the scope of LOCK(wallet.cs_wallet) (line 472), satisfying the locking requirements.

test/functional/wallet_balance.py (3)

14-14: LGTM! Import added for hash validation.

The assert_is_hash_string utility is correctly imported for validating the lastprocessedblock hash field.


178-184: LGTM! Test approach handles new field appropriately.

The strategy of removing the lastprocessedblock field before comparing balances is a clean way to test the existing balance logic while deferring lastprocessedblock validation to dedicated tests.


313-337: LGTM! Comprehensive test coverage for lastprocessedblock.

The tests correctly validate:

  • Hash string format using assert_is_hash_string
  • Hash changes after generating new blocks
  • Height increments correctly
  • Consistency across getbalances, getwalletinfo, and gettransaction RPCs
src/wallet/rpc/util.h (3)

9-10: New includes correctly satisfy type dependencies

Including <rpc/util.h> (for RPCResult) and <wallet/wallet.h> (for CWallet) in this header is appropriate and resolves the new declarations below without obvious circular-dependency concerns.


27-30: RESULT_LAST_PROCESSED_BLOCK definition is consistent and self‑contained

The RESULT_LAST_PROCESSED_BLOCK object cleanly describes the new lastprocessedblock field with hash and height subfields, matching the intended schema and using the correct result types. Declaring it static in the header is acceptable and mirrors upstream patterns for reusable RPCResult descriptors.


52-52: AppendLastProcessedBlock declaration and lock annotation look correct

The AppendLastProcessedBlock(UniValue& entry, const CWallet& wallet) signature and EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) annotation match how the function is used (called with LOCK(pwallet->cs_wallet) in the RPC handlers) and ensure thread-safety annotations remain satisfied.

src/wallet/rpc/transactions.cpp (2)

739-739: gettransaction result schema cleanly extended with lastprocessedblock

Adding RESULT_LAST_PROCESSED_BLOCK to the gettransaction RPC result integrates the new lastprocessedblock object into the help/descriptor layer in a reusable way, consistent with how other wallet RPCs describe shared fields. This should be backward compatible for clients that ignore unknown JSON keys.


799-799: AppendLastProcessedBlock usage is correctly ordered and locked

Calling AppendLastProcessedBlock(entry, *pwallet) after populating the existing fields, with BlockUntilSyncedToCurrentChain() and LOCK(pwallet->cs_wallet) already in place, satisfies the EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) contract and ensures the reported block context matches other wallet RPCs. This aligns with the intended backport behavior.


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.

Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 4e43e6b (if CI succeed)

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 4e43e6b

Comment on lines +22 to +25
-conf=/etc/dash/dash.conf \
-datadir=/var/lib/dashd \
-startupnotify='systemd-notify --ready' \
-shutdownnotify='systemd-notify --stopping'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: 25975: align with -pid

Suggested change
-conf=/etc/dash/dash.conf \
-datadir=/var/lib/dashd \
-startupnotify='systemd-notify --ready' \
-shutdownnotify='systemd-notify --stopping'
-conf=/etc/dash/dash.conf \
-datadir=/var/lib/dashd \
-startupnotify='systemd-notify --ready' \
-shutdownnotify='systemd-notify --stopping'

@PastaPastaPasta PastaPastaPasta merged commit e63410e into dashpay:develop Dec 2, 2025
57 of 61 checks passed
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.

4 participants