Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backport: bitcoin#22539, #22918, #23093, #23149, #23212, #23220, #23268, #23287, #23314, #23323 #6577

Draft
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

knst
Copy link
Collaborator

@knst knst commented Feb 14, 2025

What was done?

Just regular backports from Bitcoin Core v23.

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

fanquake and others added 2 commits February 14, 2025 00:41
38fd709 build: make --enable-werror just -Werror (fanquake)

Pull request description:

  No longer special case a set of warnings, to make up our own -Werror,
  just use -Werror outright. This shouldn't really have any effect on
  existing builders, who were already using `--enable-werror`, and is more
  inline with what they would expect `--enable-werror` to be, which is
  erroring on any/all warnings.

  We keep `-Wno-error=return-type` because we know that is broken when using
  mingw-w64. It should only be applied when cross-compiling for Windows.

  Similar to the change in bitcoin#20544, but with (hopefully) less work-arounds,
  and other bundled changes. A step towards some configure "cleanups".

ACKs for top commit:
  hebasto:
    ACK 38fd709 (also see bitcoin#23149 (comment)), tested:
  MarcoFalke:
    Concept ACK 38fd709

Tree-SHA512: 37f1857d9408442cab63e40f9280427b73e09cdf03146b19c1339d7e44abd78e93df7f270ca1da0e83b79343cd3ea915f7b9e4e347488b5bc5ceaaa7540e5926
…ased tests

d752454 build: explicitly disable libsecp256k1 openssl based tests (fanquake)

Pull request description:

  These tests are failing when run against OpenSSL 3, and have been
  removed upstream, bitcoin-core/secp256k1#983, so
  disabled them for now to avoid `make check` failures.

  Note that this will also remove warning output from our build, due to
  the use of deprecated OpenSSL API functions. See bitcoin#23048.

ACKs for top commit:
  MarcoFalke:
    cr ACK d752454
  elichai:
    Code review ACK d752454

Tree-SHA512: a3805b4123ec49aaf21378066d86be382d11a022a7530682c2d8c0e756e785f32f18bea6fe73b1eca73f70bc3aee9166c391a9bf6adc0e450ebb0ce0bcf5a45e
@knst knst added this to the 23 milestone Feb 14, 2025
@knst knst force-pushed the bp-v23-p6 branch 2 times, most recently from f50d445 to 17e980b Compare February 14, 2025 13:50
@knst knst marked this pull request as ready for review February 14, 2025 13:50
Copy link

coderabbitai bot commented Feb 14, 2025

Walkthrough

The pull request implements a series of coordinated updates across configuration, core functionality, RPC interfaces, documentation, and tests. The CI configuration is modified to update the ARM container from Debian buster to bullseye. In the build configuration, adjustments are made to the handling of compiler warnings and error flags. Several RPC and REST API methods now use a new TxVerbosity enumeration instead of a boolean flag, affecting functions such as blockToJSON, TxToUniv, and related calls, and updating corresponding documentation. Release notes and the release process documentation have been enhanced, including the addition of a new RPC command (newkeypool) and checklist items for backports and branch management. Finally, multiple functional tests and wallet test files are refactored to accommodate the new transaction verbosity handling and keypool management, and minor linting argument changes are also included.

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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
🪧 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.

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 (4)
test/functional/wallet_keypool_hd.py (1)

164-177: LGTM! Well-structured test for newkeypool functionality.

The test effectively verifies that newkeypool flushes the keypool and generates new addresses with correct keypath increments. The conditional execution for non-descriptor wallets is appropriate.

Consider adding a comment explaining why the keypath index is expected to increment by 100 to improve test maintainability:

-            # The new keypath index should be 100 more than the old one
+            # The new keypath index should be 100 more than the old one because newkeypool
+            # flushes the entire keypool (default size 100) and generates new keys
test/functional/feature_fee_estimation.py (1)

279-303: Well-structured test flow with clear logging.

The refactored run_test method provides a clear sequence of operations with informative log messages.

Consider adding a brief comment before each major test section to explain the test's purpose:

 def run_test(self):
     self.log.info("This test is time consuming, please be patient")
     self.log.info("Splitting inputs so we can generate tx's")
 
+    # Initialize test setup by splitting coinbase outputs
     self.start_node(0)
     self.initial_split(self.nodes[0])
     self.log.info("Finished splitting")
 
+    # Connect nodes after splitting to avoid affecting fee estimates
     self.start_node(1)
     self.start_node(2)
     self.connect_nodes(1, 0)
test/functional/wallet_hd.py (1)

133-133: Improved transaction output retrieval.

The change to use gettransaction with verbose=True simplifies the code by getting decoded outputs in a single RPC call, rather than using separate gettransaction and decoderawtransaction calls.

Consider adding a brief comment explaining why this approach is preferred:

+# Use verbose=True to get decoded outputs directly instead of decoding raw transaction
 outs = self.nodes[1].gettransaction(txid=txid, verbose=True)['decoded']['vout']
doc/release-notes-22918.md (1)

7-7: Fix grammar in the documentation.

Change "if the spent coins was a coinbase" to "if the spent coins were a coinbase".

-  - `generated` - true if the spent coins was a coinbase.
+  - `generated` - true if the spent coins were a coinbase.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~7-~7: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...- generated - true if the spent coins was a coinbase. - height - value ...

(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 265d9b5 and 17e980b.

📒 Files selected for processing (23)
  • .cirrus.yml (1 hunks)
  • configure.ac (5 hunks)
  • doc/release-notes-22918.md (1 hunks)
  • doc/release-notes-23093.md (1 hunks)
  • doc/release-process.md (2 hunks)
  • src/bench/rpc_blockchain.cpp (2 hunks)
  • src/chainparams.cpp (3 hunks)
  • src/core_io.h (2 hunks)
  • src/core_write.cpp (5 hunks)
  • src/rest.cpp (3 hunks)
  • src/rpc/blockchain.cpp (4 hunks)
  • src/rpc/blockchain.h (2 hunks)
  • src/rpc/rawtransaction.cpp (1 hunks)
  • src/wallet/rpcwallet.cpp (3 hunks)
  • test/functional/feature_fee_estimation.py (2 hunks)
  • test/functional/interface_rest.py (1 hunks)
  • test/functional/rpc_blockchain.py (2 hunks)
  • test/functional/wallet_basic.py (1 hunks)
  • test/functional/wallet_create_tx.py (1 hunks)
  • test/functional/wallet_hd.py (1 hunks)
  • test/functional/wallet_importdescriptors.py (1 hunks)
  • test/functional/wallet_keypool_hd.py (1 hunks)
  • test/lint/lint-python.py (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • test/functional/wallet_importdescriptors.py
  • src/chainparams.cpp
🧰 Additional context used
🪛 LanguageTool
doc/release-notes-22918.md

[uncategorized] ~7-~7: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...- generated - true if the spent coins was a coinbase. - height - value ...

(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)

🪛 GitHub Actions: Check Potential Conflicts
test/functional/wallet_basic.py

[error] 1-1: Conflict detected in test/functional/wallet_basic.py with multiple pull requests.

configure.ac

[error] 1-1: Conflict detected in configure.ac with multiple pull requests.

src/wallet/rpcwallet.cpp

[error] 1-1: Conflict detected in src/wallet/rpcwallet.cpp with multiple pull requests.

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Build container / Build container
🔇 Additional comments (32)
src/bench/rpc_blockchain.cpp (1)

48-48: LGTM! Consistent use of new verbosity enum.

The changes correctly replace the boolean verbosity parameter with TxVerbosity::SHOW_DETAILS_AND_PREVOUT in both benchmark functions, maintaining the original functionality while aligning with the new enum-based approach.

Also applies to: 59-59

src/rpc/blockchain.h (1)

44-44: LGTM! Well-structured function signature updates.

The changes properly integrate the new TxVerbosity parameter in both function signatures. The default value of SHOW_DETAILS for TxToUniv maintains backward compatibility.

Also applies to: 53-53

src/core_io.h (2)

25-32: LGTM! Well-designed enum for transaction verbosity levels.

The TxVerbosity enum class is well-documented and provides a clear progression of verbosity levels from basic TXID to full details with prevouts.


59-59: LGTM! Function signature updated consistently.

The TxToUniv function signature is properly updated to use the new TxVerbosity parameter with a sensible default value.

test/functional/wallet_create_tx.py (1)

29-29: LGTM! Simplified transaction retrieval in tests.

The changes improve efficiency by using gettransaction with verbose=True instead of separate decode calls, while maintaining the same test functionality.

Also applies to: 35-35

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

127-127:

✅ Verification successful

Stricter type checking configuration enabled.

The removal of --ignore-missing-imports from mypy args will now cause the type checker to report errors for missing imports, which helps catch import-related issues earlier in development.

Let's verify if this change might affect any existing Python files:


🏁 Script executed:

#!/bin/bash
# Description: Check for any missing imports in Python files that might be affected by the stricter mypy configuration.

# Run mypy on Python files to check for missing imports
mypy --show-error-codes $(find test/functional -name "*.py") $(find contrib/devtools -name "*.py" ! -name "github-merge.py") 2>&1 | grep "Import"

Length of output: 10862


Stricter type checking enabled.
The removal of --ignore-missing-imports from the mypy arguments now enforces reporting of missing import errors, tightening our type checks. A verification run over the relevant Python files did not yield any missing import warnings, which indicates that our current codebase complies with this stricter configuration. Please keep an eye on new additions or modifications in the future that might introduce missing import issues.

test/functional/feature_fee_estimation.py (3)

218-220: Well-documented method extraction.

The initial_split method clearly describes its purpose of splitting coinbase UTxOs into smaller coins.


245-248: Clear method name and docstring.

The sanity_check_estimates_range method's purpose is well-documented, explaining both what it does and what it verifies.


273-278: Good test coverage for edge case.

The test_feerate_mempoolminfee method effectively tests fee rate estimation with a high minimum relay fee, including verification of the warning message.

src/core_write.cpp (2)

204-204: LGTM! Function signature change improves clarity.

The change from boolean include_addresses to TxVerbosity verbosity enhances the API by providing more granular control over transaction output verbosity.


219-274: LGTM! Improved variable naming and structured verbosity handling.

The changes enhance code clarity and maintainability:

  1. Renamed calculate_fee to have_undo to better reflect its purpose
  2. Added a well-structured switch statement for handling different verbosity levels
  3. Properly organized the prevout data inclusion based on verbosity
test/functional/interface_rest.py (1)

340-347: LGTM! Enhanced test coverage for transaction input data.

The new assertions properly verify:

  1. Presence of "prevout" in non-coinbase transactions
  2. Absence of "prevout" in coinbase transactions
  3. Correct value of "generated" field in prevout data
test/functional/rpc_blockchain.py (2)

457-490: LGTM! Well-structured helper functions for testing.

The new helper functions improve test maintainability by:

  1. Encapsulating fee verification logic
  2. Providing clear prevout data validation
  3. Including thorough value checks for inputs and outputs

491-523: LGTM! Comprehensive test coverage for verbosity levels.

The test cases properly verify:

  1. Fee exclusion in verbosity level 1
  2. Fee inclusion in verbosity levels 2 and 3
  3. Prevout data handling across all verbosity levels
  4. Behavior with pruned Undo data
src/rest.cpp (2)

289-293: LGTM! Improved REST API parameter handling.

The change from bool showTxDetails to TxVerbosity tx_verbosity aligns the REST API with the new verbosity control system.


360-368: LGTM! Clear verbosity level selection for endpoints.

The endpoints now use appropriate verbosity levels:

  1. rest_block_extended uses SHOW_DETAILS_AND_PREVOUT for full details
  2. rest_block_notxdetails uses SHOW_TXID for minimal information
test/functional/wallet_basic.py (1)

688-688:

✅ Verification successful

LGTM! Consistent with the codebase-wide transaction detail handling changes.

The change from decoderawtransaction to gettransaction(verbose=True) aligns with similar changes in other test files, providing a more structured way to retrieve transaction details.

Let's verify the consistency of this change across other test files:


🏁 Script executed:

#!/bin/bash
# Description: Check if other test files have been updated to use gettransaction with verbose=True

# Search for any remaining uses of decoderawtransaction in test files
echo "Checking for remaining decoderawtransaction usage in test files..."
rg "decoderawtransaction" "test/functional/"

# Search for the new pattern to confirm consistent usage
echo "Checking for gettransaction with verbose=True usage..."
rg "gettransaction.*verbose=True" "test/functional/"

Length of output: 8422


LGTM! Consistent with our ongoing refactoring for transaction detail handling.

  • The updated call in wallet_basic.py now uses gettransaction(txid=txid, verbose=True)['decoded'], matching the pattern already introduced in files like wallet_create_tx.py and wallet_hd.py.
  • While several other test files still use decoderawtransaction, this change is an incremental step toward a more structured and consistent transaction retrieval approach.
configure.ac (5)

335-335: LGTM! Improved clarity in -Werror description.

The help string for --enable-werror has been simplified and clarified.


463-468: LGTM! Improved handling of -Werror and -Wreturn-type flags.

The changes:

  1. Simplify ERROR_CXXFLAGS to use CXXFLAG_WERROR directly
  2. Fix -Wreturn-type handling for MinGW-w64 to work around a known bug

478-478: LGTM! Added -Wreorder warning and documented -Wsuggest-override GCC bug.

The changes:

  1. Add -Wreorder to catch potential member initialization order issues
  2. Document the GCC bug affecting -Wsuggest-override before version 9.2

Also applies to: 491-493


1920-1920: LGTM! Added --disable-openssl-tests to configure args.

Added --disable-openssl-tests to ac_configure_args for secp256k1 configuration.


1-1943:

❓ Verification inconclusive

Verify potential conflicts in configure.ac.

The pipeline indicates potential conflicts with other PRs. Please ensure this PR is rebased on the latest master branch.

Run the following to check for potential conflicts:


🏁 Script executed:

#!/bin/bash
# Description: Check for potential conflicts in configure.ac

# Get the current branch name
current_branch=$(git rev-parse --abbrev-ref HEAD)

# List all open PRs that modify configure.ac
echo "Checking open PRs that modify configure.ac..."
gh pr list --search "configure.ac" --state open

# Show potential conflicts
echo "Checking potential conflicts with current branch..."
git fetch origin master
git merge-base --is-ancestor origin/master $current_branch || echo "Branch needs rebase"

Length of output: 730


Rebase Required: Verify Potential Conflicts in configure.ac
The automated conflict check script suggests that your branch may be out-of-date with the latest master ("Branch needs rebase"). Note that the script failed to run in a proper git repository context, so manual verification is recommended. Please ensure your branch is rebased onto the latest master to avoid potential merge conflicts in configure.ac with other open PRs.

🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts

[error] 1-1: Conflict detected in configure.ac with multiple pull requests.

src/rpc/rawtransaction.cpp (1)

99-99:

✅ Verification successful

LGTM: Verbosity level enhancement improves code clarity.

The change from a boolean flag to TxVerbosity::SHOW_DETAILS enum value improves code readability and maintainability by making the verbosity level more explicit.

Let's verify that this change is consistent across the codebase:


🏁 Script executed:

#!/bin/bash
# Description: Check for any remaining boolean verbosity flags in TxToUniv calls
# that might have been missed during the migration to TxVerbosity enum.

# Search for TxToUniv calls
rg "TxToUniv.*true|TxToUniv.*false" --type cpp

Length of output: 1551


LGTM: TxVerbosity Enhancement Implemented Correctly

  • The updated call in src/rpc/rawtransaction.cpp now correctly passes TxVerbosity::SHOW_DETAILS as intended, which improves code clarity and maintainability.
  • Verification shows that this change is isolated to the appropriate call at line 99. Other instances using boolean flags (e.g., for include_addresses) belong to different overloads and remain unaffected by this update.
src/rpc/blockchain.cpp (3)

158-186: LGTM: Improved transaction verbosity handling with enum.

The refactoring from boolean to enum improves code readability and extensibility by:

  • Using a clear TxVerbosity enum instead of a boolean flag
  • Adding support for different verbosity levels through a switch statement
  • Properly handling undo data access for transaction details

1922-1924: LGTM: Updated TxToUniv signature.

The function signature has been updated to match the new verbosity enum parameter.


875-986:

❓ Verification inconclusive

Verify documentation update for new verbosity level.

The RPC help documentation has been updated to include the new verbosity level 3, but we should verify that all documentation, including external docs, reflects this change.


🏁 Script executed:

#!/bin/bash
# Search for any documentation files that might need updating
fd -e md -e rst -e txt . | xargs rg -l "getblock.*verbosity"

Length of output: 109


Documentation Update Required: Verify External Docs for New 'getblock' Verbosity Level

The internal RPC help text now properly documents the new verbosity level 3. However, our search shows that external documentation references for the getblock command (e.g., in doc/release-notes/dash/release-notes-0.15.0.md) should be manually verified to ensure they also reflect this update. Please review and update any external docs as needed to maintain consistency.

src/wallet/rpcwallet.cpp (3)

1750-1750: LGTM! Improved help text clarity.

The updated help text better explains the keypool size options by mentioning both the default value and the -keypool command line option.


4672-4672: LGTM! Proper integration of the new RPC command.

The newkeypool command is correctly added to the RPC command list in the "wallet" category.


1-4699: Resolve conflicts before merging.

The pipeline indicates conflicts in this file with other pull requests. Please rebase your changes on the latest master branch and resolve any conflicts.

🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts

[error] 1-1: Conflict detected in src/wallet/rpcwallet.cpp with multiple pull requests.

doc/release-notes-23093.md (1)

1-8: LGTM! Clear and concise documentation.

The release notes clearly document the new newkeypool RPC feature and its functionality.

doc/release-notes-22918.md (1)

3-10: LGTM! Comprehensive documentation of the new feature.

The documentation clearly explains the new verbose level 3 option and its output structure.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~7-~7: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...- generated - true if the spent coins was a coinbase. - height - value ...

(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)

doc/release-process.md (1)

9-9: LGTM! Valuable additions to the release process.

The new checklist items for reviewing backport labels and managing EOL branches enhance the release process documentation.

Also applies to: 237-237

Comment on lines 1789 to 1818
static RPCHelpMan newkeypool()
{
return RPCHelpMan{"newkeypool",
"\nEntirely clears and refills the keypool."+
HELP_REQUIRING_PASSPHRASE,
{},
RPCResult{RPCResult::Type::NONE, "", ""},
RPCExamples{
HelpExampleCli("newkeypool", "")
+ HelpExampleRpc("newkeypool", "")
},
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
{
std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
if (!pwallet) return NullUniValue;

LOCK(pwallet->cs_wallet);

LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*pwallet, true);
spk_man.NewKeyPool();

return NullUniValue;
},
};
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add essential checks for robustness.

The new newkeypool RPC method should include additional checks for robustness:

  1. Add explicit wallet unlock check using EnsureWalletIsUnlocked
  2. Add check for disabled private keys
  3. Add check for descriptor wallets

Apply this diff to add the checks:

 static RPCHelpMan newkeypool()
 {
     return RPCHelpMan{"newkeypool",
                 "\nEntirely clears and refills the keypool."+
             HELP_REQUIRING_PASSPHRASE,
                 {},
                 RPCResult{RPCResult::Type::NONE, "", ""},
                 RPCExamples{
             HelpExampleCli("newkeypool", "")
             + HelpExampleRpc("newkeypool", "")
                 },
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
     if (!pwallet) return NullUniValue;
 
+    if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
+        throw JSONRPCError(RPC_WALLET_ERROR, "Error: Private keys are disabled for this wallet");
+    }
+
+    if (pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
+        throw JSONRPCError(RPC_WALLET_ERROR, "Error: This wallet is a descriptor wallet. Use keypoolrefill instead.");
+    }
+
     LOCK(pwallet->cs_wallet);
 
+    EnsureWalletIsUnlocked(*pwallet);
+
     LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*pwallet, true);
     spk_man.NewKeyPool();
 
     return NullUniValue;
 },
     };
 }
📝 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
static RPCHelpMan newkeypool()
{
return RPCHelpMan{"newkeypool",
"\nEntirely clears and refills the keypool."+
HELP_REQUIRING_PASSPHRASE,
{},
RPCResult{RPCResult::Type::NONE, "", ""},
RPCExamples{
HelpExampleCli("newkeypool", "")
+ HelpExampleRpc("newkeypool", "")
},
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
{
std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
if (!pwallet) return NullUniValue;
LOCK(pwallet->cs_wallet);
LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*pwallet, true);
spk_man.NewKeyPool();
return NullUniValue;
},
};
}
static RPCHelpMan newkeypool()
{
return RPCHelpMan{"newkeypool",
"\nEntirely clears and refills the keypool."+
HELP_REQUIRING_PASSPHRASE,
{},
RPCResult{RPCResult::Type::NONE, "", ""},
RPCExamples{
HelpExampleCli("newkeypool", "")
+ HelpExampleRpc("newkeypool", "")
},
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
{
std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
if (!pwallet) return NullUniValue;
if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Error: Private keys are disabled for this wallet");
}
if (pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Error: This wallet is a descriptor wallet. Use keypoolrefill instead.");
}
LOCK(pwallet->cs_wallet);
EnsureWalletIsUnlocked(*pwallet);
LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*pwallet, true);
spk_man.NewKeyPool();
return NullUniValue;
},
};
}

.cirrus.yml Outdated
Comment on lines 62 to 65
name: 'ARM [unit tests, no functional tests] [bullseye]'
<< : *GLOBAL_TASK_TEMPLATE
arm_container:
image: debian:buster
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix inconsistency between task name and image specification.

The task name references 'bullseye' but the image still specifies 'buster'. This inconsistency should be resolved.

  name: 'ARM [unit tests, no functional tests] [bullseye]'
  << : *GLOBAL_TASK_TEMPLATE
  arm_container:
-    image: debian:buster
+    image: debian:bullseye
📝 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
name: 'ARM [unit tests, no functional tests] [bullseye]'
<< : *GLOBAL_TASK_TEMPLATE
arm_container:
image: debian:buster
name: 'ARM [unit tests, no functional tests] [bullseye]'
<< : *GLOBAL_TASK_TEMPLATE
arm_container:
image: debian:bullseye
🧰 Tools
🪛 YAMLlint (1.35.1)

[warning] 63-63: too many spaces before colon

(colons)

@knst knst marked this pull request as draft February 14, 2025 14:27
fanquake and others added 8 commits February 14, 2025 21:32
ca2c313 Use absolute FQDN for DNS seed domains (Prayank)

Pull request description:

  Fixes bitcoin#23193 by using absolute FQDN for domains used by DNS seeds.

  It improves security and should not break anything based on my research and testing. Few things about absolute FQDN are shared in https://superuser.com/questions/1467958/why-does-putting-a-dot-after-the-url-remove-login-information

  Master branch:

  ```
  DNS seed x9.dnsseed.bitcoin.dashjr.org. responded with IP 127.0.0.1
  ```

  PR branch:

  ```
  DNS seed x9.dnsseed.bitcoin.dashjr.org. responded with IP 159.89.108.149
  ```

  Reviewers can follow the steps mentioned in Issue to test: bitcoin#23193 (comment)

ACKs for top commit:
  practicalswift:
    cr ACK ca2c313
  laanwj:
    code review ACK ca2c313
  promag:
    Code review ACK ca2c313.

Tree-SHA512: 9818227332282a78c45b4470c2fc80bf899ed78aed76644ebf014e0fff1b139402ea023acdea162363a478b6f6613dbf1da57e214d2240ea0f04310473f57cca
a46f71b lint: enable mypy checking for missing imports (josibake)
22e6526 lint mypy 0.910 (fanquake)
6ae9c2e lint: install pyzmq (22.3.0) into linter environment (josibake)
b93e229 doc: remove pointlessly duplicated linter version / install info (fanquake)

Pull request description:

  This is bitcoin#22844 with issues addressed, and two additional commits. One to move to the latest mypy version in the CI, and another to remove what is just pointless duplication from the test README. There's no need to relist package versions and install instructions (meaning 2x the work when changing anything), when you can just link to `04_install.sh` which has the versions used and pip invocations to install them.

ACKs for top commit:
  practicalswift:
    cr ACK a46f71b

Tree-SHA512: 2900dea3901d03a846dc1ea912f217d152e803845516c7d941745ec1291d145590cd4bf2ddc497f7cf628119ba9905d7b1531836062aa85b384e39cf436f62c6
…itcoin#21245 modified)

5c34507 core_write: Rename calculate_fee to have_undo for clarity (fyquah)
8edf620 release-notes: Add release note about getblock verbosity level 3. (fyquah)
459104b rest: Add test for prevout fields in getblock (fyquah)
4330af6 rpc: Add test for level 3 verbosity getblock rpc call. (fyquah)
51dbc16 rpc: Add level 3 verbosity to getblock RPC call. (fyquah)
3cc9534 rpc: Replace boolean argument for tx details with enum class. (fyquah)

Pull request description:

  Author of bitcoin#21245 expressed [time issues](bitcoin#21245 (comment)) in the original PR. Given that bitcoin#21245 has received a lot of review*, I have decided to open this new pull request with [modifications required to get ACK from luke-jr ](bitcoin#21245 (comment)) and a few nits of mine.

  ### Original PR description

  > Display the prevout in transaction inputs when calling getblock level 3 verbosity. This PR affects the existing `/rest/block` API by adding a `prevout` fields to tx inputs. This is mentioned in the change to the release notes.
  >
  > I added some functional tests that
  >
  >     * checks that the RPC call still works when TxUndo can't be found
  >
  >     * Doesn't display the "value" or "scriptPubKey" of the previous output when at a lower verbosity level
  >
  >
  > This "completes" the issue bitcoin#18771

  ### Possible improvements

  * kiminuo@b0bf4f2 - I can include even this commit to this PR if deemed useful or I can leave it for a follow-up PR. See bitcoin#21245 (comment) for more context.

  ### Examples

  Examples of the `getblock` output with various verbose levels. Note that `000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5` contains only 2 transactions.

  #### Verbose level 0

  ```bash
  ./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 0
  ```

  ##### Verbose level 1

  ```bash
  ./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 1
  ```

  ##### Verbose level 2

  ```bash
  ./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 2
  ```

  ##### Verbose level 3

  ```bash
  ./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 3
  ```

  #### REST

  ```bash
  curl -H "content-type:text/plain;" http://127.0.0.1:18332/rest/block/000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5.json
  ```

  <sub>* ... and my everyday obsessive checking of my email inbox whether the PR moves forward.</sub>

  Edit laanwj: Removed at symbol from message, and large example output to prevent it from all ending up in the commit message.

ACKs for top commit:
  0xB10C:
    ACK 5c34507
  meshcollider:
    utACK 5c34507
  theStack:
    ACK 5c34507 👘
  promag:
    Concept ACK 5c34507

Tree-SHA512: bbff120d8fd76e617b723b102b0c606e0d8eb27f21c631d5f4cdab0892137c4bc7c65b1df144993405f942c91be47a26e80480102af55bff22621c19f518aea3
…ction` RPC call

130ee48 test: get and decode tx with a single `gettransaction` RPC call (Sebastian Falbesoner)

Pull request description:

  Rather than subsequently calling `gettransaction` and `decoderawtransaction` to get the decoded information  for a specific tx-id, we can simply use the verbose version of `gettransaction`, which returns this in a 'decoded' key. I.e.

  ```
  node.decoderawtransaction(node.gettransaction(txid)['hex'])
  ```

  can simply be replaced by:

  ```
  node.gettransaction(txid=txid, verbose=True)['decoded']
  ```

  Rationale: shorter code, shorter test logs, less RPC calls.

ACKs for top commit:
  stratospher:
    tested ACK 130ee48
  amadeuszpawlik:
    tACK 130ee48
  lsilva01:
    Tested ACK 130ee48 on Ubuntu 20.04.
  shaavan:
    ACK 130ee48

Tree-SHA512: cf0bd26e1e21b8022fb8062857906e0706f0ee32d3277f985c461e2519405afe445ab005f5f763fb268c7b4d6e48b2d47eda7af8621b3bce67cece8dfc9bc153
fa38d98 doc: Add note on deleting past-EOL release branches (MarcoFalke)

Pull request description:

  This is being done for years now, but wasn't documented.

  Some reasons to do it:
  * Backports to those branches are unlikely to be tested both on CI (since it is often fragile and broken for stale branches) and by users (since those users likely don't exist). If a user exists, they are better off backporting any fixes they need from the last still-supported branch and test them on their own infrastructure.
  * Community support of those branches is still possible, though this will need to be done in another project to relieve the burden on this project.
  * All release tags will remain, so no historic code is lost.

ACKs for top commit:
  hebasto:
    ACK fa38d98
  fanquake:
    ACK fa38d98 - I think this is fine as-is.

Tree-SHA512: caa714af541a6902925c89cc6a896b125f61bd77e901c5d384d84b34def2ee654bdae9f3e995001154c29672f60d2b689d0ff92d345666564fd5aa321a5b3fe7
3b61372 Add release notes for fee est with replacement txs (Antoine Poinsot)
4556406 qa: test fee estimation with replacement transactions (Antoine Poinsot)
053415b qa: split run_test into smaller parts (Antoine Poinsot)
06c5ce9 Re-include RBF replacement txs in fee estimation (Antoine Poinsot)

Pull request description:

  This effectively reverts bitcoin#9519.

  RBF is now largely in use on the network (signaled for by around 20% of
  all transactions on average) and replacement logic is implemented in
  most end-user wallets. The rate of replaced transactions is also
  expected to rise as fee-bumping techniques are being developed for
  pre-signed transaction ("L2") protocols.

ACKs for top commit:
  prayank23:
    reACK bitcoin@3b61372
  Zero-1729:
    re-ACK 3b61372
  benthecarman:
    reACK 3b61372
  glozow:
    ACK 3b61372
  theStack:
    re-ACK 3b61372 🍪

Tree-SHA512: a6146d15c80ff4ba9249314b0ef953a66a15673e61b8f98979642814f1b169b5695e330e3ee069fa9a7e4d1f8aa10e1dcb7f9aa79181cea5a4c4dbcaf5483023

Revert "Merge bitcoin#22539: Re-include RBF replacement txs in fee estimation"

This reverts commit 7f5fa35.

qa: split run_test into smaller parts

Let's not have run_test get into a giant function as we add more tests

Signed-off-by: Antoine Poinsot <[email protected]>
…en upgrading non-HD to HD

6531599 test: Add check that newkeypool flushes change addresses too (Samuel Dobson)
84fa19c Add release notes for keypool flush changes (Samuel Dobson)
f9603ee Add test for flushing keypool with newkeypool (Samuel Dobson)
6f6f7bb Make legacy wallet upgrades from non-HD to HD always flush the keypool (Samuel Dobson)
2434b10 Fix outdated keypool size default (Samuel Dobson)
22cc797 Add newkeypool RPC to flush the keypool (Samuel Dobson)

Pull request description:

  This PR makes two main changes:
  1) Adds a new RPC `newkeypool` which will entirely flush and refill the keypool.
  2) When upgradewallet is called on old, non-HD wallets upgrading them to HD, we now always flush the keypool and generate a new one, to immediately start using the HD generated keys.

  This PR is motivated by a number of users with old, pre-compressed-key wallets upgrading them and being confused about why they still can't generate p2sh-segwit or bech32 addresses -- this is due to uncompressed keys remaining in the keypool post-upgrade and being illegal in these newer address formats. There is currently no easy way to flush the keypool other than to call `getnewaddress` a hundred/thousand times or an ugly hack of using a `sethdseed` call.

ACKs for top commit:
  laanwj:
    re-ACK 6531599
  meshcollider:
    Added new commit 6531599 to avoid invalidating previous ACKs.
  instagibbs:
    ACK bitcoin@6531599

Tree-SHA512: 50c79c5d42dd27ab0ecdbfdc4071fdaa1b2dbb2f9195ed325b007106ff19226419ce57fe5b1539c0c24101b12f5e034bbcfb7bbb0451b766cb1071295383d774
…allet backups

a2a9231 rpc: Add warning to user about newkeypool command (Samuel Dobson)

Pull request description:

  This PR prevents `newkeypool` from being run on non-HD wallets, because this would require a new backup every time, so it isn't very safe.

  David Harding also suggested [here](bitcoin#23093 (comment)) that the RPC help text should include a warning to the users about the interaction between newkeypool.

ACKs for top commit:
  achow101:
    ACK a2a9231

Tree-SHA512: 0aa497900f1d179764bce13ffce0bb081ba2ca354492bf2e04b21d0212e960b3ed13a386fbf65602b6b50461f4056a0285752ef707d312da28e82449cd8ea048
@knst knst added the RPC Some notable changes to RPC params/behaviour/descriptions label Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants