Skip to content

Conversation

@vijaydasmp
Copy link

@vijaydasmp vijaydasmp commented Dec 18, 2025

Backporting Bitcoin

@github-actions
Copy link

github-actions bot commented Dec 18, 2025

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@vijaydasmp vijaydasmp changed the title backport : Merge bitcoin#28523, 28583 backport: Merge bitcoin#28523, 28583 Dec 18, 2025
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#28523, 28583 backport: Merge bitcoin#28583 Dec 20, 2025
@vijaydasmp vijaydasmp force-pushed the Jan_2026_2 branch 2 times, most recently from d20d4f3 to af9d7c0 Compare December 20, 2025 17:57
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#28583 backport: Merge bitcoin#28085,26066 Dec 20, 2025
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#28085,26066 backport: Merge bitcoin#28085,26066, 28101 Dec 20, 2025
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#28085,26066, 28101 backport: Merge bitcoin#28085,26066, 28101, 28059 Dec 20, 2025
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#28085,26066, 28101, 28059 backport: Merge bitcoin#28085, 26066, 28101, 28059 Dec 20, 2025
@vijaydasmp vijaydasmp force-pushed the Jan_2026_2 branch 2 times, most recently from d243418 to dc3ee4b Compare December 21, 2025 15:37
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#28085, 26066, 28101, 28059 backport: Merge bitcoin#28085, 26066, 28101 Dec 21, 2025
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#28085, 26066, 28101 backport: Merge bitcoin#26066, 28101 Dec 21, 2025
@vijaydasmp vijaydasmp force-pushed the Jan_2026_2 branch 3 times, most recently from 8196782 to ad07778 Compare December 22, 2025 15:54
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#26066, 28101 backport: Merge bitcoin#28101, 28649, 28787, 27572, 28025 Dec 22, 2025
@vijaydasmp vijaydasmp force-pushed the Jan_2026_2 branch 2 times, most recently from d6c3099 to 9ab5b24 Compare December 23, 2025 05:13
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#28101, 28649, 28787, 27572, 28025 backport: Merge bitcoin#28101, 28787, 27572, 28025 Dec 23, 2025
@vijaydasmp vijaydasmp marked this pull request as ready for review December 24, 2025 12:12
@coderabbitai
Copy link

coderabbitai bot commented Dec 24, 2025

Walkthrough

This pull request centralizes the default Tor control port by adding constexpr int DEFAULT_TOR_CONTROL_PORT = 9051 and updating references and help text to use it. It removes the wallet CLI option -zapwallettxes from registration and validation. Test code changes replace inline signing and manual hashing with a new sign_input helper and use sha256sum_file for file SHA-256 computations; several test files import and use these helpers accordingly. No public API signatures were changed.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 2 inconclusive)
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.
Title check ❓ Inconclusive The title references four Bitcoin PR numbers but does not clearly summarize the main changes; it reads as a commit reference list rather than describing what the changes accomplish. Consider a more descriptive title such as 'refactor: Extract Tor control port constant and remove -zapwallettxes' to better communicate the primary changes.
Description check ❓ Inconclusive The description 'Backporting Bitcoin' is too vague and generic; it does not convey any meaningful information about what is being backported or what changes are included. Expand the description to briefly outline the main changes, such as extracting the Tor control port constant, removing the -zapwallettxes option, and refactoring test helper functions.
✨ 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: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b655259 and b1269fe.

📒 Files selected for processing (10)
  • src/init.cpp
  • src/torcontrol.cpp
  • src/torcontrol.h
  • src/wallet/init.cpp
  • test/functional/feature_block.py
  • test/functional/rpc_dumptxoutset.py
  • test/functional/test_framework/script.py
  • test/functional/test_framework/wallet.py
  • test/functional/tool_wallet.py
  • test/lint/check-doc.py
💤 Files with no reviewable changes (1)
  • src/wallet/init.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
  • src/torcontrol.h
  • src/torcontrol.cpp
  • src/init.cpp
  • test/lint/check-doc.py
  • test/functional/test_framework/script.py
  • test/functional/rpc_dumptxoutset.py
🧰 Additional context used
📓 Path-based instructions (1)
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/test_framework/wallet.py
  • test/functional/feature_block.py
  • test/functional/tool_wallet.py
🧠 Learnings (1)
📓 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.
🧬 Code graph analysis (2)
test/functional/test_framework/wallet.py (1)
test/functional/test_framework/script.py (1)
  • sign_input (669-677)
test/functional/tool_wallet.py (1)
test/functional/test_framework/util.py (1)
  • sha256sum_file (304-311)
⏰ 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_nowallet-build / Build source
  • GitHub Check: arm-linux-build / Build source
  • GitHub Check: mac-build / Build source
  • GitHub Check: win64-build / Build source
  • GitHub Check: linux64_ubsan-build / Build source
  • GitHub Check: linux64-build / Build source
  • GitHub Check: linux64_sqlite-build / Build source
  • GitHub Check: linux64_fuzz-build / Build source
  • GitHub Check: linux64_tsan-build / Build source
  • GitHub Check: Lint / Run linters
🔇 Additional comments (5)
test/functional/tool_wallet.py (1)

15-18: LGTM! Clean refactor to use centralized hashing helper.

The change replaces manual hashing logic with the sha256sum_file utility function, which provides consistent SHA-256 checksumming across the test suite. The implementation correctly calls .hex() on the digest returned by the helper.

Based on learnings, this is a backport PR and the refactoring aligns with test framework improvements from upstream.

Also applies to: 52-52

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

35-35: LGTM! Refactor to centralized signing helper is correct.

The change replaces manual sighash computation and DER signature construction with the sign_input helper. The retry loop correctly generates signatures until achieving the 73-byte target for deterministic fee calculation (high-R case: 33-byte R + 32-byte S + 6-byte DER headers + 1-byte sighash type + 1-byte push opcode).

The sign_input helper prepends the signature to scriptSig, which works correctly here since scriptSig starts empty.

Based on learnings, this backport aligns with the broader test framework refactor to standardize transaction signing across tests.

Also applies to: 138-143

test/functional/feature_block.py (3)

45-45: LGTM! Updated import to use centralized signing helper.


553-554: LGTM! Correct P2SH signing pattern.

The code correctly constructs P2SH scriptSig by:

  1. Setting scriptSig = [redeem_script]
  2. Calling sign_input, which prepends the signature

This produces the standard P2SH spend format: [signature] + [redeem_script].

Based on learnings, this backport correctly adopts the centralized sign_input helper.


1347-1352: LGTM! Clean replacement of manual signing logic.

The refactor correctly uses sign_input to replace manual sighash computation and signature construction. Since scriptSig starts empty (from the transaction constructor), the helper correctly sets it to the signature.


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.

UdjinM6
UdjinM6 previously approved these changes Dec 27, 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 b655259 with a nit

achow101 and others added 4 commits December 27, 2025 17:53
…ify that a default port is used

9a84200 doc, refactor: Changing -torcontrol help to specify that a default port is used (kevkevin)

Pull request description:

  Right now when we get the help for -torcontrol it says that there is a default ip and port we dont specify if there is a specified ip that we would also use port 9051 as default

  Also I create a new const instead of using 9051 directly in the function

  linking this PR because this was discussed here bitcoin#28018

ACKs for top commit:
  jonatack:
    re-ACK 9a84200
  achow101:
    ACK 9a84200
  MarnixCroes:
    utACK 9a84200
  kristapsk:
    utACK 9a84200

Tree-SHA512: 21d9e65f3c280a2853a9cf60d4e93e8d72caccea106206d1862c19535bde7ea6ada7f55e6ea19a1fc0f59dbe791ec6fc4084fdbe7fa6d6991fa89c62070db637
…ing hidden option)

5039c34 init: completely remove `-zapwallettxes` (remaining hidden option) (Sebastian Falbesoner)

Pull request description:

  The `-zapwallettxes` functionality has been removed in v0.21.0 (see commit 3340dba / PR bitcoin#19671), with the parameter being kept as hidden option, to inform users via an exit error that `abandontransaction` should be used instead.

  As any guides that still suggest to use `-zapwallettxes` would refer to a Bitcoin Core version that is EOL since many years (i.e. <= v0.20.x), it is highly unlikely that the error caused by the option is still relevant for any user, hence it seems fine to remove it now.

ACKs for top commit:
  achow101:
    ACK 5039c34
  BrandonOdiwuor:
    ACK 5039c34
  fanquake:
    ACK 5039c34

Tree-SHA512: e3ccc6918e0f8fa68dbd1a7ec4999cc2a44e28038711919fcddaf0727648c73a9ba0fb77674317147592a113fad20755d4e727f48176bc17b048fbdebad2d6c9
…helper

2c0c6f4 test: dedup file hashing using `sha256sum_file` helper (Sebastian Falbesoner)

Pull request description:

  Rather than doing the open/read/hash-steps manually in the affected functional tests, we can just use the `sha256sum_file` helper from the utils module instead.

  Note that for the tool_wallet.py test, the used hash is changed from sha1 to sha256, but as the only purpose is to detect file content changes, this doesn't matter. Also, the optimization using `memoryview` is overkill here, as the opened file has only a size of 24KiB and determining the hash via the helper doesn't take longer than a few hundred micro-seconds on my machine.

ACKs for top commit:
  kristapsk:
    ACK 2c0c6f4

Tree-SHA512: 64fe21650b56a50e9f1a95f6ef27d88d8bfbb621e5be456f327ef8dbb5596b529d03976c200f3fd68da48cc427de9f257b403f3228e38cf1df918006674fac68
… for tx inputs

5cf4427 test: refactor: deduplicate legacy ECDSA signing for tx inputs (Sebastian Falbesoner)

Pull request description:

  There are several instances in functional tests and the framework (MiniWallet, feature_block.py, p2p_segwit.py) where we create a legacy ECDSA signature for a certain transaction's input by doing the following steps:

  1. calculate the `LegacySignatureHash` with the desired sighash type
  2. create the actual digital signature by calling `ECKey.sign_ecdsa` on the signature message hash calculated above
  3. put the DER-encoded result as CScript data push into tx input's scriptSig

  Create a new helper `sign_input_legacy` which hides those details and takes only the necessary parameters (tx, input index, relevant scriptPubKey, private key, sighash type [SIGHASH_ALL by default]). For further convenience, the signature is prepended to already existing data-pushes in scriptSig, in order to avoid rehashing the transaction after calling the new signing function.

ACKs for top commit:
  dimitaracev:
    ACK `5cf4427`
  achow101:
    ACK 5cf4427
  pinheadmz:
    ACK 5cf4427

Tree-SHA512: 8f0e4fb2c3e0f84fac5dbc4dda87973276242b0f628034272a7f3e45434c1e17dd1b26a37edfb302dcaf380dbfe98b0417391ace5e0ac9720155d8fba702031e
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 b1269fe

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