Skip to content

Conversation

@knst
Copy link
Collaborator

@knst knst commented Nov 16, 2025

What was done?

Trivial backports from Bitcoin Core v26, v27

How Has This Been Tested?

Run unit & functional tests

Breaking Changes

N/A

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

@knst knst added this to the 23.1 milestone Nov 16, 2025
@github-actions
Copy link

github-actions bot commented Nov 16, 2025

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link

coderabbitai bot commented Nov 16, 2025

Walkthrough

This pull request encompasses multiple improvements across fuzzing, debugging, testing, and build tooling. Changes include documentation updates (loglevel help text), refactored fuzzing infrastructure (minisketch with parameterized implementations, new local\_address fuzz target), enhanced logging for database inconsistencies and transaction states, test enhancements for mempool filtering and network hashrate computation, expanded UBSan suppressions for third-party libraries, and environment variable overrides for test executables. No control flow changes are made to core functionality; most changes add logging, testing coverage, or suppress known issues.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–35 minutes

Areas requiring extra attention:

  • src/test/fuzz/minisketch.cpp — Verify that the impl selection logic from fuzz data is correct and that MakeFuzzMinisketch32 properly instantiates sketches; ensure SetSeed initialization matches the original behavior.
  • src/test/fuzz/net.cpp — Review the new local\_address fuzz target logic, particularly the CallOneOf lambda dispatch and validation checks (IsRoutable, IsLocal, SeenLocal) to ensure they exercise the intended code paths.
  • src/wallet/transaction.h — Check toString() formatting for each TxState struct variant; ensure consistent and meaningful output for all states.
  • test/sanitizer_suppressions/ubsan — Verify that the expanded suppression paths correctly target the third-party libraries (leveldb, minisketch, secp256k1, boost) without over-suppressing real issues in the main codebase.
  • test/functional/rpc\_blockchain.py — Confirm the DIFFICULTY\_ADJUSTMENT\_INTERVAL constant usage and the negative-parameter getnetworkhashps logic compute the expected value correctly.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% 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
Title check ✅ Passed The title lists multiple Bitcoin Core issue numbers being backported, which directly corresponds to the changeset containing backports from Bitcoin Core v26 and v27.
Description check ✅ Passed The description explains that the PR contains trivial backports from Bitcoin Core v26 and v27, which aligns with the changeset modifications across multiple files.
✨ 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 2e95cee and b84939e.

📒 Files selected for processing (9)
  • src/init/common.cpp (1 hunks)
  • src/test/fuzz/minisketch.cpp (2 hunks)
  • src/test/fuzz/net.cpp (1 hunks)
  • src/txdb.cpp (1 hunks)
  • src/wallet/transaction.h (5 hunks)
  • src/wallet/wallet.cpp (1 hunks)
  • test/functional/interface_rest.py (1 hunks)
  • test/functional/p2p_addr_relay.py (1 hunks)
  • test/functional/rpc_blockchain.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
  • src/init/common.cpp
  • src/wallet/transaction.h
  • test/functional/interface_rest.py
  • src/wallet/wallet.cpp
  • test/functional/p2p_addr_relay.py
  • src/txdb.cpp
  • src/test/fuzz/minisketch.cpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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: 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: 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: 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.
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
📚 Learning: 2025-11-04T18:23:28.175Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/governance/classes.cpp:147-154
Timestamp: 2025-11-04T18:23:28.175Z
Learning: In src/governance/classes.cpp, CSuperblock::GetPaymentsLimit intentionally uses synthetic difficulty constants (nBits = 1 for mainnet, powLimit for networks with min difficulty) and simple height-based V20 activation checks instead of actual chain block data. This is by design because superblocks themselves are "synthetic" governance payment blocks, not regular mined blocks.

Applied to files:

  • test/functional/rpc_blockchain.py
🧬 Code graph analysis (2)
test/functional/rpc_blockchain.py (1)
test/functional/test_framework/util.py (1)
  • assert_equal (69-74)
src/test/fuzz/net.cpp (2)
src/evo/netinfo.h (1)
  • service (240-244)
src/test/fuzz/util.h (1)
  • ConsumeService (315-318)
⏰ 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: mac-build / Build source
  • GitHub Check: linux64_tsan-build / Build source
  • GitHub Check: win64-build / Build source
  • GitHub Check: arm-linux-build / Build source
  • GitHub Check: linux64_nowallet-build / Build source
  • GitHub Check: linux64_ubsan-build / Build source
  • GitHub Check: linux64_sqlite-build / Build source
  • GitHub Check: linux64-build / Build source
  • GitHub Check: linux64_fuzz-build / Build source
  • GitHub Check: Lint / Run linters
🔇 Additional comments (3)
src/test/fuzz/net.cpp (1)

98-133: LGTM! Well-structured fuzz target for local address operations.

The new local_address fuzz target is well-implemented and follows the established patterns from the existing net fuzz target. Key aspects are correctly handled:

  • Proper initialization and setup using the shared test infrastructure
  • Thread-safe clearing of mapLocalHost under mutex lock
  • Bounded fuzzing loop using LIMITED_WHILE
  • Intentional invariant assertions (lines 116-118) that verify AddLocal's contract: when it returns true, the service must be routable, local, and seen
  • Appropriate handling of failed AddLocal calls via early return
  • Comprehensive exercise of all local address functions
test/functional/rpc_blockchain.py (2)

62-62: DIFFICULTY_ADJUSTMENT_INTERVAL constant looks correct

Using a local DIFFICULTY_ADJUSTMENT_INTERVAL = 2016 matches Bitcoin Core’s tests and keeps this file self-contained. No issues from a Dash standpoint as long as the C++ side still uses the same interval for getnetworkhashps’s default window, which appears consistent with the backport intent.


435-443: Negative-argument tests for getnetworkhashps are consistent with upstream semantics

The added logic correctly derives the block window since the last difficulty change and verifies that getnetworkhashps(-1) and getnetworkhashps(-2) both return that same averaged value. This matches the C++ implementation behavior for nblocks <= 0 and should guard against regressions in the RPC semantics.

Warning

Tools execution failed with the following error:

Failed to run tools: 14 UNAVAILABLE: read ECONNRESET

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (2)
src/txdb.cpp (1)

117-119: Minor: daemon name in user-facing log

Message says “restart bitcoind”; in Dash this is “dashd”. Consider adjusting for branding; if strict backport fidelity is desired, ignore.

-                LogPrintLevel(BCLog::COINDB, BCLog::Level::Error, "The coins database detected an inconsistent state, likely due to a previous crash or shutdown. You will need to restart bitcoind with the -reindex-chainstate or -reindex configuration option.\n");
+                LogPrintLevel(BCLog::COINDB, BCLog::Level::Error, "The coins database detected an inconsistent state, likely due to a previous crash or shutdown. You will need to restart dashd with the -reindex-chainstate or -reindex configuration option.\n");

Based on learnings

src/wallet/transaction.h (1)

31-31: TxState stringification LGTM

toString() methods and TxStateString() improve logging and are header‑safe. Optionally include explicitly to avoid relying on indirect includes.

Also applies to: 36-36, 45-45, 56-56, 69-69, 116-121

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between caf5010 and 5bb99b9.

📒 Files selected for processing (13)
  • configure.ac (1 hunks)
  • src/init/common.cpp (1 hunks)
  • src/test/fuzz/minisketch.cpp (2 hunks)
  • src/test/fuzz/net.cpp (1 hunks)
  • src/txdb.cpp (1 hunks)
  • src/wallet/transaction.h (5 hunks)
  • src/wallet/wallet.cpp (1 hunks)
  • test/functional/interface_rest.py (1 hunks)
  • test/functional/p2p_addr_relay.py (1 hunks)
  • test/functional/rpc_blockchain.py (2 hunks)
  • test/lint/lint-python.py (2 hunks)
  • test/sanitizer_suppressions/ubsan (1 hunks)
  • test/util/test_runner.py (1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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: 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: 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: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
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.
📚 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:

  • src/wallet/wallet.cpp
  • src/wallet/transaction.h
📚 Learning: 2025-01-14T09:06:19.717Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6532
File: src/test/fuzz/netaddress.cpp:83-84
Timestamp: 2025-01-14T09:06:19.717Z
Learning: In fuzzer harness tests, CServiceHash can be used with both default constructor (CServiceHash()) and parameterized constructor (CServiceHash(salt_k0, salt_k1)) to test different variants. The usage pattern CServiceHash()(service) and CServiceHash(0, 0)(service) is valid and intentional in such tests.

Applied to files:

  • src/test/fuzz/minisketch.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/interface_rest.py
📚 Learning: 2025-11-04T18:23:28.175Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/governance/classes.cpp:147-154
Timestamp: 2025-11-04T18:23:28.175Z
Learning: In src/governance/classes.cpp, CSuperblock::GetPaymentsLimit intentionally uses synthetic difficulty constants (nBits = 1 for mainnet, powLimit for networks with min difficulty) and simple height-based V20 activation checks instead of actual chain block data. This is by design because superblocks themselves are "synthetic" governance payment blocks, not regular mined blocks.

Applied to files:

  • test/functional/rpc_blockchain.py
🧬 Code graph analysis (4)
src/test/fuzz/net.cpp (2)
src/evo/netinfo.h (1)
  • service (240-244)
src/test/fuzz/util.h (1)
  • ConsumeService (315-318)
src/wallet/wallet.cpp (1)
src/wallet/transaction.h (1)
  • TxStateString (118-121)
test/functional/rpc_blockchain.py (1)
test/functional/test_framework/util.py (1)
  • assert_equal (69-74)
test/functional/p2p_addr_relay.py (4)
test/functional/test_framework/test_node.py (1)
  • assert_debug_log (444-472)
test/functional/test_framework/p2p.py (2)
  • send_and_ping (750-752)
  • sync_with_ping (762-770)
test/functional/test_framework/messages.py (1)
  • msg_getaddr (1859-1873)
test/functional/test_framework/util.py (1)
  • assert_equal (69-74)
⏰ 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_fuzz-build / Build source
  • GitHub Check: linux64_sqlite-build / Build source
  • GitHub Check: linux64_ubsan-build / Build source
  • GitHub Check: linux64-build / Build source
  • GitHub Check: linux64_nowallet-build / Build source
  • GitHub Check: mac-build / Build source
  • GitHub Check: linux64_tsan-build / Build source
  • GitHub Check: arm-linux-build / Build source
  • GitHub Check: win64-build / Build source
  • GitHub Check: Lint / Run linters
🔇 Additional comments (15)
test/util/test_runner.py (1)

77-80: LGTM! Clean implementation of environment variable overrides.

The addition of BITCOINUTIL and BITCOINTX environment variable overrides provides useful flexibility for test execution while maintaining backward compatibility through the default parameter.

src/test/fuzz/net.cpp (1)

98-133: LGTM! Well-designed fuzz target.

The new local_address fuzz target is well-structured and follows good practices:

  • Properly clears mapLocalHost with appropriate locking before fuzzing
  • Uses bounded iteration to prevent runaway execution
  • Exercises all key local address operations (AddLocal, RemoveLocal, IsLocal, SeenLocal, GetLocalAddress)
  • Includes post-condition assertions (lines 116-118) to verify invariants after successful AddLocal calls

The implementation is correct and safe.

src/test/fuzz/minisketch.cpp (3)

15-22: LGTM! Good helper function for consistent Minisketch construction.

The helper function appropriately wraps Minisketch construction with an assertion to ensure validity, which is good defensive programming for fuzz tests. The anonymous namespace correctly limits its scope to this file.


27-35: LGTM! Implementation selection and seed initialization enhance fuzz coverage.

The changes correctly:

  • Select a Minisketch implementation from fuzzed data and skip testing if unsupported
  • Create sketches using the new helper function with parameterized implementation
  • Initialize seeds from fuzzed data, improving test coverage by exercising different seed values

63-67: LGTM! Consistent reconstruction pattern.

The reconstructed sketches now follow the same initialization pattern as the original sketches. Setting seeds before deserialization (lines 68-69) is appropriate for fuzz testing, as it verifies that Deserialize() properly restores all state including the seed value.

test/functional/p2p_addr_relay.py (1)

299-308: LGTM! The test logic correctly verifies the getaddr rate-limiting behavior.

The test properly validates that the node ignores repeated getaddr messages from the same connection by:

  1. Recording the address count baseline
  2. Sending a repeated getaddr and confirming the expected debug log
  3. Advancing mocktime to trigger the address send timer
  4. Verifying no additional addresses were sent

The implementation follows the established patterns in this test file for time manipulation and synchronization.

test/lint/lint-python.py (2)

15-16: LGTM! Modernized import statement.

The imports correctly introduce importlib.metadata, which is the modern standard library replacement for the deprecated pkg_resources module.


101-103: Code change verified and approved.

The refactored dependency checking using importlib.metadata.metadata() is correct. Imports are properly declared (from importlib.metadata import metadata, PackageNotFoundError), the try-except pattern appropriately handles missing dependencies, and Python 3.9 (minimum version specified in .python-version) fully supports importlib.metadata (requires 3.8+).

test/functional/rpc_blockchain.py (2)

62-62: LGTM!

The constant correctly defines Bitcoin's difficulty adjustment interval (2016 blocks) and improves code readability for the new test logic below.


435-442: LGTM!

The test enhancement correctly validates that getnetworkhashps with negative block counts returns the average hash rate since the last difficulty adjustment. The calculation using modulo arithmetic properly accounts for 0-indexed block heights, and testing with both -1 and -2 provides confidence in the consistency of the behavior.

test/sanitizer_suppressions/ubsan (2)

1-2: LGTM! Improved documentation clarity.

The updated comment provides clearer guidance on the expected suppression format, which helps maintain consistency across entries.


16-16: Inconsistency between summary and code annotations.

The AI summary states that "an additional suppression entry for unsigned-integer-overflow targeting test/fuzz/crypto_diff_fuzz_chacha20.cpp" was added, but line 16 is not marked with the ~ indicator for changes. This suggests the line already existed in the file, contradicting the summary.

configure.ac (1)

1147-1153: Comment path update looks good

Reference now correctly points to src/util/fs_helpers.cpp; no logic change.

src/wallet/wallet.cpp (1)

968-969: Richer AddToWallet log

Appending TxStateString(state) is safe and useful for diagnostics. Format specifiers match.

src/init/common.cpp (1)

68-68: -loglevel help text clarity

Clearer description; no behavior change.

Comment on lines +339 to +341
# pop unstable unbroadcastcount before check
for obj in [json_obj, mempool_info]:
obj.pop("unbroadcastcount")
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Avoid KeyError when field is absent

json_obj or mempool_info may lack "unbroadcastcount". Use a default.

-        for obj in [json_obj, mempool_info]:
-            obj.pop("unbroadcastcount")
+        for obj in [json_obj, mempool_info]:
+            obj.pop("unbroadcastcount", None)
📝 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
# pop unstable unbroadcastcount before check
for obj in [json_obj, mempool_info]:
obj.pop("unbroadcastcount")
# pop unstable unbroadcastcount before check
for obj in [json_obj, mempool_info]:
obj.pop("unbroadcastcount", None)
🤖 Prompt for AI Agents
In test/functional/interface_rest.py around lines 339 to 341, popping
"unbroadcastcount" directly can raise KeyError when json_obj or mempool_info
doesn't contain that key; change the pop calls to use a default value (e.g.,
obj.pop("unbroadcastcount", None)) or guard with an if "unbroadcastcount" in obj
to safely remove the key only when present.

unsigned-integer-overflow:*/include/c++/
# unsigned-integer-overflow in FuzzedDataProvider's ConsumeIntegralInRange
unsigned-integer-overflow:FuzzedDataProvider.h
unsigned-integer-overflow:FuzzedDataProvider::ConsumeIntegralInRange
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Appropriate suppression for fuzzing infrastructure.

The explicit suppression for FuzzedDataProvider::ConsumeIntegralInRange is correct, as this LLVM libFuzzer function can intentionally trigger unsigned integer overflow during range calculations. This is expected behavior in fuzzing infrastructure.

Since this is a backport PR, please verify that this change matches the upstream Bitcoin Core commits referenced in the PR description.


🏁 Script executed:

#!/bin/bash
# Description: Verify the suppression entry matches upstream Bitcoin Core changes.

# Search for FuzzedDataProvider suppressions in the repository
rg -n "FuzzedDataProvider" test/sanitizer_suppressions/

Length of output: 105


The search reveals a significant finding: upstream uses unsigned-integer-overflow:FuzzedDataProvider.h (file-level suppression), while your repository uses unsigned-integer-overflow:FuzzedDataProvider::ConsumeIntegralInRange (function-level suppression) on line 12.

I found a discrepancy: upstream Bitcoin Core uses unsigned-integer-overflow:FuzzedDataProvider.h (file-level suppression), while your repository uses unsigned-integer-overflow:FuzzedDataProvider::ConsumeIntegralInRange (function-level suppression) on line 12.

Both formats are valid UBSAN suppression syntax. However, since this is a backport PR, there's a format mismatch with upstream that should be addressed for consistency.

Adjust suppression format to match upstream Bitcoin Core.

The suppression on line 12 should use the file-based format instead of the function-qualified format to maintain backport fidelity:

unsigned-integer-overflow:FuzzedDataProvider.h
🤖 Prompt for AI Agents
In test/sanitizer_suppressions/ubsan around line 12, update the UBSAN
suppression entry which currently targets the function
"unsigned-integer-overflow:FuzzedDataProvider::ConsumeIntegralInRange" to the
file-level format used upstream "unsigned-integer-overflow:FuzzedDataProvider.h"
so the backport matches upstream; replace the function-qualified suppression
with the file-based suppression on that line and ensure there are no duplicate
or conflicting entries elsewhere in the file.

@knst knst force-pushed the bp-v26-trivial branch 2 times, most recently from 66c82ca to c6b75c4 Compare November 17, 2025 17:49
@knst knst marked this pull request as draft November 17, 2025 20:22
@knst knst requested a review from UdjinM6 November 18, 2025 09:22
@knst knst marked this pull request as ready for review November 18, 2025 09:22
@knst knst changed the title backport: bitcoin#27656, #27717, #27852, #28147, #28347, #28350, #28396, #28544, #29064, #29230, #29467, #29495 backport: bitcoin#27717, #27852, #28147, #28347, #28350, #28396, #28544, #29064, #29230, #29467, #29495 Nov 18, 2025
…TIL` and `BITCOINTX`

4f2f615 test: Make `util/test_runner.py` honor `BITCOINUTIL` and `BITCOINTX` (Hennadii Stepanov)

Pull request description:

  This PR is a continuation of changes to our testing frameworks (bitcoin#27554, bitcoin#27561) that allow them to work correctly in a multi-config build environment that is possible for [upcoming](bitcoin#25797) CMake-based build system. That means that built for different configurations binaries (e.g., "Debug" and "Release") can coexist in separated directories.

  The commit has been pulled from hebasto#15 and it seems [useful](hebasto#15 (comment)) by itself as:
  > I believe the rationale for allowing to drop in the executables via env var is to allow to test the guix-produced, or other third-party-produced executables...

  The current implementation of the `test/functional/test_framework/test_framework.py` script uses the same approach: https://github.com/bitcoin/bitcoin/blob/09351f51d279612973ecd76811dc075dff08209f/test/functional/test_framework/test_framework.py#L231-L246

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 4f2f615
  TheCharlatan:
    ACK 4f2f615
  stickies-v:
    ACK 4f2f615

Tree-SHA512: 99ee9a727b266700649d8f2ec528dfaeb04a1e48f0cb1d4eeaece65917165be647c10c4548429a9e8b30d094597f67e860c1db03ac689ebb409b223ce1b63aa9
…ame` should be used

d0c6cc4 suppressions: note that 'type:ClassName::MethodName' should be used (fanquake)

Pull request description:

  Now that the symbolizer is back in play, suppressions can once-again be targeted to functions, rather than file-wide.

ACKs for top commit:
  MarcoFalke:
    lgtm ACK d0c6cc4
  hebasto:
    ACK d0c6cc4

Tree-SHA512: fb65398eae18a6ebc5f8414275c568cf2664ab5357c2b3160f3bf285b67bc3af788225c5dba3c824c0e098627789450bec775375f52529d71c6ef700a9632d65
kwvg
kwvg previously approved these changes Nov 21, 2025
Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

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

utACK 2e95cee

UdjinM6
UdjinM6 previously approved these changes Nov 21, 2025
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 2e95cee

@knst knst dismissed stale reviews from UdjinM6 and kwvg via b84939e November 21, 2025 10:29
Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

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

utACK b84939e

@kwvg kwvg requested a review from UdjinM6 November 21, 2025 10:43
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 b84939e

@PastaPastaPasta PastaPastaPasta merged commit 11a36d5 into dashpay:develop Nov 21, 2025
31 of 35 checks passed
…in inconsistent state

df60de7 log: Print error message when coindb is in inconsistent state (Fabian Jahr)

Pull request description:

  While doing manual testing on assumeutxo this week I managed to put the coindb into an inconsistent state twice. For a normal user, this can also happen if their computer crashes during a flush or if they try to stop their node during a flush and then get tired of waiting and just shut their computer down or kill the process. It's an edge case but I wouldn't be surprised if this does happen more often when assumeutxo gets used more widely because there might be multiple flushes happening during loading of the UTXO set in the beginning and users may think something is going wrong because of the unexpected wait or they forgot some configs and want to start over quickly.

  The problem is, when this happens at first the node starts up normally until it's time to flush again and then it hits an assert that the user can not understand.

  ```
  2023-08-25T16:31:09Z [httpworker.0] [snapshot] 52000000 coins loaded (43.30%, 6768 MB)
  2023-08-25T16:31:16Z [httpworker.0] Cache size (7272532192) exceeds total space (7256510300)
  2023-08-25T16:31:16Z [httpworker.0] FlushSnapshotToDisk: flushing coins cache (7272 MB) started
  Assertion failed: (old_heads[0] == hashBlock), function BatchWrite, file txdb.cpp, line 126.
  Abort trap: 6
  ```

  We should at least log an error message that gives users a hint of what the problem is and what they can do to resolve it. I am keeping this separate from the assumeutxo project since this issue can also happen during any regular flush.

ACKs for top commit:
  jonatack:
    ACK df60de7
  achow101:
    ACK df60de7
  ryanofsky:
    Code review ACK df60de7
  jamesob:
    Code review ACK df60de7

Tree-SHA512: b546aa0b0323ece2962867a29c38e014ac83ae8f1ded090da2894b4ff2450c05229629c7e8892f7b550cf7def4038a0b4119812e548e11b00c60b1dc3d4276d2
fanquake and others added 7 commits November 22, 2025 00:28
…sponded once per connection

668aa6a test: p2p: check that `getaddr` msgs are only responded once per connection (Sebastian Falbesoner)

Pull request description:

  This simple PR adds missing test coverage for ignoring repeated `getaddr` requests (introduced in bitcoin#7856, commit 66b0724):
  https://github.com/bitcoin/bitcoin/blob/6f03c45f6bb5a6edaa3051968b6a1ca4f84d2ccb/src/net_processing.cpp#L4642-L4648

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 668aa6a
  brunoerg:
    crACK 668aa6a

Tree-SHA512: edcdc6501c684fb41911e393f55ded9b044cd2f92918877eca152edd5a4287d1a9d57ae999f1cb42185eae00c3a0af411fcb9bcd5b990ef48849c3834b141584
… and logging

8a553c9 wallet: Add TxStateString function for debugging and logging (Ryan Ofsky)

Pull request description:

  I found this useful while debugging silent conflict between bitcoin#10102 and bitcoin#27469 recently

ACKs for top commit:
  ishaanam:
    utACK 8a553c9
  achow101:
    ACK 8a553c9
  furszy:
    Code ACK 8a553c9

Tree-SHA512: 87965c66bcb59a21e7639878bb567e583a0e624735721ff7ad1104eed6bb9fba60607d0e3de7be3304232b3a55f48bab7039ea9c26b0e81963e59f9acd94f666
376dc2c test: add coverage to rpc_blockchain.py (kevkevin)

Pull request description:

  Included a test that checks the functionality of setting
  the first param of getnetworkhashps to negative value returns
  the average network hashes per second from the last difficulty change.

ACKs for top commit:
  jlopp:
    tACK bitcoin@376dc2c
  achow101:
    ACK 376dc2c
  ismaelsadeeq:
    Tested ACK 376dc2c
  pablomartin4btc:
    tACK 376dc2c

Tree-SHA512: 02d52f622e9cb7a1240c5d124510dd75d03f696f119b2625b0befd60b004ec50ff1a2d5515e0e227601adeecd837e0778ed131ee2a8c5f75f1b824be711213a7
…arness

b2fc7a2 [fuzz] Improve fuzzing stability for minisketch harness (dergoegge)

Pull request description:

  The `minisketch` harness has low stability due to:
  * Rng internal to minisketch
  * Benchmarkning for the best minisketch impl

  Fix this by seeding the rng and letting the fuzzer choose the impl.

  Also see bitcoin#29018.

ACKs for top commit:
  maflcko:
    review ACK b2fc7a2

Tree-SHA512: 3d81414299c6803c34e928a53bcf843722fa8c38e1d3676cde7fa80923f9058b1ad4b9a2941f718303a6641b17eeb28b4a22eda09678102e9fb7c4e31d06f8f2
…always logged levels

ec779a2 doc: add unconditional info loglevel following merge of PR 28318 (Jon Atack)

Pull request description:

  Commit ab34dc6 of bitcoin#28318 was an incomplete version of [`118c756` (bitcoin#25203)](bitcoin@118c756) from the `Severity-based logging` parent PR.

  Add the missing text to update the `-loglevel` help doc.

  While here, make the help text a little easier to understand.

  Can be tested by running:

  ```
  ./src/bitcoind -regtest -help-debug | grep -A12 loglevel=
  ```

  before
  ```
    -loglevel=<level>|<category>:<level>
         Set the global or per-category severity level for logging categories
         enabled with the -debug configuration option or the logging RPC:
         info, debug, trace (default=debug); warning and error levels are
         always logged.
  ```

  after
  ```
    -loglevel=<level>|<category>:<level>
         Set the global or per-category severity level for logging categories
         enabled with the -debug configuration option or the logging RPC.
         Possible values are info, debug, trace (default=debug). The
         following levels are always logged: error, warning, info.
  ```

ACKs for top commit:
  stickies-v:
    ACK ec779a2

Tree-SHA512: 0c375e30a5a4c168ca7d97720e8c287f598216767afedae329824e09a480830faf8537b792c5c4bb647c68681c287fe3005c62093708ce85624e9a71c8245e42
faeed91 test: Fix intermittent issue in interface_rest.py (MarcoFalke)

Pull request description:

  Fixes:

  ```
   test  2024-02-22T16:15:37.465000Z TestFramework (ERROR): Assertion failed
    Traceback (most recent call last):
    File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
     self.run_test()
    File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/interface_rest.py", line 340, in run_test
     assert_equal(json_obj, mempool_info)
    File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 57, in assert_equal
     raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
   AssertionError: not({'loaded': True, 'size': 3, 'bytes': 312, 'usage': 3600, 'total_fee': Decimal('0.00093600'), 'maxmempool': 300000000, 'mempoolminfee': Decimal('0.00001000'), 'minrelaytxfee': Decimal('0.00001000'), 'incrementalrelayfee': Decimal('0.00001000'), 'unbroadcastcount': 1, 'fullrbf': False} == {'loaded': True, 'size': 3, 'bytes': 312, 'usage': 3600, 'total_fee': Decimal('0.00093600'), 'maxmempool': 300000000, 'mempoolminfee': Decimal('0.00001000'), 'minrelaytxfee': Decimal('0.00001000'), 'incrementalrelayfee': Decimal('0.00001000'), 'unbroadcastcount': 0, 'fullrbf': False})
  ```

  https://cirrus-ci.com/task/4852944378527744?logs=ci#L4436

ACKs for top commit:
  m3dwards:
    ACK bitcoin@faeed91
  mzumsande:
    ACK faeed91

Tree-SHA512: 513422229db45d2586c554b9a466e86848bfcf5280b0f000718cbfc44d93dd1af69e19a56f6ac578f5d7aada74ab0c90d4a9e09a324062b6f9ed239e5e34f540
25eab52 fuzz: add target for local addresses (brunoerg)

Pull request description:

  This PR adds fuzz target for local address functions - (`AddLocal`, `RemoveLocal`, `SeenLocal`, `IsLocal`)

ACKs for top commit:
  dergoegge:
    ACK 25eab52
  vasild:
    ACK 25eab52

Tree-SHA512: 24faaab86dcd8835ba0e2d81fb6322a39a9266c7edf66415dbc4421754054f47efb6e0de4efdc7ea026b0686792658e86a526f7cf27cbc6cf9ed0c4aed376f97
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.

6 participants