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: Merge bitcoin#22507, 21464, 23050, 24367 #6099

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

vijaydasmp
Copy link

bitcoin backports

@vijaydasmp vijaydasmp changed the title backport: backport: Merge bitcoin#22006, 22155, 22507, 22371 Jul 6, 2024
@vijaydasmp vijaydasmp force-pushed the bp23_100_1 branch 3 times, most recently from 9881474 to c50dd0a Compare July 19, 2024 15:48
Copy link

This pull request has conflicts, please rebase.

@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#22006, 22155, 22507, 22371 backport: Merge bitcoin#22006, 22507, 22155, 22371 Jul 20, 2024
@vijaydasmp vijaydasmp force-pushed the bp23_100_1 branch 2 times, most recently from 7b66f1f to 0c23f20 Compare July 20, 2024 09:47
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#22006, 22507, 22155, 22371 backport: Merge bitcoin#22006, 22507 Jul 20, 2024
Copy link

github-actions bot commented Aug 7, 2024

This pull request has conflicts, please rebase.

Copy link

github-actions bot commented Sep 4, 2024

This pull request has conflicts, please rebase.

@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#22006, 22507 backport: Merge bitcoin#22507 Sep 6, 2024
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#22507 backport: Merge bitcoin#22507, 21464, 23591, 24153, 23002, 22677, 23547, 23065, 23723, 23642 Sep 6, 2024
@vijaydasmp vijaydasmp force-pushed the bp23_100_1 branch 7 times, most recently from 04076d6 to a587c2b Compare September 8, 2024 12:21
Copy link

This pull request has conflicts, please rebase.

@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#22507, 21464, 23591, 24153, 23002, 22677, 23547, 23065, 23723, 23642, 21679 backport: Merge bitcoin#22507, 21464, 22677, 23065, 23642, 21679 Oct 5, 2024
Copy link

github-actions bot commented Oct 5, 2024

This pull request has conflicts, please rebase.

@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#22507, 21464, 22677, 23065, 23642, 21679 backport: Merge bitcoin#22507, 21464, 23065 Nov 24, 2024
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#22507, 21464, 23065 backport: Merge bitcoin#22507, 21464 Jan 2, 2025
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#22507, 21464 backport: Merge bitcoin#22507, 21464, 23050, 24367 Jan 2, 2025
@vijaydasmp vijaydasmp marked this pull request as ready for review January 2, 2025 15:14
Copy link

coderabbitai bot commented Jan 2, 2025

Walkthrough

The pull request introduces a series of grammatical corrections across multiple files in the Dash project codebase. The primary focus is on replacing the phrase "can not" with "cannot" in various error messages, comments, and logging statements. These changes span multiple directories, including source code files (src/), documentation (doc/), and test scripts (test/).

In addition to the grammatical updates, there are modifications in the transaction memory pool management within src/txmempool.cpp and src/txmempool.h, which introduce new parameters for tracking and limiting ancestor transactions during block updates. The changes also include refinements to error messages related to wallet transactions and improvements in the handling of transaction change outputs in src/wallet/wallet.cpp.

Overall, while most changes are textual and do not affect functionality, the updates enhance code readability and grammatical consistency throughout the project.


📜 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 f126679 and 7972af9.

📒 Files selected for processing (23)
  • contrib/guix/guix-build (1 hunks)
  • doc/developer-notes.md (2 hunks)
  • doc/release-process.md (1 hunks)
  • src/net.cpp (1 hunks)
  • src/node/miner.cpp (1 hunks)
  • src/qt/bitcoin.cpp (1 hunks)
  • src/qt/intro.cpp (1 hunks)
  • src/rpc/misc.cpp (1 hunks)
  • src/rpc/txoutproof.cpp (1 hunks)
  • src/test/util_tests.cpp (1 hunks)
  • src/txdb.cpp (1 hunks)
  • src/txmempool.cpp (5 hunks)
  • src/txmempool.h (2 hunks)
  • src/validation.cpp (1 hunks)
  • src/wallet/rpcwallet.cpp (3 hunks)
  • src/wallet/test/wallet_tests.cpp (1 hunks)
  • src/wallet/wallet.cpp (1 hunks)
  • test/functional/feature_txindex_compatibility.py (1 hunks)
  • test/functional/mempool_updatefromblock.py (1 hunks)
  • test/functional/rpc_decodescript.py (1 hunks)
  • test/functional/rpc_uptime.py (1 hunks)
  • test/functional/wallet_encryption.py (1 hunks)
  • test/util/test_runner.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (21)
  • contrib/guix/guix-build
  • test/util/test_runner.py
  • src/node/miner.cpp
  • test/functional/wallet_encryption.py
  • src/qt/intro.cpp
  • test/functional/feature_txindex_compatibility.py
  • src/test/util_tests.cpp
  • src/qt/bitcoin.cpp
  • test/functional/rpc_decodescript.py
  • test/functional/mempool_updatefromblock.py
  • src/rpc/misc.cpp
  • src/net.cpp
  • src/txdb.cpp
  • test/functional/rpc_uptime.py
  • src/rpc/txoutproof.cpp
  • doc/developer-notes.md
  • src/wallet/test/wallet_tests.cpp
  • doc/release-process.md
  • src/wallet/wallet.cpp
  • src/validation.cpp
  • src/wallet/rpcwallet.cpp
⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: Build (linux64_ubsan, x86_64-pc-linux-gnu, linux64)
  • GitHub Check: Build (linux64_tsan, x86_64-pc-linux-gnu, linux64)
  • GitHub Check: Build (linux64_sqlite, x86_64-pc-linux-gnu, linux64)
  • GitHub Check: Build (linux64_nowallet, x86_64-pc-linux-gnu, linux64)
  • GitHub Check: Build (linux64_fuzz, x86_64-pc-linux-gnu, linux64)
  • GitHub Check: Build (linux64_cxx20, x86_64-pc-linux-gnu, linux64)
  • GitHub Check: Build (linux64, x86_64-pc-linux-gnu, linux64)
  • GitHub Check: Build (arm-linux, arm-linux-gnueabihf, arm-linux)
🔇 Additional comments (4)
src/txmempool.cpp (3)

Line range hint 127-171: Ensure Proper Iterator Usage to Avoid Invalidation

In the UpdateForDescendants method, the code correctly avoids direct removal of transactions to prevent iterator invalidation. Instead, it marks transactions for removal by adding them to descendants_to_remove. This approach maintains iterator validity during traversal, which is essential for the stability of the mempool's data structures.


Line range hint 177-228: Consistent Enforcement of Ancestor Limits in UpdateTransactionsFromBlock

The UpdateTransactionsFromBlock method now includes ancestor_size_limit and ancestor_count_limit parameters, which are passed to UpdateForDescendants to enforce the same limits. This enhancement ensures consistency in applying ancestor constraints during mempool updates after a block is connected or disconnected, improving the robustness of transaction management.


166-171: Verify Accuracy of Ancestor Limit Checks

The code checks if a descendant's ancestor count or size exceeds the specified limits and adds such transactions to descendants_to_remove. It's important to verify that these checks accurately enforce the intended limits and that no transactions are incorrectly marked or omitted.

Please run the following script to ensure that all relevant calls to UpdateForDescendants include the new parameters and that the ancestor limit checks are implemented correctly:

✅ Verification successful

Ancestor Limit Checks Implementation Verified

The ancestor limit checks are correctly implemented and consistently used throughout the codebase. The UpdateForDescendants method properly includes and utilizes the ancestor limit parameters, and the transaction marking logic in the original snippet correctly enforces these limits.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all calls to UpdateForDescendants include the new parameters with ancestor limits.

# Search for calls to UpdateForDescendants without the new ancestor limit parameters
ast-grep --pattern $'UpdateForDescendants($_1, $_2, $_3)' src/

# Expected result: No matches, indicating all calls have been updated with the new parameters.

# Optionally, verify the implementation of ancestor limit checks
rg --context 5 'descendant\.Get(Size|Count)WithAncestors\(\)' src/

Length of output: 63


Script:

#!/bin/bash
# Search for UpdateForDescendants implementation and calls with any parameters
ast-grep --pattern 'UpdateForDescendants' src/

# Find all usages of ancestor-related functions
rg -l "WithAncestors" src/

# Search for the specific limit check implementation
rg -B 5 -A 5 "descendants_to_remove\.insert" src/

Length of output: 587


Script:

#!/bin/bash
# Get the full method declaration and surrounding context
rg -B 2 -A 5 "void.*UpdateForDescendants" src/

# Get the context around the usage to verify parameters
rg -B 5 -A 5 "UpdateForDescendants.*ancestor" src/

Length of output: 2134

src/txmempool.h (1)

683-701: Update Method Signatures to Include Ancestor Limits

The method signatures for UpdateTransactionsFromBlock and UpdateForDescendants have been updated to include ancestor_size_limit and ancestor_count_limit parameters. It is crucial to ensure that all callers of these methods pass the appropriate limit values to maintain consistency and prevent potential runtime errors.

Please run the following script to check for any calls to these methods that may not have been updated with the new parameters:

✅ Verification successful

All Method Calls Updated Correctly

The verification confirms that all calls to UpdateTransactionsFromBlock have been properly updated with the new ancestor limit parameters. The only external call is in validation.cpp and correctly includes all parameters. UpdateForDescendants is only called internally from UpdateTransactionsFromBlock with the proper parameter forwarding.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for outdated calls to UpdateTransactionsFromBlock and UpdateForDescendants.

# Find calls to UpdateTransactionsFromBlock with fewer than three arguments (excluding 'this' pointer)
ast-grep --pattern $'UpdateTransactionsFromBlock($_)' src/

# Find calls to UpdateForDescendants with fewer than six arguments (excluding 'this' pointer)
ast-grep --pattern $'UpdateForDescendants($_1, $_2, $_3)' src/

# Expected result: No matches, ensuring all calls have been updated with the new parameters.

Length of output: 225


Script:

#!/bin/bash
# First, let's verify the class definition and method signatures
ast-grep --pattern 'class CTxMemPool {
  $$$
  UpdateTransactionsFromBlock($$$)
  $$$
}'

# Find all calls to UpdateTransactionsFromBlock with their arguments
rg -U "UpdateTransactionsFromBlock\([^)]+\)" -A 1

# Find all calls to UpdateForDescendants with their arguments
rg -U "UpdateForDescendants\([^)]+\)" -A 1

# Backup search to ensure we don't miss any calls
rg "UpdateTransactionsFromBlock|UpdateForDescendants" --type cpp

Length of output: 90


Script:

#!/bin/bash
# Find relevant files
fd -t f "txmempool" src/

# Simple pattern search in cpp files
rg "UpdateTransactionsFromBlock" --type cpp

# Look for the implementation
rg -U "void CTxMemPool::UpdateTransactionsFromBlock" -A 3

# Look for test files that might call these methods
rg "UpdateTransactionsFromBlock" src/test/

Length of output: 1779


Script:

#!/bin/bash
# Find UpdateForDescendants implementation and calls
rg -U "UpdateForDescendants" src/txmempool.cpp -A 3

# Look for any other calls to UpdateForDescendants
rg "UpdateForDescendants" --type cpp

# Check the implementation file content around UpdateTransactionsFromBlock
rg -U "UpdateTransactionsFromBlock" src/txmempool.cpp -B 5 -A 20

Length of output: 5553


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: 0

🧹 Nitpick comments (2)
src/txmempool.h (1)

678-698: Good clarity on parameter usage and documentation.
The updated documentation thoroughly describes how ancestor_size_limit and ancestor_count_limit are used in UpdateTransactionsFromBlock. Consider clarifying the behavior if these limits are zero or extremely high to avoid confusion.

src/validation.cpp (1)

366-368: Enhance mempool ancestor limit control

The addition of ancestor_size_limit and ancestor_count_limit parameters improves control over transaction ancestor limits during mempool updates after reorganization. These limits help prevent memory exhaustion from large transaction chains.

Consider adding validation to ensure the limits don't exceed system constraints, and document the expected ranges in comments.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between a5787c9 and e2db0fd.

📒 Files selected for processing (20)
  • contrib/guix/guix-build (1 hunks)
  • doc/developer-notes.md (2 hunks)
  • doc/release-process.md (1 hunks)
  • src/net.cpp (1 hunks)
  • src/node/miner.cpp (1 hunks)
  • src/qt/bitcoin.cpp (1 hunks)
  • src/qt/intro.cpp (1 hunks)
  • src/rpc/misc.cpp (1 hunks)
  • src/test/util_tests.cpp (1 hunks)
  • src/txdb.cpp (1 hunks)
  • src/txmempool.cpp (5 hunks)
  • src/txmempool.h (2 hunks)
  • src/validation.cpp (1 hunks)
  • src/wallet/rpcwallet.cpp (3 hunks)
  • test/functional/feature_txindex_compatibility.py (1 hunks)
  • test/functional/mempool_updatefromblock.py (1 hunks)
  • test/functional/rpc_decodescript.py (1 hunks)
  • test/functional/rpc_uptime.py (1 hunks)
  • test/functional/wallet_encryption.py (1 hunks)
  • test/util/test_runner.py (1 hunks)
✅ Files skipped from review due to trivial changes (14)
  • test/util/test_runner.py
  • src/txdb.cpp
  • src/node/miner.cpp
  • src/qt/intro.cpp
  • contrib/guix/guix-build
  • doc/developer-notes.md
  • src/net.cpp
  • test/functional/wallet_encryption.py
  • test/functional/rpc_decodescript.py
  • test/functional/feature_txindex_compatibility.py
  • src/qt/bitcoin.cpp
  • src/test/util_tests.cpp
  • src/rpc/misc.cpp
  • src/wallet/rpcwallet.cpp
🔇 Additional comments (9)
test/functional/rpc_uptime.py (1)

26-26: LGTM! Grammatical improvement in error message.

The change from "can not" to "cannot" improves the grammatical accuracy of the error message while maintaining the same semantic meaning.

Let's verify this matches Bitcoin's implementation:

✅ Verification successful

Error message standardization is consistent within the codebase

The verification shows that the error message "Mocktime cannot be negative" is consistently used in both the RPC implementation (src/rpc/misc.cpp) and its corresponding test (test/functional/rpc_uptime.py). The error message format follows the same pattern in both locations, ensuring internal consistency within the Dash codebase.

While we couldn't directly verify against Bitcoin's implementation, the change aligns with good coding practices by:

  1. Maintaining consistency between implementation and test
  2. Using proper grammar ("cannot" instead of "can not")
  3. Preserving the same error code (-8) and error handling pattern
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the error message matches Bitcoin's implementation
# Expected: The error message should be identical in both codebases

# Search for this error message in Bitcoin's codebase
rg -l "Mocktime cannot be negative" $(gh repo clone bitcoin/bitcoin --depth 1 2>/dev/null || echo "bitcoin")

Length of output: 152


Script:

#!/bin/bash
# Let's try searching in the current repository first to understand the context better
rg "Mocktime cannot be negative" -A 2 -B 2

Length of output: 658


Script:

#!/bin/bash
# Let's check the git history to see if this was indeed backported from Bitcoin
git log -n 5 --grep="cannot" --pretty=format:"%H %s" test/functional/rpc_uptime.py

Length of output: 0

src/txmempool.h (1)

840-871: Well-defined pre-/post-conditions for UpdateForDescendants.
The expanded docstring clearly outlines expectations for cachedDescendants and descendants_to_remove. Ensure edge cases (e.g., repeated removal requests, extremely large mempools) are handled gracefully.

src/txmempool.cpp (4)

127-129: Added parameters for ancestor limits.
Passing uint64_t ancestor_size_limit, ancestor_count_limit provides greater control. Verify that upstream calls handle invalid or zero values appropriately to avoid unexpected early removal of transactions.


166-171: Marked transactions for later removal, avoiding iterator invalidation.
This is a good approach to prevent invalid iterator usage. Confirm that follow-up calls remove these offending transactions in all scenarios to maintain a consistent mempool state.


Line range hint 177-228: Enhanced removal workflow for re-introduced transactions.
Using descendants_to_remove ensures that transactions exceeding ancestor limits after a block disconnection are removed in a separate pass. Verify that any reorg edge cases (e.g., chain tips toggling frequently) do not leave stale entries behind.


1481-1481: Refined log message in PrioritiseTransaction.
The logging now precisely indicates fee adjustments. This is consistent with related debug statements and improves clarity for debugging fee deltas in the mempool.

test/functional/mempool_updatefromblock.py (1)

20-20: Fine-tune ancestor count limit for test environment.
Adding -limitancestorcount=100 helps avoid unintentional errors when building large transaction graphs. Ensure that other tests setting conflicting ancestor limits don’t interfere with expected behaviors.

doc/release-process.md (1)

181-181: Improved commit message clarity.
Replacing “add detached sigs” with “Add attestations” more accurately describes the nature of the committed files. The instructions are now more intuitive and easier to follow for contributors.

src/validation.cpp (1)

368-368: Properly propagate ancestor limits to mempool updates

The ancestor limits are correctly passed through to UpdateTransactionsFromBlock, ensuring consistent application of the limits during reorganization.

Let's verify that these limits are properly enforced by checking the mempool code:

✅ Verification successful

Ancestor limits are properly enforced in mempool updates

The verification confirms that the ancestor limits are correctly propagated and enforced:

  1. UpdateTransactionsFromBlock passes the limits to UpdateForDescendants
  2. Inside UpdateForDescendants, transactions exceeding the limits are identified and added to descendants_to_remove
  3. These transactions are then removed from the mempool via removeRecursive

This ensures that after a block reorganization, any transaction in the mempool that would exceed the ancestor limits is properly removed, maintaining consistency with the ancestor limit policy.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check how UpdateTransactionsFromBlock uses the ancestor limits
ast-grep --pattern 'UpdateTransactionsFromBlock($_, $_, $_)'

Length of output: 228


Script:

#!/bin/bash
# Let's try a different approach to find the UpdateTransactionsFromBlock implementation
rg -U "UpdateTransactionsFromBlock.*\(" --type cpp

Length of output: 709


Script:

#!/bin/bash
# Let's check the implementation in txmempool.cpp to see how the limits are used
rg -A 20 "UpdateTransactionsFromBlock.*ancestor" src/txmempool.cpp

Length of output: 1222


Script:

#!/bin/bash
# Let's search for where these ancestor limits are actually used in the update process
rg -B 5 -A 15 "ancestor_size_limit|ancestor_count_limit" src/txmempool.cpp

Length of output: 3900

@UdjinM6
Copy link

UdjinM6 commented Jan 3, 2025

24367: missing changes in:

  • verifytxoutproof() help text (it's in src/rpc/txoutproof.cpp now)
  • CreateTransactionInternal() error message
  • test/functional/rpc_decodescript.py comment

Also, we don't have test/functional/wallet_timelock.py yet, so 24367 should be partial I guess

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: 0

🔭 Outside diff range comments (1)
src/wallet/wallet.cpp (1)

Line range hint 1990-2000: Implement SQLite wallet backup support

The SQLite wallet backup functionality is currently not implemented, which could lead to data loss for SQLite wallets.

Consider implementing proper backup support for SQLite wallets similar to BDB implementation. Key requirements:

  1. Atomic backup operations
  2. Proper error handling
  3. Backup rotation support
  4. Verification of backup integrity

Example implementation structure:

bool CWallet::AutoBackupWallet(const fs::path& wallet_path, bilingual_str& error_string, std::vector<bilingual_str>& warnings)
{
    if (m_database->Format() == DatabaseFormat::SQLITE) {
        // Implement SQLite-specific backup logic
        return BackupSQLiteWallet(wallet_path, error_string, warnings);
    }
    // Existing BDB backup logic
    return BackupBDBWallet(wallet_path, error_string, warnings);
}
🧹 Nitpick comments (4)
src/wallet/wallet.cpp (4)

Line range hint 3751-3792: Consider enhancing transaction fee handling

The fee calculation and validation logic could be improved in a few ways:

  1. Add explicit bounds checking for fees to prevent integer overflow
  2. Add more detailed error messages for specific fee calculation failures
  3. Consider adding fee estimation fallback logic when initial calculation fails

Example improvement for fee validation:

 if (nFeeRet > m_default_max_tx_fee) {
+    // Add specific error details
+    error = strprintf(_("Fee %d exceeds maximum fee %d"), nFeeRet, m_default_max_tx_fee);
     return false;
 }

Line range hint 2900-2950: Enhance encryption key handling security

The encryption key handling could be improved in several ways:

  1. Use secure memory wiping for all sensitive data
  2. Add configurable key derivation parameters
  3. Improve error messages for encryption failures

Example improvements:

 bool CWallet::Unlock(const SecureString& strWalletPassphrase, bool fForMixingOnly, bool accept_no_keys)
 {
     CCrypter crypter;
-    CKeyingMaterial _vMasterKey;
+    SecureVector<unsigned char> _vMasterKey;
     
     // Add configurable parameters
+    const unsigned int kdfIterations = gArgs.GetArg("-kdfiterations", 25000);
+    if (kdfIterations < 20000) {
+        throw std::runtime_error("KDF iterations too low, minimum is 20000");
+    }

Line range hint 4450-4500: Improve descriptor wallet robustness

The descriptor wallet implementation could be enhanced:

  1. Add more comprehensive descriptor validation
  2. Improve error handling for invalid descriptors
  3. Add migration utilities from legacy wallets

Example improvements:

bool CWallet::ValidateDescriptor(const std::string& descriptor, bilingual_str& error)
{
    // Add comprehensive descriptor validation
    FlatSigningProvider keys;
    std::string desc_error;
    auto parsed = Parse(descriptor, keys, desc_error, false);
    if (!parsed) {
        error = Untranslated(strprintf("Invalid descriptor: %s", desc_error));
        return false;
    }
    // Add more validation checks
    return true;
}

Line range hint 1-100: Consider enhancing logging and documentation

The wallet implementation would benefit from:

  1. More detailed logging of critical operations
  2. Better documentation of complex workflows
  3. Additional test coverage recommendations

Example logging improvements:

 bool CWallet::CreateTransaction(/*...*/)
 {
+    WalletLogPrintf("Creating transaction with %d recipients\n", vecSend.size());
     // ... existing code ...
+    WalletLogPrintf("Transaction created successfully: %s\n", tx->GetHash().ToString());
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between e2db0fd and c36eff7.

📒 Files selected for processing (16)
  • contrib/guix/guix-build (1 hunks)
  • doc/developer-notes.md (2 hunks)
  • src/net.cpp (1 hunks)
  • src/qt/bitcoin.cpp (1 hunks)
  • src/qt/intro.cpp (1 hunks)
  • src/rpc/misc.cpp (1 hunks)
  • src/rpc/txoutproof.cpp (1 hunks)
  • src/test/util_tests.cpp (1 hunks)
  • src/txdb.cpp (1 hunks)
  • src/wallet/rpcwallet.cpp (3 hunks)
  • src/wallet/wallet.cpp (1 hunks)
  • test/functional/feature_txindex_compatibility.py (1 hunks)
  • test/functional/rpc_decodescript.py (1 hunks)
  • test/functional/rpc_uptime.py (1 hunks)
  • test/functional/wallet_encryption.py (1 hunks)
  • test/util/test_runner.py (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/rpc/txoutproof.cpp
🚧 Files skipped from review as they are similar to previous changes (14)
  • test/util/test_runner.py
  • src/rpc/misc.cpp
  • contrib/guix/guix-build
  • src/test/util_tests.cpp
  • test/functional/rpc_uptime.py
  • src/qt/intro.cpp
  • test/functional/feature_txindex_compatibility.py
  • src/qt/bitcoin.cpp
  • src/txdb.cpp
  • doc/developer-notes.md
  • test/functional/rpc_decodescript.py
  • test/functional/wallet_encryption.py
  • src/net.cpp
  • src/wallet/rpcwallet.cpp
🔇 Additional comments (1)
src/wallet/wallet.cpp (1)

3786-3786: Improved error message for transaction change output index validation

The error message has been updated to be more descriptive when the change output index is out of range.

laanwj and others added 4 commits January 8, 2025 13:19
….sigs repo

fafade9 doc: Adjust commit message template for the guix.sigs repo (MarcoFalke)

Pull request description:

  Seems to be the most common template used, so adjust this here, too.

ACKs for top commit:
  laanwj:
    ACK fafade9
  hebasto:
    re-ACK fafade9

Tree-SHA512: 20477d14ecfad94f3b28b94786a4c0d98df539360d0c1deefa94766064a7d0700c849e54d6b251f922e135fcfa964ada0c724090f7f92b459ea39f7c3ca8c65d
c5b36b1 Mempool Update Cut-Through Optimization (Jeremy Rubin)
c49daf9 [TESTS] Increase limitancestorcount in tournament RPC test to showcase improved algorithm (Jeremy Rubin)

Pull request description:

  Often when we're updating mempool entries we update entries that we ultimately end up removing the updated entries shortly thereafter. This patch makes it so that we filter for such entries a bit earlier in processing, which yields a mild improvement for these cases, and is negligible overhead otherwise.

  There's potential for a better -- but more sophisticated -- algorithm that can be used taking advantage of epochs, but I figured it is better to do something that is simple and works first and upgrade it later as the other epoch mempool work proceeds as it makes the patches for the epoch algorithm simpler to understand, so you can consider this as preparatory work. It could either go in now if it is not controversial, or we could wait until the other patch is ready to go.

ACKs for top commit:
  instagibbs:
    reACK c5b36b1
  sipa:
    utACK c5b36b1
  mzumsande:
    Code Review ACK c5b36b1

Tree-SHA512: 78b16864f77a637d8a68a65e23c019a9757d8b2243486728ef601d212ae482f6084cf8e69d810958c356f1803178046e4697207ba40d6d10529ca57de647fae6
…ce-versa

c17f554 Fix BlockAssembler::AddToBlock, CTxMemPool::PrioritiseTransaction logging (Jon Atack)

Pull request description:

  This is a tale of two fees, er, fee rates... indeed, one is misdescribed as a fee, and the other is incorrectly called a fee rate.

  From this review discussion: bitcoin#22689 (comment) (thanks to John Newbery).

ACKs for top commit:
  laanwj:
    Code review ACK c17f554

Tree-SHA512: 3d9df3209a72562c5f9bbf815923d5b089d04491b8d19caa2c04158c501b47ef01e47f1c32d89adcbaf3c6357329507f65b4bb2963214c3451bbfa61ac812530
…ixups from transifex translator feedback

4874269 Replace "can not" with "cannot" in docs, user messages, and tests (Jon Atack)
e670edd User-facing content fixups from transifex translator feedback (Jon Atack)

Pull request description:

  Closes bitcoin#24366.

ACKs for top commit:
  laanwj:
    Code review re-ACK 4874269
  hebasto:
    re-ACK 4874269, only suggested change since my previous [review](bitcoin#24367 (review)).

Tree-SHA512: 4dcdcb417251a413e65fab6070515e13a1267c8e0dbcf521386b842511391f24c84a0c2168fe13458c977682034466509bf2a3453719d4d94d3c568fd9f4adb4
@vijaydasmp
Copy link
Author

Hello @UdjinM6 , requesting review

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 7972af9

@UdjinM6
Copy link

UdjinM6 commented Jan 9, 2025

Gitlab failure https://gitlab.com/dashpay/dash/-/jobs/8793673680 looks unrelated, restarted the job

@vijaydasmp
Copy link
Author

Hello @PastaPastaPasta @knst requesting review

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