backport: bitcoin#22766, #22972, #23305, #23332, #23338, #23371, #23410, #23482, partial #23311

[knst]

What was done?

Just regular backports from Bitcoin Core v23.

How Has This Been Tested?

Run unit & functional tests

Unit test for coinselector is indeed much faster:

   $ time src/test/test_dash -t '*coinselector*'
    Running 4 test cases...

    *** No errors detected

    real    0m31,104s
    user    0m30,086s
    sys     0m0,885s

    to

    real    0m5,663s
    user    0m5,154s
    sys     0m0,385s

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 (for repository code-owners and collaborators only)

[coderabbitai]

Walkthrough

This pull request introduces multiple modifications and additions across the Bitcoin Core codebase and its test framework. Documentation updates include new Bitcoin Improvement Proposal (BIP) entries (BIPs 380–386) and the addition of a section on Bitcoin Core fuzzing. Across several source files, command-line argument handling has been revised to permit more flexible input types while imposing restrictions (such as disallowing negative values) on certain options. Wallet-related code has been updated by switching from pointer usage to references for transaction handling, and error messages have been modified to dynamically reference the package name instead of hardcoded identifiers. Further changes include adjustments to conditional compilation for wallet database selection and the introduction of a new utility function for multisignature script creation within the test framework. Overall, the diff standardizes argument parsing, refines error messaging, and modernizes code constructs for enhanced internal consistency.

---

📜 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 32aecb0 and 636ba5a.

📒 Files selected for processing (1)

• test/functional/wallet_multiwallet.py (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• test/functional/wallet_multiwallet.py

⏰ Context from checks skipped due to timeout of 90000ms (1)

• GitHub Check: Build container / Build container

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (4)

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

44-52: LGTM! Well-structured multisig script utility.

The implementation is clean and includes proper validation. The use of encode_op_n for k and n values is the correct approach.

Consider adding input validation for empty key list:

 def keys_to_multisig_script(keys, *, k=None):
+    if not keys:
+        raise ValueError("At least one key is required")
     n = len(keys)
     if k is None:  # n-of-n multisig by default
         k = n

test/functional/mempool_limit.py (1)

67-69: Clarify the comment about transaction size and fee calculation.

The comment about vsize and fee rate calculation is unclear. It mentions "65k" but doesn't explain how this relates to the multiplier of 130.

Consider updating the comment to be more precise:

-        # The tx has an approx. vsize of 65k, i.e. multiplying the previous fee rate (in sats/kvB)
-        # by 130 should result in a fee that corresponds to 2x of that fee rate
+        # Calculate base fee by multiplying relay fee by 130 to ensure higher priority
+        # for subsequent transactions in the mempool

src/chainparams.cpp (2)

1386-1386: LGTM! Consider adding a maximum value check.

The change to disallow negative values for -powtargetspacing is a good improvement to input validation. However, consider also adding a maximum value check to prevent unreasonably large values that could affect block generation timing.

---

1314-1316: Improve the error message for invalid powTargetSpacing values.

The current error message could be more descriptive to help users understand the valid range of values.

Apply this diff to improve the error message:

-        throw std::runtime_error(strprintf("Invalid value of powTargetSpacing (%s)", strPowTargetSpacing));
+        throw std::runtime_error(strprintf("Invalid value of powTargetSpacing (%s). Value must be greater than 0", strPowTargetSpacing));

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d9704d3 and 3a2cc47b0bfcb1943701455c06d35e364b57924a.

📒 Files selected for processing (26)

• doc/bips.md (1 hunks)
• doc/fuzzing.md (1 hunks)
• src/bench/bench_bitcoin.cpp (1 hunks)
• src/bitcoin-cli.cpp (2 hunks)
• src/bitcoin-wallet.cpp (1 hunks)
• src/chainparams.cpp (1 hunks)
• src/init.cpp (6 hunks)
• src/test/getarg_tests.cpp (1 hunks)
• src/util/system.cpp (0 hunks)
• src/util/system.h (1 hunks)
• src/wallet/bdb.cpp (1 hunks)
• src/wallet/init.cpp (1 hunks)
• src/wallet/sqlite.cpp (1 hunks)
• src/wallet/test/coinselector_tests.cpp (1 hunks)
• src/wallet/transaction.h (1 hunks)
• src/wallet/wallet.cpp (3 hunks)
• src/wallet/wallet.h (1 hunks)
• src/wallet/walletdb.cpp (1 hunks)
• test/functional/feature_filelock.py (1 hunks)
• test/functional/mempool_accept.py (2 hunks)
• test/functional/mempool_limit.py (3 hunks)
• test/functional/rpc_blockchain.py (4 hunks)
• test/functional/test_framework/script_util.py (2 hunks)
• test/functional/test_framework/wallet_util.py (2 hunks)
• test/functional/tool_wallet.py (1 hunks)
• test/functional/wallet_multiwallet.py (2 hunks)

💤 Files with no reviewable changes (1)

• src/util/system.cpp

✅ Files skipped from review due to trivial changes (3)

• src/wallet/bdb.cpp
• doc/fuzzing.md
• src/wallet/wallet.cpp

🧰 Additional context used

🪛 Cppcheck (2.10-2)

src/wallet/walletdb.cpp

[error] 1193-1193: Syntax error in #elif

(preprocessorErrorDirective)

🪛 LanguageTool

doc/bips.md

[uncategorized] ~53-~53: Loose punctuation mark.
Context: ...oin/bips/blob/master/bip-0386.mediawiki): tr() Output Script Descriptors are impl...

(UNLIKELY_OPENING_PUNCTUATION)

⏰ Context from checks skipped due to timeout of 90000ms (4)

• GitHub Check: win64-build / Build source
• GitHub Check: linux64_multiprocess-build / Build source
• GitHub Check: linux64_tsan-build / Build source
• GitHub Check: linux64_ubsan-build / Build source

🔇 Additional comments (23)

test/functional/feature_filelock.py (1)

38-38: LGTM! Good improvement to error message.

The change to use self.config['environment']['PACKAGE_NAME'] instead of a hardcoded application name makes the error message more maintainable and reusable across different implementations.

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

65-65: LGTM! Good refactoring using the new utility.

The change to use keys_to_multisig_script simplifies the code and improves maintainability by removing direct dependencies on operation codes.

test/functional/mempool_limit.py (1)

32-40: Verify fee calculation logic.

The change from fee rate to absolute fee has implications:

1. Setting fee_rate=0 but subtracting an absolute fee might be confusing.
2. The assertion only checks the base fee but not the effective fee rate.

Consider:

1. Using fee rate consistently throughout the code.
2. Adding an assertion to verify the effective fee rate.

-    def send_large_txs(self, node, miniwallet, txouts, fee, tx_batch_size):
+    def send_large_txs(self, node, miniwallet, txouts, target_fee, tx_batch_size):
         for _ in range(tx_batch_size):
-            tx = miniwallet.create_self_transfer(from_node=node, fee_rate=0, mempool_valid=False)['tx']
+            tx = miniwallet.create_self_transfer(from_node=node, fee=target_fee, mempool_valid=False)['tx']
             for txout in txouts:
                 tx.vout.append(txout)
-            tx.vout[0].nValue -= int(fee * COIN)
             res = node.testmempoolaccept([tx.serialize().hex()])[0]
-            assert_equal(res['fees']['base'], fee)
+            assert_equal(res['fees']['base'], target_fee)
+            # Verify effective fee rate
+            tx_vsize = res['vsize']
+            effective_fee_rate = (target_fee * COIN) / tx_vsize
+            assert_greater_than(effective_fee_rate, relayfee)

src/bench/bench_bitcoin.cpp (2)

30-30: LGTM! Enhanced flexibility for -list argument.

The change from ALLOW_BOOL to ALLOW_ANY provides more flexibility in how the argument can be specified.

---

31-31: LGTM! Added safeguard against negative time values.

The addition of DISALLOW_NEGATION for -min_time is a good defensive programming practice as negative time values would be invalid.

src/bitcoin-wallet.cpp (2)

38-38: LGTM! Added safeguard for -dumpfile argument.

The addition of DISALLOW_NEGATION for -dumpfile is appropriate as negating a file path would be meaningless.

---

40-40: LGTM! Enhanced flexibility for -descriptors argument.

The change from ALLOW_BOOL to ALLOW_ANY provides more flexibility in how the argument can be specified.

test/functional/mempool_accept.py (1)

276-276: LGTM! Improved code maintainability using helper function.

Using keys_to_multisig_script instead of direct script construction improves code maintainability and standardizes multisig script handling across the codebase.

src/wallet/transaction.h (1)

196-197: LGTM! Improved code clarity with descriptive parameter names.

The constructor parameters have been renamed to be more descriptive (e.g., block_height instead of b), which enhances code readability and maintainability.

src/wallet/init.cpp (1)

114-114:

Details

✅ Verification successful

LGTM! Argument type updated for consistency.

The change from ALLOW_BOOL to ALLOW_ANY aligns with the broader standardization of argument handling across the codebase. The argument remains properly guarded by the USE_SQLITE macro and is marked as DEBUG_ONLY.

Let's verify the usage of this argument:

---

🏁 Script executed:

#!/bin/bash
# Description: Check how the -unsafesqlitesync argument is used in the codebase.

# Search for usage of the argument
rg -A 5 $'unsafesqlitesync'

Length of output: 3423

---

LGTM – Verified Consistency across the Codebase

The updated argument type (ALLOW_ANY) for -unsafesqlitesync in src/wallet/init.cpp has been cross-verified. Its usage in test files (e.g., in wallet_tests.cpp and test framework scripts) and in the SQLite-related logic (in src/wallet/sqlite.cpp) confirms that the changes are applied consistently. The argument remains properly guarded (including via hidden arguments based on preprocessor conditions) and is used as intended in debug/test contexts.

src/test/getarg_tests.cpp (1)

427-428: LGTM! Test cases updated for consistency.

The test cases have been updated to use ALLOW_ANY instead of ALLOW_BOOL, aligning with the broader changes to argument handling. The test assertions remain unchanged, indicating that the expected behavior is preserved.

src/util/system.h (1)

188-191: LGTM! Commented out unimplemented argument types.

The commented-out flags were draft implementations from PR bitcoin#16545. The comments clearly indicate their status, helping prevent confusion about available argument types.

src/wallet/sqlite.cpp (1)

247-247: LGTM! Error message updated to use package name dynamically.

The error message now uses the PACKAGE_NAME macro instead of a hardcoded value, making it more maintainable and adaptable to different package names.

test/functional/rpc_blockchain.py (3)

71-71: LGTM! Initializing MiniWallet for test wallet operations.

The initialization of MiniWallet provides a consistent way to manage wallet operations throughout the test.

---

102-102: LGTM! Using wallet for block generation.

Replaced generatetoaddress with generate using the wallet instance, which is a cleaner approach.

---

394-394: LGTM! Consistent use of wallet for block generation.

Block generation calls consistently use the wallet instance, maintaining code uniformity.

Also applies to: 399-399

test/functional/wallet_multiwallet.py (1)

203-203: LGTM! Improved error message clarity with dynamic package name.

Error messages now use the actual package name from configuration, making them more accurate and maintainable.

Also applies to: 304-304

test/functional/tool_wallet.py (1)

195-195: LGTM! Consistent error message improvement with dynamic package name.

Error message now uses the package name from configuration, maintaining consistency with other wallet-related error messages.

src/wallet/test/coinselector_tests.cpp (1)

77-89: LGTM! Improved transaction handling in coin selector tests.

The changes enhance transaction handling by:

1. Calculating transaction hash before wallet operations
2. Using proper locking for thread safety
3. Improving wallet transaction state management
4. Setting cache flags appropriately

src/wallet/wallet.h (1)

312-315: LGTM! Documentation updates improve clarity.

The comment updates accurately reflect the parameter name changes from pIndex to confirm.block_* and from posInBlock to block_hash, making the documentation more consistent with the implementation.

src/bitcoin-cli.cpp (1)

87-87: LGTM! Improved argument handling by disallowing negation.

The addition of ArgsManager::DISALLOW_NEGATION flag for -color and -rpcwaittimeout arguments is a good practice because:

1. For -color, negation would be confusing since it accepts specific values (always/auto/never)
2. For -rpcwaittimeout, negation wouldn't make sense for a timeout value

Also applies to: 97-97

src/init.cpp (1)

556-556: Command-line arguments updated to allow any value type.

The following command-line arguments have been updated to use ArgsManager::ALLOW_ANY instead of ArgsManager::ALLOW_BOOL:

• -dnsseed: Query for peer addresses via DNS lookup
• -fixedseeds: Allow fixed seeds if DNS seeds don't provide peers
• -networkactive: Enable all P2P network activity
• -natpmp: Use NAT-PMP to map the listening port
• -capturemessages: Capture all P2P messages to disk
• -daemon and -daemonwait: Run in the background as a daemon

This change provides more flexibility in argument handling while maintaining backward compatibility.

Also applies to: 558-558, 584-584, 598-598, 706-706, 765-766

doc/bips.md (1)

46-53: Documentation updated with new BIP implementations.

The documentation has been enhanced with the addition of BIPs 380-386, which cover Output Script Descriptors and Script Expressions. The entries provide clear implementation details and reference the relevant pull requests.

🧰 Tools

🪛 LanguageTool

[uncategorized] ~53-~53: Loose punctuation mark.
Context: ...oin/bips/blob/master/bip-0386.mediawiki): tr() Output Script Descriptors are impl...

(UNLIKELY_OPENING_PUNCTUATION)

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

doc/bips.md (1)

53-53: Review Punctuation for the BIP 386 Entry
Static analysis has flagged a loose punctuation mark on this line. Consider revisiting the punctuation (for example, checking the usage of the colon after the hyperlink) so it aligns with the consistency of other BIP entries in the document.

🧰 Tools

🪛 LanguageTool

[uncategorized] ~53-~53: Loose punctuation mark.
Context: ...oin/bips/blob/master/bip-0386.mediawiki): tr() Output Script Descriptors are impl...

(UNLIKELY_OPENING_PUNCTUATION)

test/functional/mempool_limit.py (1)

67-69: Verify the fee calculation constant.

The comment explains that 130 is based on a tx size of 65k, but this seems like a magic number. Consider adding more detailed documentation about how this number was derived.

-        # The tx has an approx. vsize of 65k, i.e. multiplying the previous fee rate (in sats/kvB)
-        # by 130 should result in a fee that corresponds to 2x of that fee rate
-        base_fee = relayfee * 130
+        # The tx has an approx. vsize of 65k bytes. To achieve a fee rate that is 2x the previous rate:
+        # - Previous rate is in sats/kvB (sats per kilobyte)
+        # - 65k bytes = 65 kB
+        # - 2x rate multiplier
+        # Therefore: 65 * 2 ≈ 130
+        base_fee = relayfee * 130  # Multiply by 130 to achieve 2x fee rate for 65kB tx

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3a2cc47b0bfcb1943701455c06d35e364b57924a and 32aecb0.

📒 Files selected for processing (26)

• doc/bips.md (1 hunks)
• doc/fuzzing.md (1 hunks)
• src/bench/bench_bitcoin.cpp (1 hunks)
• src/bitcoin-cli.cpp (2 hunks)
• src/bitcoin-wallet.cpp (1 hunks)
• src/chainparams.cpp (1 hunks)
• src/init.cpp (6 hunks)
• src/test/getarg_tests.cpp (1 hunks)
• src/util/system.cpp (0 hunks)
• src/util/system.h (1 hunks)
• src/wallet/bdb.cpp (1 hunks)
• src/wallet/init.cpp (1 hunks)
• src/wallet/sqlite.cpp (1 hunks)
• src/wallet/test/coinselector_tests.cpp (1 hunks)
• src/wallet/transaction.h (1 hunks)
• src/wallet/wallet.cpp (3 hunks)
• src/wallet/wallet.h (1 hunks)
• src/wallet/walletdb.cpp (1 hunks)
• test/functional/feature_filelock.py (1 hunks)
• test/functional/mempool_accept.py (2 hunks)
• test/functional/mempool_limit.py (3 hunks)
• test/functional/rpc_blockchain.py (4 hunks)
• test/functional/test_framework/script_util.py (2 hunks)
• test/functional/test_framework/wallet_util.py (2 hunks)
• test/functional/tool_wallet.py (1 hunks)
• test/functional/wallet_multiwallet.py (2 hunks)

💤 Files with no reviewable changes (1)

• src/util/system.cpp

🚧 Files skipped from review as they are similar to previous changes (20)

• src/wallet/sqlite.cpp
• doc/fuzzing.md
• src/wallet/walletdb.cpp
• test/functional/feature_filelock.py
• test/functional/test_framework/wallet_util.py
• src/bench/bench_bitcoin.cpp
• src/bitcoin-wallet.cpp
• src/wallet/transaction.h
• src/wallet/init.cpp
• test/functional/tool_wallet.py
• test/functional/rpc_blockchain.py
• src/test/getarg_tests.cpp
• src/util/system.h
• src/wallet/bdb.cpp
• test/functional/wallet_multiwallet.py
• src/wallet/wallet.h
• src/bitcoin-cli.cpp
• src/wallet/wallet.cpp
• src/chainparams.cpp
• src/init.cpp

🧰 Additional context used

🪛 LanguageTool

doc/bips.md

[uncategorized] ~53-~53: Loose punctuation mark.
Context: ...oin/bips/blob/master/bip-0386.mediawiki): tr() Output Script Descriptors are impl...

(UNLIKELY_OPENING_PUNCTUATION)

⏰ Context from checks skipped due to timeout of 90000ms (1)

• GitHub Check: Build container / Build container

🔇 Additional comments (7)

src/wallet/test/coinselector_tests.cpp (3)

77-82: LGTM! Well-implemented transaction creation and insertion.

The code follows best practices by using RAII-style locking, modern C++ features, and proper error checking with assertions.

---

85-86: LGTM! Correct implementation of transaction amount and cache state.

The code properly simulates a self-sent transaction by setting the DEBIT amount and updating the cache state.

---

88-88: LGTM! Well-structured COutput object creation.

The code properly initializes the COutput object with all necessary parameters, contributing to the improved coinselector test performance mentioned in the PR objectives.

doc/bips.md (1)

46-52: New BIP Entries for BIPs 380–385 Added
The new entries for BIPs 380 through 385 are clearly added and follow the established listing style. They provide proper hyperlinks and reference the appropriate pull request, making it easy for readers to verify the details. Please double-check that the version information (i.e., v0.17.0) and PR references are still accurate within the broader context of the documentation.

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

44-52: LGTM! Well-structured multisig script creation function.

The function is well-implemented with proper validation and error handling:

• Handles n-of-n multisig by default
• Validates that k <= n
• Checks each key for validity
• Uses proper encoding for script operations

test/functional/mempool_limit.py (1)

32-40: LGTM! Improved transaction fee handling.

The changes make the fee handling more explicit and accurate:

• Clear parameter naming (fee instead of fee_rate)
• Proper separation of transaction creation and fee adjustment

test/functional/mempool_accept.py (1)

276-276: LGTM! Good use of the new utility function.

The change improves code clarity by using the dedicated keys_to_multisig_script function instead of direct script construction.

[PastaPastaPasta]

Looks fine; but CI is mad: https://gitlab.com/dashpay/dash/-/jobs/9158414108#L2624

[UdjinM6]

utACK 636ba5a