Refactor Public Transaction Creation

[ghost]

This PR refactors public transaction creation, which is a major change requiring careful review.

[psolstice]

Why -10 specifically?

[psolstice]

Is this the only way to implement the test? Wallet changes can certainly be done with an extra member function.

[psolstice]

Using std::vector::data() method is the only possible improvement here. Other methods work but at() is just as bad or even worse than []

[psolstice]

why not use quorumInstantSendManager->IsLocked() here?

[psolstice]

I don't think changing function signature (reserveKey parameter) is justified here, this function is inherited from bitcoin and subsequent merges with bitcoin PRs may create quite of lot of surprises

[psolstice]

it's only a type but I can't find its usage of TxSpends outside of the CWallet class, why change it to public then?

[psolstice]

same as above

[reubenyap]

@coderabbitai review

[coderabbitai]

Walkthrough

The recent updates introduce significant enhancements to transaction handling and wallet functionality. Key modifications include simplifying transaction test logic, improving member accessibility, and adding methods for identifying pay-to-public-key scripts. The wallet's management of transparent transactions has also been strengthened with new methods, streamlining the creation and signing processes. Overall, these changes bolster the system's robustness and flexibility.

Changes

Files	Summary
qa/rpc-tests/txn_clone.py, qa/rpc-tests/txn_doublespend.py	Simplified balance calculations and improved locking mechanisms in transaction tests to enhance integrity.
src/Makefile.test.include, src/wallet/test/createtransaction_tests.cpp	Updated BITCOIN_TESTS to include new tests for transaction creation functionality and removed outdated tests.
src/llmq/quorums_instantsend.h	Moved member variables CCriticalSection cs and CInstantSendDb db to the public section for better accessibility.
src/primitives/transaction.h	Removed nRounds member from CTxOut, simplifying class structure and potentially reducing memory usage.
src/script/script.cpp, src/script/script.h	Introduced IsPayToPublicKey() method in CScript class to identify Pay-to-Public-Key scripts.
src/sync.cpp	Included <set> header and improved syntax for InvLockOrders.
src/test/test_bitcoin.cpp	Updated TestingSetup constructor to use GenerateNewMnemonic() for key creation.
src/validation.cpp	Added logging for missing transaction inputs in AcceptToMemoryPoolWorker.
src/wallet/coincontrol.h	Introduced features for coin control: WITH_1000, fAllowUnconfirmed, nMaxInputs, and nMaxSize.
src/wallet/sigmaspendbuilder.cpp/h, .../txbuilder.cpp/h	Renamed classes and updated method signatures reflecting the new transaction building structure.
src/wallet/wallet.cpp	Enhanced CWallet for managing transparent transactions; added methods for CTransparentTxout.
src/wallet/rpcwallet.cpp	Added a locking mechanism in fundrawtransaction for improved thread safety.
qa/rpc-tests/dip3-deterministicmns.py, qa/rpc-tests/test_framework/mn_utils.py	Added functions for locking and unlocking masternode collateral to enhance security and control flow.

"In the realm of code, where the bits entwine,
Changes unfold, with design so fine.
🐇 With a hop and a skip, in the digital glade,
A rabbit crafts, improvements made.
🌟 Through fields of logic, we briskly roam,
Crafting a burrow, we call our own."

---

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>.
  • 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 generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @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 as 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 resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

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]

Review Status

Actionable comments generated: 23

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 53c0526 and da30da3.

Files selected for processing (18)

• qa/rpc-tests/txn_clone.py (4 hunks)
• src/Makefile.test.include (1 hunks)
• src/llmq/quorums_instantsend.h (2 hunks)
• src/primitives/transaction.h (1 hunks)
• src/script/script.cpp (1 hunks)
• src/script/script.h (1 hunks)
• src/sync.cpp (3 hunks)
• src/test/test_bitcoin.cpp (1 hunks)
• src/validation.cpp (1 hunks)
• src/wallet/coincontrol.h (5 hunks)
• src/wallet/sigmaspendbuilder.cpp (3 hunks)
• src/wallet/sigmaspendbuilder.h (2 hunks)
• src/wallet/test/createtransaction_tests.cpp (1 hunks)
• src/wallet/test/sigmatxbuilder_tests.cpp (5 hunks)
• src/wallet/txbuilder.cpp (3 hunks)
• src/wallet/txbuilder.h (1 hunks)
• src/wallet/wallet.cpp (6 hunks)
• src/wallet/wallet.h (8 hunks)

Files not summarized due to errors (2)

• src/wallet/test/createtransaction_tests.cpp: Error: Message exceeds token limit
• src/wallet/wallet.cpp: Error: Message exceeds token limit

Files skipped from review due to trivial changes (1)

• src/wallet/test/sigmatxbuilder_tests.cpp

Additional comments: 55

src/wallet/sigmaspendbuilder.h (2)

• 10-10: Renaming the base class from TxBuilder to SigmaTxBuilderSuperclass aligns with the refactor's goal to specialize the transaction building process for Sigma transactions. This change enhances clarity and specificity in the codebase.
• 22-22: Updating the type of the input parameter in the GetInputs method from InputSigner to SigmaTxBuilderInputSigner is a logical step following the renaming of InputSigner to SigmaTxBuilderInputSigner. This ensures type consistency and leverages the specialized functionality of the new SigmaTxBuilderInputSigner class.

src/wallet/txbuilder.h (2)

• 16-25: Renaming InputSigner to SigmaTxBuilderInputSigner and updating its constructors enhances the clarity and specificity of the code, especially in the context of Sigma transactions. This change makes the purpose of the class more explicit and aligns with the overall refactor's goals.
• 30-44: Renaming TxBuilder to SigmaTxBuilderSuperclass and updating its constructor and method signatures are consistent with the refactor's aim to specialize the transaction building process for Sigma transactions. These changes improve code clarity and ensure that the class's functionality is explicitly tailored to Sigma transactions.

src/wallet/coincontrol.h (2)

• 21-22: Adding WITH_MINTS and WITH_1000 to the CoinType enum introduces more granular control over the types of coins that can be selected for transactions. This enhancement supports the refactor's goal of providing more flexibility and specificity in transaction creation and management.
• 40-51: Introducing fAllowUnconfirmed, nMaxInputs, and nMaxSize to the CCoinControl class provides users with more control over transaction creation, particularly in terms of allowing unconfirmed inputs and setting limits on the number of inputs and the maximum transaction size. These additions align with the refactor's objectives to enhance coin control features.

src/wallet/sigmaspendbuilder.cpp (3)

• 21-21: Renaming the class SigmaSpendSigner to SigmaTxBuilderInputSigner and adjusting its inheritance to reflect the changes made in the header files is a necessary step to maintain consistency across the codebase. This change aligns with the refactor's goal of specializing the transaction building process for Sigma transactions.
• 111-111: The change in inheritance for the SigmaSpendBuilder class from TxBuilder to SigmaTxBuilderSuperclass is consistent with the refactor's aim to specialize the transaction building process. This adjustment ensures that SigmaSpendBuilder leverages the specialized functionality provided by SigmaTxBuilderSuperclass.
• 132-132: Updating the GetInputs method signature in SigmaSpendBuilder to use SigmaTxBuilderInputSigner instead of the previous InputSigner type is a logical continuation of the renaming and specialization efforts. This ensures that the method correctly utilizes the specialized input signer for Sigma transactions.

src/sync.cpp (3)

• 21-21: Including the <set> header is necessary for the use of the std::set container in the InvLockOrders declaration. This inclusion ensures that the necessary data structures are available for managing lock orders, which is crucial for detecting potential deadlocks.
• 70-70: Modifying the declaration of InvLockOrders to use a different syntax for the set declaration (std::set<std::pair<void*, void*>>) is a minor change that does not affect functionality. However, it's important to ensure that such changes are consistent across the codebase and that they follow the project's coding standards.
• 177-177: Correcting the minor formatting issue in the AssertLockNotHeldInternal function improves code readability. While this change is minor, maintaining consistent formatting is important for code maintainability and readability.

qa/rpc-tests/txn_clone.py (2)

• 99-99: Adjusting balance calculations and confirmations handling for transactions tx1 and tx2 is crucial for accurately reflecting the state of the blockchain and the wallet balances after the transactions have been processed. These changes are necessary to ensure that the test script accurately tests the intended scenarios, especially in the context of transaction malleability and cloning.
• 105-105: Sending fund_bar_tx to the miner and handling the clone of tx1 separately are important steps in setting up the test scenario. These actions ensure that the blockchain state is correctly prepared for testing the handling of transaction clones and their impact on wallet balances and confirmations.

src/Makefile.test.include (1)

• 211-212: Adding sigmatxbuilder_tests.cpp and createtransaction_tests.cpp while removing txbuilder_tests.cpp is a positive change, reflecting the refactoring and renaming in the codebase. Ensure that these new test files cover all the functionalities previously tested by txbuilder_tests.cpp and include additional tests for any new functionality introduced by the refactor.

src/wallet/txbuilder.cpp (2)

• 22-30: Renaming InputSigner to SigmaTxBuilderInputSigner and updating its constructors improves clarity and specificity, aligning the class name with its specialized purpose for Sigma transactions. This change enhances code readability and maintainability.
• 42-43: The comment indicating legacy usage for SigmaTxBuilderSuperclass::Build is helpful for understanding the context in which this method is used. It's good practice to mark legacy code clearly, but also consider if there's a plan to refactor or replace this legacy code to align with the new architecture.

src/test/test_bitcoin.cpp (1)

• 112-112: The addition of pwalletMain->GenerateNewMnemonic(); before generating a new HD master key is a positive change, enhancing the realism of the wallet setup process in tests. This aligns more closely with actual wallet initialization scenarios, potentially improving test coverage and relevance.

However, please ensure that this change does not adversely affect existing tests or introduce new assumptions that could impact test reliability.

src/primitives/transaction.h (3)

• 174-174: The change of the nRounds default value from 0 to -10 in the CTxOut class:

• Ensure that this change does not affect the compatibility with existing transactions, especially those that might rely on the default value of nRounds for certain logic or calculations.
• Clarify the rationale behind choosing -10 as the new default value. It's important to understand the significance of this specific value and how it impacts the transaction creation or processing logic.
• Consider the potential impact on transaction privacy or mixing features. Since nRounds could be related to transaction mixing or privacy enhancements, changing its default value might have implications on these aspects.

• 171-177: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]

Ensure the copyright notice at the top of the file is up-to-date and accurately reflects the copyright years and holders.

• 174-174: The introduction of a non-standard default value (-10) for nRounds in CTxOut:

• Verify that this change is well-documented within the project's documentation or code comments. It's crucial for maintainers and developers to understand the purpose and implications of this default value.
• Assess the impact on external systems or libraries that interact with this codebase. Ensure that this change does not introduce unexpected behavior in systems that rely on the CTxOut structure.

src/script/script.h (1)

• 665-665: The addition of the IsPayToPublicKey() method to the CScript class is noted. However, the implementation details of this method are not provided in the snippet. Assuming the implementation is correct and efficient, this method could enhance the script validation capabilities by allowing easy identification of scripts that pay directly to a public key, which aligns with the PR's objectives of refining transaction creation and handling.

Ensure that the method's implementation follows best practices for performance, especially considering the potential frequency of its invocation in transaction validation processes. Additionally, thorough testing should be conducted to verify its correctness in various scenarios, including edge cases.

src/wallet/test/createtransaction_tests.cpp (18)

• 14-18: Utility function GetAddress correctly converts a CTxDestination to a Bitcoin address string using CBitcoinAddress. This is a straightforward and commonly used pattern in Bitcoin-related code.
• 39-43: The function GetRandomDest generates a random script destination. It uses CKey::MakeNewKey to create a new private key and then derives the public key destination. This is a standard method for generating random addresses in tests.
• 77-82: GetFakeTransparentTxout is an overloaded version that simplifies creating a fake transparent output by deciding whether the output is "mine" based on the isMine parameter. It delegates to the more specific GetFakeTransparentTxout function with either a random wallet address or a random destination. This is a good use of function overloading to provide a simpler interface for common use cases.
• 84-90: The function GetFakeTransparentTxouts generates a vector of fake transparent outputs for given values. It demonstrates good use of modern C++ features like range-based for loops and emplace_back for efficient vector operations.
• 92-98: GetFakeRecipients creates a vector of recipients for given values, using CRecipient structs. This function is straightforward and utilizes efficient vector operations similar to GetFakeTransparentTxouts.
• 110-112: AssertVoutAddr correctly asserts that a transaction's output script matches the expected recipient's script. This is a concise and effective way to validate transaction outputs in tests.
• 114-115: AssertVoutValue is a simple assertion function that checks if a transaction output's value matches the expected value. It uses BOOST_ASSERT effectively.
• 118-123: AssertHasKey ensures that a specific output of a transaction is recognized as "mine" by the wallet. It correctly locks the wallet and keeps the reserve key. This function is essential for tests that involve transaction signing and ownership checks.
• 126-135: The macros defined for assertions (ASSERT_VIN_VALUE, ASSERT_VOUT_ADDR, etc.) provide a concise way to perform common assertions in the test cases. They improve readability and maintainability of the test code by abstracting repetitive assertion patterns.
• 140-141: The test suite createtransaction_tests is correctly set up using BOOST_FIXTURE_TEST_SUITE with WalletTestingSetup as the fixture. This setup ensures that each test case has a clean environment with necessary wallet and blockchain components initialized.
• 247-270: The test case selects_smallest_input_required verifies that the wallet selects the smallest necessary input to cover the transaction amount and fees. This is an important test for ensuring efficient use of inputs. The test is clear and effectively uses utility functions and assertion macros. It's well-constructed and covers the intended scenario adequately.
• 273-311: The test case insufficient_funds checks the wallet's behavior when there are insufficient funds to cover the transaction amount and fees. It tests both a scenario where the funds are indeed insufficient and a scenario where adding an additional input makes the transaction possible. This test case is crucial for ensuring robust error handling in the wallet. It's well-implemented and makes good use of utility functions and assertion macros.
• 314-358: The test case no_unconfirmed_inputs tests the wallet's behavior when configured not to use unconfirmed inputs for transactions. It verifies that transactions fail when only unconfirmed inputs are available and succeed when confirmed inputs are used. This test case is important for ensuring that coin control settings are respected. It's well-constructed and effectively uses utility functions and assertion macros.
• 361-387: The test case takes_from_front_and_back verifies the wallet's input selection strategy when it needs to take inputs from both the beginning and the end of the available inputs to cover the transaction amount. This scenario tests the wallet's ability to efficiently select inputs. The test is well-structured and makes effective use of utility functions and assertion macros.
• 390-418: The test case doesnt_select_used_inputs ensures that the wallet does not select inputs that are already marked as spent. This is crucial for preventing double-spending attempts and ensuring transaction validity. The test is clear, concise, and effectively uses utility functions and assertion macros to validate the expected behavior.
• 838-885: The test case coincontrol_input_count_limit verifies the wallet's behavior when a maximum input count is specified via coin control settings. It tests scenarios where the input count is sufficient and where it is not. This test case is crucial for ensuring that input count limits are respected during transaction creation. It's well-structured and effectively uses utility functions and assertion macros.
• 974-1000: The test case coincontrol_destination_address verifies the wallet's ability to use a specific change address specified via coin control settings. It tests the scenario where a custom change destination is provided and ensures that the change is sent to the correct address. This test case is crucial for ensuring that coin control settings related to change addresses are respected. It's well-structured and effectively uses utility functions and assertion macros.
• 1119-1172: The test case coincontrol_require_all_inputs tests the wallet's behavior when the requirement to use all selected inputs is specified via coin control settings. It covers scenarios where all inputs are required and where they are not. This test case is crucial for ensuring that the wallet correctly interprets and applies coin control settings related to input selection. It's well-constructed and effectively uses utility functions and assertion macros.

src/wallet/wallet.h (2)

• 723-755: The methods AddToSpends, GetFee, GetTransparentTxouts, and templated methods GetAvailableInputs and GetInputsForTx introduce significant new logic for handling transactions. Ensure that:

• The logic is correctly implemented and aligns with the intended functionality.
• Error handling is robust, especially for external calls and interactions.
• Performance implications are considered, particularly for loops and data structure access patterns.
• Unit tests are added or updated to cover these new functionalities.

• 1093-1105: The methods SignTransparentInputs, CheckTransparentTransactionSanity, and CreateTransaction have been updated to support transparent transactions. Ensure that:

• The changes do not introduce any security vulnerabilities, especially in the signing process.
• The transaction sanity checks are comprehensive and prevent invalid transactions from being created.
• The modifications to CreateTransaction do not adversely affect existing functionalities and are compatible with the rest of the wallet's operations.

src/wallet/wallet.cpp (13)

• 7-7: The inclusion of new headers such as script/standard.h, walletexcept.h, sigmaspendbuilder.h, and lelantusjoinsplitbuilder.h suggests enhancements and new functionalities related to transaction creation and handling. Ensure that these headers are necessary and correctly utilized in the wallet's refactored code.
• 88-90: The method IsTransparentTxout correctly identifies if a transaction output is transparent based on its scriptPubKey. This is crucial for distinguishing between different types of transactions in the wallet.
• 156-172: The method IsMine checks if a transaction output belongs to the wallet, considering both spendable and watch-only outputs. The use of AssertLockHeld ensures thread safety. Ensure that the locking conventions are consistently applied throughout the wallet code to prevent data races.
• 214-231: The method IsCoinTypeCompatible checks for compatibility based on coin type, which is essential for filtering inputs in various transaction creation scenarios. The logic appears sound, but ensure that all coin types are correctly handled and that this method's behavior aligns with the intended use cases for coin control.
• 233-242: The method IsLLMQInstantSendLocked checks for LLMQ InstantSend lock status. The locking mechanisms (AssertLockHeld) used here are crucial for thread safety, especially given the potential complexity of querying InstantSend lock status. Ensure that the lock ordering is consistent with the rest of the application to avoid deadlocks.
• 408-408: The modification to set the master key from the mnemonic container's seed is a critical part of securely handling wallet keys. Ensure that the seed data is correctly managed and that this change integrates well with the overall key management strategy.
• 470-470: Reserving space for the seed vector before setting the master key is a good practice to ensure memory efficiency. This change, along with the correct handling of the master key, contributes to the wallet's secure key management.
• 495-500: The logic for deriving child keys and updating the HD chain counters is correctly implemented, ensuring that keys already known to the wallet are skipped. This is crucial for maintaining the wallet's hierarchical deterministic structure and avoiding key reuse.
• 4536-4555: The method GetFee calculates the transaction fee based on the transaction size and coin control settings. The logic for overriding the fee rate and ensuring the fee does not fall below the required minimum is correctly implemented. Ensure that the fee calculation aligns with the network's fee policy and that the error handling for an unrelayable transaction due to a low fee is appropriate.
• 4573-4617: The method SignTransparentInputs signs the transparent inputs of a transaction. It includes comprehensive error handling for various failure scenarios, such as non-standard inputs and signature issues. Ensure that the signing process is secure and that the error messages provide clear guidance for resolving potential issues.
• 4619-4695: The method CheckTransparentTransactionSanity performs various checks to ensure the transaction's sanity, including size, input count, fee adequacy, and standardness of outputs. This comprehensive sanity checking is crucial for preventing the creation of invalid or problematic transactions. Ensure that all checks are aligned with the network's consensus rules and transaction policies.
• 4709-4718: The CreateTransaction method's overload that does not take vTransparentTxouts as an argument retrieves the transparent txouts and then calls the overloaded version that does. This separation of concerns allows for more flexible transaction creation workflows. Ensure that the retrieval of transparent txouts is efficient and that the subsequent call to the overloaded method correctly handles the transaction creation process.
• 4943-5194: The GetInputsForTx method is a complex algorithm for selecting inputs for a transaction, considering various constraints such as size, fee, and coin control settings. The logic is intricate, aiming to optimize input selection for efficiency and cost-effectiveness. Given the complexity, ensure that thorough testing covers various scenarios to confirm the algorithm's correctness and efficiency.

[coderabbitai]

Moving CCriticalSection cs and CInstantSendDb db from private to public sections in CInstantSendManager introduces potential risks related to encapsulation and data integrity. Public members can be modified from outside the class, which could lead to race conditions or inconsistent states. Consider using getter methods or friend classes to maintain encapsulation and protect the integrity of these members.

[coderabbitai]

Renaming TxBuilder to SigmaTxBuilderSuperclass and updating its constructor similarly improves clarity and specificity. However, the term "Superclass" in the name might be misleading if this class is not intended to be a base class for inheritance. Consider a more descriptive name that reflects its role without implying inheritance if that's not the intent.

- SigmaTxBuilderSuperclass
+ SigmaTxBuilder or SigmaTransactionBuilder

---

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.

Suggested change

	SigmaTxBuilderSuperclass::SigmaTxBuilderSuperclass(CWallet& wallet) noexcept : wallet(wallet)
	{
	}
	TxBuilder::~TxBuilder()
	SigmaTxBuilderSuperclass::~SigmaTxBuilderSuperclass()
	SigmaTxBuilder::SigmaTxBuilder(CWallet& wallet) noexcept : wallet(wallet)
	{
	}
	SigmaTxBuilder::~SigmaTxBuilder()

[aleflm]

I would remove the word Super from this name too. @psolstice do you prefere to keep it with Super or do you agree we should remove the word Super from this name.

[coderabbitai]

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

[coderabbitai]

📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [12-37]

Including multiple headers directly in wallet.h increases coupling and compilation times. Consider forward declarations where possible and include headers in the implementation file (wallet.cpp) instead.

[coderabbitai]

The CTransparentTxout class has been introduced to handle transparent transactions. Ensure that:

• The class interface is clear and consistent.
• Member variables are appropriately encapsulated.
• Methods that do not modify class state are marked as const.
• Consider moving the implementation of non-trivial methods to the corresponding .cpp file to improve readability and compilation times.

[coderabbitai]

The test case coincontrol_coin_type tests the wallet's behavior when specific coin types are selected via coin control settings. It covers scenarios with different coin types and masternode mode settings. This test case is important for ensuring that coin control settings are correctly applied during transaction creation. It's well-constructed and makes effective use of utility functions and assertion macros. Consider adding checks for specific error messages related to coin type selection to provide more detailed diagnostics.

Add checks for specific error messages related to coin type selection for more detailed diagnostics.

[coderabbitai]

The test case coincontrol_transaction_size_limit tests the wallet's behavior when a maximum transaction size is specified via coin control settings. It covers scenarios with different size limits and verifies that the wallet respects these limits during transaction creation. This test case is important for ensuring that transaction size limits are correctly applied. It's well-constructed and effectively uses utility functions and assertion macros. Consider adding checks for the actual transaction size in addition to the fee to ensure comprehensive validation.

Add checks for the actual transaction size in addition to the fee for comprehensive validation.

[coderabbitai]

The test case coincontrol_minimum_total_fee tests the wallet's behavior when a minimum total fee is specified via coin control settings. It covers scenarios with different minimum fee settings and verifies that the wallet respects these settings during transaction creation. This test case is important for ensuring that minimum fee settings are correctly applied. It's well-constructed and effectively uses utility functions and assertion macros. Consider adding checks for the actual fee in addition to the success or failure of the transaction to ensure comprehensive validation.

Add checks for the actual fee in addition to the success or failure of the transaction for comprehensive validation.

[coderabbitai]

The test case coincontrol_confirm_target tests the wallet's behavior when a confirmation target is specified via coin control settings. It verifies that transactions fail when inputs do not meet the confirmation target and succeed when they do. This test case is important for ensuring that confirmation targets are respected during transaction creation. It's well-structured and effectively uses utility functions and assertion macros. Consider adding checks for specific error messages related to confirmation targets to provide more detailed diagnostics.

Add checks for specific error messages related to confirmation targets for more detailed diagnostics.

[coderabbitai]

The test case multisig tests the wallet's ability to create transactions involving multisig addresses with various combinations of owned and external keys. It's an important test for ensuring that the wallet correctly handles multisig transactions. The test is well-structured and makes effective use of utility functions and assertion macros. Consider adding more detailed checks for the multisig script and signatures to ensure comprehensive validation of multisig transactions.

Add more detailed checks for the multisig script and signatures for comprehensive validation of multisig transactions.

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between da30da3 and 1dac305.

Files selected for processing (4)

• src/llmq/quorums_instantsend.h (2 hunks)
• src/primitives/transaction.h (2 hunks)
• src/wallet/test/createtransaction_tests.cpp (1 hunks)
• src/wallet/wallet.cpp (6 hunks)

Additional comments not posted (46)

src/llmq/quorums_instantsend.h (1)

112-113: Maintain encapsulation for cs and db.

Moving CCriticalSection cs and CInstantSendDb db from private to public sections in CInstantSendManager introduces potential risks related to encapsulation and data integrity. Public members can be modified from outside the class, which could lead to race conditions or inconsistent states. Consider using getter methods or friend classes to maintain encapsulation and protect the integrity of these members.

src/wallet/test/createtransaction_tests.cpp (17)

14-18: Utility function GetAddress is well-implemented.

The GetAddress function correctly converts a CTxDestination to a string representation of a Bitcoin address.

---

20-24: Utility function GetAddress for CScript is well-implemented.

The GetAddress function correctly extracts a CTxDestination from a CScript and converts it to a string representation of a Bitcoin address.

---

26-37: Encapsulate test-specific logic in GetFakeTransparentTxout.

The function GetFakeTransparentTxout creates a mock transparent output for testing. It directly modifies internal fields _mockupIsMine and _mockupDepthInMainChain of CTransparentTxout, which suggests these fields are intended for testing purposes. Ensure that such modifications are encapsulated within the test or mock classes to avoid leaking test-specific logic into production code.

Consider adding setter methods or friend test classes to handle test-specific modifications, maintaining encapsulation and separation of test and production code.

---

46-52: Improve modularity of GetRandomWalletAddress.

GetRandomWalletAddress assumes that pwalletMain is locked (AssertLockHeld) and available globally. This global dependency can make the tests less modular and harder to maintain. Consider passing the wallet as a parameter to the function to improve modularity and testability.

---

54-74: Improve modularity of GetRandomMultisigAddress.

GetRandomMultisigAddress correctly generates a multisig address with a mix of keys from the wallet and newly generated keys. It properly asserts the logical constraints on the parameters (nOurs <= m, m >= n, n). However, similar to GetRandomWalletAddress, it relies on a global pwalletMain, which could be refactored for better test isolation.

Consider refactoring to accept a wallet parameter for improved modularity.

---

100-108: Enhance assertions in AssertVinValue with descriptive error messages.

The function AssertVinValue checks if a specific input in a transaction matches an expected value. It iterates over a vector of CTransparentTxout to find a matching outpoint. This is a clear and effective way to assert conditions in test cases. However, consider adding error messages to the BOOST_ASSERT calls to provide more context when assertions fail.

Enhance assertions with descriptive error messages for better debugging.

---

140-164: Add checks for specific error messages in sends_money test case.

The test case sends_money verifies the basic functionality of sending money, ensuring that the transaction is created with the correct inputs and outputs. It uses the utility functions and macros effectively for setting up the test and asserting the expected outcomes. However, there's a missed opportunity to check for specific error messages in failure scenarios, which could improve the test's diagnostic capabilities.

Consider adding checks for specific error messages in failure scenarios to provide more detailed diagnostics when tests fail.

---

166-191: Add checks for specific error messages in multiple_recipients test case.

The test case multiple_recipients correctly tests transaction creation with multiple recipients, ensuring that inputs are selected and outputs are created as expected. It demonstrates good use of the utility functions and assertion macros. Similar to the previous comment, consider enhancing the test with checks for specific error messages in failure scenarios.

Add checks for specific error messages to improve diagnostics in failure scenarios.

---

194-243: Explicitly verify the change output's script and value in mints_change test case.

The test case mints_change focuses on the scenario where change is minted as part of the transaction. It tests both the creation of a change output and the absence of a change output when the inputs exactly match the outputs plus fees. This test case is well-structured and effectively uses utility functions and assertion macros. It's a good practice to also verify the change output's script and value explicitly in addition to the existing assertions.

Explicitly verify the change output's script and value to ensure they meet the expected criteria.

---

420-478: Verify specific InstantSend-related error messages in instantsend test case.

The test case instantsend focuses on InstantSend transactions, testing both scenarios where inputs are locked by InstantSend and where they are not. This is important for ensuring that InstantSend transactions are handled correctly. The test is well-constructed and makes good use of utility functions and assertion macros. It's a good practice to also verify the specific InstantSend-related error messages or conditions to ensure detailed diagnostics.

Verify specific InstantSend-related error messages or conditions for more detailed diagnostics.

---

481-594: Add checks for the change output's script and value in change_position test case.

The test case change_position verifies the wallet's ability to handle requests for specific change output positions within a transaction. It tests various scenarios, including valid positions, out-of-range positions, and negative positions. This test case is important for ensuring that the wallet correctly interprets and applies coin control settings related to change output positioning. It's well-structured and effectively uses utility functions and assertion macros. Consider adding checks for the change output's script and value in addition to the position to ensure comprehensive validation.

Add checks for the change output's script and value in addition to the position for comprehensive validation.

---

597-656: Verify specific error messages related to watch-only addresses in watch_only_address test case.

The test case watch_only_address tests the wallet's behavior when dealing with watch-only addresses, both when they are allowed and when they are not. This is crucial for ensuring that the wallet correctly handles transactions involving watch-only addresses according to the user's preferences. The test is clear and effectively uses utility functions and assertion macros. It's a good practice to also verify specific error messages related to watch-only addresses to provide more detailed diagnostics.

Verify specific error messages related to watch-only addresses for more detailed diagnostics.

---

659-834: Add checks for specific error messages related to coin type selection in coincontrol_coin_type test case.

The test case coincontrol_coin_type tests the wallet's behavior when specific coin types are selected via coin control settings. It covers scenarios with different coin types and masternode mode settings. This test case is important for ensuring that coin control settings are correctly applied during transaction creation. It's well-constructed and makes effective use of utility functions and assertion macros. Consider adding checks for specific error messages related to coin type selection to provide more detailed diagnostics.

Add checks for specific error messages related to coin type selection for more detailed diagnostics.

---

887-970: Add checks for the actual transaction size in coincontrol_transaction_size_limit test case.

The test case coincontrol_transaction_size_limit tests the wallet's behavior when a maximum transaction size is specified via coin control settings. It covers scenarios with different size limits and verifies that the wallet respects these limits during transaction creation. This test case is important for ensuring that transaction size limits are correctly applied. It's well-constructed and effectively uses utility functions and assertion macros. Consider adding checks for the actual transaction size in addition to the fee to ensure comprehensive validation.

Add checks for the actual transaction size in addition to the fee for comprehensive validation.

---

1002-1068: Add checks for the actual fee in coincontrol_minimum_total_fee test case.

The test case coincontrol_minimum_total_fee tests the wallet's behavior when a minimum total fee is specified via coin control settings. It covers scenarios with different minimum fee settings and verifies that the wallet respects these settings during transaction creation. This test case is important for ensuring that minimum fee settings are correctly applied. It's well-constructed and effectively uses utility functions and assertion macros. Consider adding checks for the actual fee in addition to the success or failure of the transaction to ensure comprehensive validation.

Add checks for the actual fee in addition to the success or failure of the transaction for comprehensive validation.

---

1071-1115: Add checks for specific error messages related to confirmation targets in coincontrol_confirm_target test case.

The test case coincontrol_confirm_target tests the wallet's behavior when a confirmation target is specified via coin control settings. It verifies that transactions fail when inputs do not meet the confirmation target and succeed when they do. This test case is important for ensuring that confirmation targets are respected during transaction creation. It's well-structured and effectively uses utility functions and assertion macros. Consider adding checks for specific error messages related to confirmation targets to provide more detailed diagnostics.

Add checks for specific error messages related to confirmation targets for more detailed diagnostics.

---

1174-1205: Add more detailed checks for multisig script and signatures in multisig test case.

The test case multisig tests the wallet's ability to create transactions involving multisig addresses with various combinations of owned and external keys. It's an important test for ensuring that the wallet correctly handles multisig transactions. The test is well-structured and makes effective use of utility functions and assertion macros. Consider adding more detailed checks for the multisig script and signatures to ensure comprehensive validation of multisig transactions.

Add more detailed checks for the multisig script and signatures for comprehensive validation of multisig transactions.

src/wallet/wallet.cpp (28)

7-7: LGTM!

The inclusion of script/standard.h is appropriate for the new functionality.

---

88-90: LGTM!

The IsTransparentTxout method correctly identifies transparent transaction outputs.

---

92-94: LGTM!

The GetHash method correctly returns the hash of the outpoint.

---

96-98: LGTM!

The GetOutpoint method correctly returns the outpoint.

---

100-103: LGTM!

The GetValue method correctly returns the value of the transaction output and includes an appropriate assertion.

---

105-107: LGTM!

The GetScriptPubkey method correctly returns the scriptPubKey of the transaction output.

---

109-154: Consider documenting the rationale behind the specific sizes.

The GetMarginalSpendSize method calculates the additional size a given input will add to a transaction. It would be beneficial to add comments explaining the basis for the specific sizes used in the switch-case structure.

---

156-172: LGTM!

The IsMine method correctly checks if the transaction output belongs to the wallet and handles both mockup and real scenarios.

---

174-192: LGTM!

The IsSpendable method correctly checks if the transaction output is spendable and handles both mockup and real scenarios.

---

194-202: LGTM!

The IsLocked method correctly checks if the transaction output is locked and handles both mockup and real scenarios.

---

204-212: LGTM!

The IsAbandoned method correctly checks if the transaction output is abandoned and handles both mockup and real scenarios.

---

214-231: LGTM!

The IsCoinTypeCompatible method correctly checks if the transaction output is compatible with the specified coin type and includes various conditions to ensure compatibility.

---

233-241: LGTM!

The IsLLMQInstantSendLocked method correctly checks if the transaction output is locked by LLMQ InstantSend and handles both mockup and real scenarios.

---

243-251: LGTM!

The IsCoinBase method correctly checks if the transaction output is a coinbase transaction and handles both mockup and real scenarios.

---

253-270: LGTM!

The IsFromMe method correctly checks if the transaction output is from the wallet and handles both mockup and real scenarios.

---

272-280: LGTM!

The GetDepthInMainChain method correctly returns the depth of the transaction output in the main chain and handles both mockup and real scenarios.

---

282-295: LGTM!

The GetDepthInMempool method correctly returns the depth of the transaction output in the mempool and handles both mockup and real scenarios.

---

407-407: LGTM!

The changes correctly handle the decryption and extraction of the seed from the mnemonic container.

---

469-469: LGTM!

The changes correctly handle the decryption and extraction of the seed from the mnemonic container, with an additional check for an empty seed.

---

494-494: LGTM!

The changes correctly handle the derivation of child keys and updating of metadata.

---

4535-4553: LGTM!

The GetFee method correctly calculates the fee for a transaction based on the coin control settings and transaction size.

---

4556-4570: LGTM!

The GetTransparentTxouts method correctly retrieves all transparent transaction outputs from the wallet and ensures proper locking.

---

4572-4615: LGTM!

The SignTransparentInputs method correctly signs the transparent inputs of a transaction, with proper handling of signature creation and validation, and appropriate error handling.

---

4618-4694: LGTM!

The CheckTransparentTransactionSanity method correctly checks the sanity of a transparent transaction, including various checks for size, fee, and outputs.

---

4696-4866: LGTM!

The CreateTransaction method correctly creates a transaction with the specified recipients and coin control settings, including proper handling of input selection, fee calculation, and change output creation.

---

4868-4932: LGTM!

The GetAvailableInputs method correctly retrieves the available inputs for a transaction based on the specified coin control settings, ensuring compatibility with coin control settings.

---

4942-5201: LGTM!

The GetInputsForTx method correctly retrieves the inputs for a transaction based on the specified coin control settings and required amount, including proper handling of input selection, fee calculation, and validation.

---

5197-5201: LGTM!

The explicit instantiation of the GetInputsForTx method for CTransparentTxout is appropriate.

[coderabbitai]

Actionable comments posted: 3

Outside diff range, codebase verification and nitpick comments (3)

src/wallet/txbuilder.h (1)

30-44: Update all references to TxBuilder to SigmaTxBuilderSuperclass.

The class TxBuilder has been renamed to SigmaTxBuilderSuperclass, but some references to TxBuilder still exist in the codebase. Ensure that all references are updated to maintain consistency.

• src/wallet/test/sigmatxbuilder_tests.cpp: Update references to TestTxBuilder to reflect the new class name SigmaTxBuilderSuperclass.

The method GetInputs is correctly using SigmaTxBuilderInputSigner.

Analysis chain

Verify the class renaming and method signature change.

The class TxBuilder has been renamed to SigmaTxBuilderSuperclass. Ensure that all references to TxBuilder have been updated to SigmaTxBuilderSuperclass.

The method GetInputs now accepts a vector of std::unique_ptr<SigmaTxBuilderInputSigner>. Ensure that all calls to this method use the correct type.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all references to `TxBuilder` have been updated to `SigmaTxBuilderSuperclass` and calls to `GetInputs` use `SigmaTxBuilderInputSigner`.

# Test: Search for references to `TxBuilder`. Expect: No occurrences of `TxBuilder`.
rg --type cpp $'TxBuilder'

# Test: Search for calls to `GetInputs`. Expect: Only occurrences with `SigmaTxBuilderInputSigner`.
rg --type cpp -A 5 $'GetInputs'

Length of output: 8250

src/script/script.cpp (1)

220-228: Maintainability: Add comments explaining script structure.

Adding a brief comment explaining the expected structure of a pay-to-public-key script would enhance the maintainability of the code by making it easier for future developers to understand the rationale behind the checks.

bool CScript::IsPayToPublicKey() const {
    if (size() != 35) return false;

    opcodetype opcode;
    const_iterator pc = begin();
+   // Expect the script to start with a push operation of the public key (33 bytes) followed by OP_CHECKSIG
    if (!GetOp(pc, opcode) || opcode != OP_PUSHDATA1) return false;
    GetOp(pc, opcode);
    return opcode == OP_CHECKSIG;
}

src/validation.cpp (1)

1199-1199: Consider adding a comment to explain the logging statement.

The addition of the logging statement enhances the debugging capabilities by providing detailed information about missing transaction inputs. Adding a brief comment above the logging statement will help in quickly understanding the context of the log message without diving deep into the logic.

+ // Log missing transaction input details for debugging
  LogPrintf("%s(): Couldn't find input %s-%d for tx %s\n", __func__, txin.prevout.hash.GetHex(), txin.prevout.n, tx.GetHash().GetHex());

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 1dac305 and fbea637.

Files selected for processing (18)

• qa/rpc-tests/txn_clone.py (4 hunks)
• src/Makefile.test.include (1 hunks)
• src/llmq/quorums_instantsend.h (2 hunks)
• src/primitives/transaction.h (2 hunks)
• src/script/script.cpp (1 hunks)
• src/script/script.h (1 hunks)
• src/sync.cpp (3 hunks)
• src/test/test_bitcoin.cpp (1 hunks)
• src/validation.cpp (1 hunks)
• src/wallet/coincontrol.h (5 hunks)
• src/wallet/sigmaspendbuilder.cpp (3 hunks)
• src/wallet/sigmaspendbuilder.h (2 hunks)
• src/wallet/test/createtransaction_tests.cpp (1 hunks)
• src/wallet/test/sigmatxbuilder_tests.cpp (5 hunks)
• src/wallet/txbuilder.cpp (3 hunks)
• src/wallet/txbuilder.h (1 hunks)
• src/wallet/wallet.cpp (6 hunks)
• src/wallet/wallet.h (8 hunks)

Files skipped from review due to trivial changes (2)

• src/primitives/transaction.h
• src/sync.cpp

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

• src/llmq/quorums_instantsend.h
• src/wallet/test/createtransaction_tests.cpp

Additional context used

Ruff

qa/rpc-tests/txn_clone.py

99-99: assert_equal may be undefined, or defined from star imports

(F405)

---

101-101: assert_equal may be undefined, or defined from star imports

(F405)

---

143-143: assert_equal may be undefined, or defined from star imports

(F405)

Additional comments not posted (59)

src/wallet/sigmaspendbuilder.h (2)

22-22: Verify the method signature change.

The method GetInputs now accepts a vector of std::unique_ptr<SigmaTxBuilderInputSigner> instead of std::unique_ptr<InputSigner>. Ensure that all calls to this method use the correct type.

Verification successful

---

Verification successful. All calls to the GetInputs method correctly use std::unique_ptr<SigmaTxBuilderInputSigner>.

• src/wallet/sigmaspendbuilder.cpp
• src/wallet/test/sigmatxbuilder_tests.cpp
• src/wallet/txbuilder.h
• src/wallet/txbuilder.cpp
• src/wallet/sigmaspendbuilder.h

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all calls to `GetInputs` use `SigmaTxBuilderInputSigner`.

# Test: Search for calls to `GetInputs`. Expect: Only occurances with `SigmaTxBuilderInputSigner`.
rg --type cpp -A 5 $'GetInputs'

Length of output: 4268

---

Script:

#!/bin/bash
# Check the context around the identified lines to confirm the use of std::unique_ptr<SigmaTxBuilderInputSigner>

# Check context in sigmaspendbuilder.cpp
rg --type cpp -A 10 -B 10 'GetInputs' src/wallet/sigmaspendbuilder.cpp

# Check context in sigmatxbuilder_tests.cpp
rg --type cpp -A 10 -B 10 'GetInputs' src/wallet/test/sigmatxbuilder_tests.cpp

# Check context in txbuilder.h
rg --type cpp -A 10 -B 10 'GetInputs' src/wallet/txbuilder.h

# Check context in txbuilder.cpp
rg --type cpp -A 10 -B 10 'GetInputs' src/wallet/txbuilder.cpp

# Check context in sigmaspendbuilder.h
rg --type cpp -A 10 -B 10 'GetInputs' src/wallet/sigmaspendbuilder.h

Length of output: 3498

---

10-10: Verify the change in inheritance.

The class now inherits from SigmaTxBuilderSuperclass instead of TxBuilder. Ensure that SigmaTxBuilderSuperclass provides the necessary functionality and that all inherited methods are correctly overridden.

Verification successful

---

Verification complete: Inheritance and method signature change.

The class SigmaSpendBuilder correctly inherits from SigmaTxBuilderSuperclass, and the method GetInputs has the appropriate signature matching the superclass.

• SigmaSpendBuilder inherits from SigmaTxBuilderSuperclass.
• GetInputs method signature is CAmount GetInputs(std::vector<std::unique_ptr<SigmaTxBuilderInputSigner>>& signers, CAmount required) override.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the definition of `SigmaTxBuilderSuperclass` and its methods.

# Test: Search for the definition of `SigmaTxBuilderSuperclass`. Expect: Class definition with necessary methods.
ast-grep --lang cpp --pattern 'class SigmaTxBuilderSuperclass { $$$ }'

Length of output: 417

---

Script:

#!/bin/bash
# Description: Verify the definition of `SigmaTxBuilderSuperclass` and its methods.

# Test: Search for the definition of `SigmaTxBuilderSuperclass`. Expect: Class definition with necessary methods.
ast-grep --lang cpp --pattern 'class SigmaTxBuilderSuperclass { $$$ }'

# Additional search for any mention of SigmaTxBuilderSuperclass to ensure it exists in the codebase.
rg 'SigmaTxBuilderSuperclass'

Length of output: 1648

---

Script:

#!/bin/bash
# Description: Verify the method signature change for `GetInputs` in `SigmaSpendBuilder`.

# Test: Search for the definition of `GetInputs` method in `SigmaSpendBuilder`.
ast-grep --lang cpp --pattern 'class SigmaSpendBuilder { $$$ GetInputs($_$) { $$$ } $$$ }'

# Additional search for any mention of GetInputs to ensure it exists in the codebase.
rg 'GetInputs'

Length of output: 1702

src/wallet/txbuilder.h (1)

16-25: Verify the class renaming.

The class InputSigner has been renamed to SigmaTxBuilderInputSigner. Ensure that all references to InputSigner have been updated to SigmaTxBuilderInputSigner.

Verification successful

---

Verify the class renaming.

The class InputSigner has been successfully renamed to SigmaTxBuilderInputSigner across the codebase. All references to InputSigner have been updated accordingly.

• Verified in the following files:
  • src/wallet/txbuilder.cpp
  • src/wallet/sigmaspendbuilder.h
  • src/wallet/txbuilder.h
  • src/wallet/test/sigmatxbuilder_tests.cpp
  • src/wallet/sigmaspendbuilder.cpp

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all references to `InputSigner` have been updated to `SigmaTxBuilderInputSigner`.

# Test: Search for references to `InputSigner`. Expect: No occurrences of `InputSigner`.
rg --type cpp $'InputSigner'

Length of output: 2522

src/wallet/coincontrol.h (2)

9-10: Ensure correct initialization of new member variables.

The new member variables fAllowUnconfirmed, nMaxInputs, and nMaxSize enhance the functionality of the class. Ensure that these variables are correctly initialized and used throughout the codebase.

Verification successful

---

The new member variables fAllowUnconfirmed, nMaxInputs, and nMaxSize are correctly initialized and used throughout the codebase.

The initialization and usage of these variables are well-covered in both the main code and the test files.

• fAllowUnconfirmed: Initialized and used in src/wallet/wallet.cpp, tested in src/wallet/test/createtransaction_tests.cpp.
• nMaxInputs: Initialized in src/wallet/coincontrol.h, used in src/wallet/wallet.cpp, tested in src/wallet/test/createtransaction_tests.cpp.
• nMaxSize: Initialized in src/wallet/coincontrol.h, used in src/wallet/wallet.cpp, tested in src/wallet/test/createtransaction_tests.cpp.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the initialization and usage of new member variables.

# Test: Search for references to `fAllowUnconfirmed`. Expect: Correct initialization and usage.
rg --type cpp $'fAllowUnconfirmed'

# Test: Search for references to `nMaxInputs`. Expect: Correct initialization and usage.
rg --type cpp $'nMaxInputs'

# Test: Search for references to `nMaxSize`. Expect: Correct initialization and usage.
rg --type cpp $'nMaxSize'

Length of output: 4081

---

107-110: Verify the new method GetSelectedSize.

The new method GetSelectedSize provides additional functionality. Ensure that the method is correctly used throughout the codebase.

Verification successful

Let's extract the context around the usage of GetSelectedSize in src/wallet/wallet.cpp to verify its correct implementation.

---

The method GetSelectedSize is correctly used in the codebase.

• The method is used in a conditional statement to validate the number of selected coin control inputs, ensuring they match the expected size.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of the new method `GetSelectedSize`.

# Test: Search for calls to `GetSelectedSize`. Expect: Correct usage.
rg --type cpp $'GetSelectedSize'

Length of output: 186

---

Script:

#!/bin/bash
# Extract context around the usage of `GetSelectedSize` in `src/wallet/wallet.cpp`
rg --type cpp -A 10 -B 10 'GetSelectedSize' src/wallet/wallet.cpp

Length of output: 1209

src/wallet/sigmaspendbuilder.cpp (3)

132-132: LGTM! Verify the usage of the new method signature.

The change in method signature to accept std::unique_ptr<SigmaTxBuilderInputSigner> is approved.

Ensure that all calls to GetInputs are compatible with the new signature.

Verification successful

---

Verify the usage of the new method signature.

The change in method signature to accept std::unique_ptr<SigmaTxBuilderInputSigner> is approved. Ensure that all calls to GetInputs are compatible with the new signature.

• src/wallet/txbuilder.h: Ensure the abstract method declaration is updated.
• src/wallet/sigmaspendbuilder.cpp: Verify the implementation.
• src/wallet/sigmaspendbuilder.h: Ensure the method declaration matches the new signature.
• src/wallet/txbuilder.cpp: Verify the call to GetInputs.
• src/wallet/test/sigmatxbuilder_tests.cpp: Ensure the test implementation is updated.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all calls to `GetInputs` are compatible with the new signature.

# Test: Search for the method usage. Expect: Only compatible references.
rg --type cpp -A 5 $'GetInputs'

Length of output: 4268

---

21-21: LGTM! Verify the usage of the new base class.

The change in inheritance from InputSigner to SigmaTxBuilderInputSigner is approved.

Ensure that all references to SigmaSpendSigner are compatible with the new base class.

Verification successful

---

Verification successful.

All references to SigmaSpendSigner are contained within src/wallet/sigmaspendbuilder.cpp and are compatible with the new base class SigmaTxBuilderInputSigner.

• SigmaSpendSigner class definition and usage are correctly updated.
• No other references found across the codebase.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all references to `SigmaSpendSigner` are compatible with the new base class.

# Test: Search for the class usage. Expect: Only compatible references.
rg --type cpp -A 5 $'SigmaSpendSigner'

Length of output: 1620

---

Script:

#!/bin/bash
# Description: Verify all references to `SigmaSpendSigner` across the codebase to ensure compatibility with the new base class.

# Test: Search for all references to `SigmaSpendSigner` across the codebase.
rg --type cpp 'SigmaSpendSigner'

Length of output: 441

---

111-114: LGTM! Verify the usage of the new base class.

The change in inheritance from TxBuilder to SigmaTxBuilderSuperclass is approved.

Ensure that all references to SigmaSpendBuilder are compatible with the new base class.

Verification successful

---

Verified: The integration of SigmaSpendBuilder with the new base class SigmaTxBuilderSuperclass is correct and compatible with the existing logic.

• The class SigmaSpendBuilder correctly inherits from SigmaTxBuilderSuperclass.
• The methods GetInputs and GetChanges in SigmaSpendBuilder properly override methods from SigmaTxBuilderSuperclass.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all references to `SigmaSpendBuilder` are compatible with the new base class.

# Test: Search for the class usage. Expect: Only compatible references.
rg --type cpp -A 5 $'SigmaSpendBuilder'

Length of output: 2913

---

Script:

#!/bin/bash
# Description: Verify the definition and methods of the new base class `SigmaTxBuilderSuperclass`.

# Locate the definition of `SigmaTxBuilderSuperclass`
rg --type cpp -A 5 $'class SigmaTxBuilderSuperclass'

# Locate the methods in `SigmaTxBuilderSuperclass`
rg --type cpp -A 5 $'SigmaTxBuilderSuperclass::'

Length of output: 1833

qa/rpc-tests/txn_clone.py (3)

105-105: Validate the correctness of the test logic.

The removal of references to tx2 simplifies the test and focuses on the transaction from the "foo" account.

Ensure the test logic and assertions are correct.

Verification successful

---

Test logic and assertions are correct.

The removal of references to tx2 does not negatively impact the test. The test logic correctly handles the transactions and balances involving the "foo" and "bar" accounts.

• The definition and usage of fund_bar_tx are appropriate.
• Assertions validate the expected outcomes effectively.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Validate the correctness of the test logic.

# Test: Run the test script. Expect: No errors.
python3 qa/rpc-tests/txn_clone.py

Length of output: 110

---

Script:

#!/bin/bash
# Find the definition of `fund_bar_tx` in the test file
rg 'fund_bar_tx' -A 5 qa/rpc-tests/txn_clone.py

# Find all assertions in the test file to understand the test logic
rg 'assert' qa/rpc-tests/txn_clone.py

Length of output: 2326

---

99-99: Validate the correctness of the test logic.

The removal of references to tx2 simplifies the test and focuses on the transaction from the "foo" account.

Ensure the test logic and assertions are correct.

Verification successful

---

The test logic and assertions are correct.

The removal of references to tx2 simplifies the test and focuses on the transaction from the "foo" account. The assertion assert_equal(self.nodes[1].getbalance("from0"), -(tx1["amount"])) correctly verifies the expected balance for the "from0" account on node1.

• The test logic correctly handles the transactions and balance updates.
• The assertions are valid and consistent with the expected outcomes.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Validate the correctness of the test logic.

# Test: Run the test script. Expect: No errors.
python3 qa/rpc-tests/txn_clone.py

Length of output: 110

---

Script:

#!/bin/bash
# Extract lines around the assertion in qa/rpc-tests/txn_clone.py for context
sed -n '90,110p' qa/rpc-tests/txn_clone.py

Length of output: 954

---

Script:

#!/bin/bash
# Extract the definition of the run_test method to understand the test logic
ast-grep --lang python --pattern $'def run_test(self):\n    $$$' qa/rpc-tests/txn_clone.py

Length of output: 9331

Tools

Ruff

99-99: assert_equal may be undefined, or defined from star imports

(F405)

---

143-143: Validate the correctness of the test logic.

The removal of references to tx2 simplifies the test and focuses on the transaction from the "foo" account.

Ensure the test logic and assertions are correct.

Verification successful

---

Test logic and assertions are correct.

The removal of references to tx2 simplifies the test and focuses on the transaction from the "foo" account. The assertion correctly verifies that the "from0" account balance on node1 is the negative of tx1["amount"], consistent with the test setup and expected behavior.

• The test logic is correct and the assertion is valid.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Validate the correctness of the test logic.

# Test: Run the test script. Expect: No errors.
python3 qa/rpc-tests/txn_clone.py

Length of output: 110

---

Script:

#!/bin/bash
# Extract relevant lines around the assertion to understand the test logic
rg -A 20 'assert_equal\(self.nodes\[1\]\.getbalance\("from0", 0\), \-\(tx1\["amount"\]\)\)' qa/rpc-tests/txn_clone.py

Length of output: 249

---

Script:

#!/bin/bash
# Extract more lines from the script to understand the setup of tx1 and transactions involving the "from0" account
rg -A 50 'tx1\s*=' qa/rpc-tests/txn_clone.py

Length of output: 4752

Tools

Ruff

143-143: assert_equal may be undefined, or defined from star imports

(F405)

src/wallet/test/sigmatxbuilder_tests.cpp (4)

15-15: LGTM! Verify the usage of the new base class.

The change in inheritance from InputSigner to SigmaTxBuilderInputSigner is approved.

Ensure that all references to TestInputSigner are compatible with the new base class.

Verification successful

---

Verification Successful!

The change in inheritance from InputSigner to SigmaTxBuilderInputSigner for the TestInputSigner class is compatible with its usage in the codebase. No issues were found.

• The constructors correctly initialize the base class.
• The class is used appropriately in a vector of SigmaTxBuilderInputSigner pointers.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all references to `TestInputSigner` are compatible with the new base class.

# Test: Search for the class usage. Expect: Only compatible references.
rg --type cpp -A 5 $'TestInputSigner'

Length of output: 1727

---

37-37: LGTM! Verify the usage of the new base class.

The change in inheritance from TxBuilder to SigmaTxBuilderSuperclass is approved.

Ensure that all references to TestTxBuilder are compatible with the new base class.

---

54-54: LGTM! Verify the usage of the new method signature.

The change in method signature to accept std::unique_ptr<SigmaTxBuilderInputSigner> is approved.

Ensure that all calls to GetInputs are compatible with the new signature.

Verification successful

---

Verify the compatibility of the new method signature for GetInputs.

The GetInputs method signature has been updated to use std::unique_ptr<SigmaTxBuilderInputSigner>. The following instances need to be verified for compatibility:

• Implementation: src/wallet/sigmaspendbuilder.cpp
• Test: src/wallet/test/sigmatxbuilder_tests.cpp
• Usage: src/wallet/txbuilder.cpp

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all calls to `GetInputs` are compatible with the new signature.

# Test: Search for the method usage. Expect: Only compatible references.
rg --type cpp -A 5 $'GetInputs'

Length of output: 4268

---

187-189: LGTM! Verify the correctness of the lambda function.

The change in the lambda function to use SigmaTxBuilderInputSigner type for the signers is approved.

Ensure the lambda function operates correctly with the updated type.

src/Makefile.test.include (1)

211-212: Verify the impact on testing coverage.

The BITCOIN_TESTS variable has been updated to include wallet/test/sigmatxbuilder_tests.cpp and wallet/test/createtransaction_tests.cpp, and remove wallet/test/txbuilder_tests.cpp. Ensure that the new tests provide adequate coverage and that the removal of txbuilder_tests.cpp does not negatively impact the testing suite.

Verification successful

---

Verification successful.

The BITCOIN_TESTS variable has been correctly updated to include wallet/test/sigmatxbuilder_tests.cpp and wallet/test/createtransaction_tests.cpp, and remove wallet/test/txbuilder_tests.cpp. There are no remaining references to txbuilder_tests.cpp in the repository, ensuring that the changes do not negatively impact the testing suite.

• wallet/test/sigmatxbuilder_tests.cpp exists.
• wallet/test/createtransaction_tests.cpp exists.
• wallet/test/txbuilder_tests.cpp has been removed from the build configuration.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact on testing coverage.

# Test: Search for the existence of the new test files and the removal of the old test file.
# Expect: The new test files should exist and the old test file should be removed.

fd 'sigmatxbuilder_tests.cpp' --type f
fd 'createtransaction_tests.cpp' --type f
fd 'txbuilder_tests.cpp' --type f

Length of output: 240

---

Script:

#!/bin/bash
# Description: Verify if `txbuilder_tests.cpp` is still being referenced in the Makefile or any other build configuration files.

# Test: Search for references to `txbuilder_tests.cpp` in the Makefile and other build configuration files.
# Expect: There should be no references to `txbuilder_tests.cpp`.

# Search in the Makefile
rg 'txbuilder_tests.cpp' Makefile

# Search in other build configuration files
fd -e mk -e make -e cmake -x rg 'txbuilder_tests.cpp' {}

Length of output: 141

---

Script:

#!/bin/bash
# Description: Verify if `txbuilder_tests.cpp` is still being referenced in the repository.

# Test: Search for references to `txbuilder_tests.cpp` in all files within the repository.
# Expect: There should be no references to `txbuilder_tests.cpp`.

rg 'txbuilder_tests.cpp'

Length of output: 91

src/wallet/txbuilder.cpp (4)

22-30: LGTM! Consistent renaming and updates.

The class InputSigner has been renamed to SigmaTxBuilderInputSigner, and its constructor and destructor have been updated accordingly. The changes are consistent and correctly implemented.

---

34-38: LGTM! Consistent renaming and updates.

The class TxBuilder has been renamed to SigmaTxBuilderSuperclass, and its constructor and destructor have been updated accordingly. The changes are consistent and correctly implemented.

---

268-270: LGTM! Consistent renaming and updates.

The AdjustFee function has been updated to reflect the new class name SigmaTxBuilderSuperclass. The changes are consistent and correctly implemented.

---

Line range hint 42-268:
Verify the logic in the Build function.

The Build function has been updated to use SigmaTxBuilderInputSigner. Ensure that the logic is correct and that the function works as intended with the new class structure.

src/test/test_bitcoin.cpp (1)

112-113: LGTM! Correct implementation of mnemonic generation.

The TestingSetup constructor has been updated to generate a new mnemonic for the wallet. The changes are correctly implemented and align with the new key initialization approach.

src/script/script.h (1)

665-665: LGTM! Consistent method declaration.

The method declaration is consistent with the implementation in script.cpp.

src/wallet/wallet.h (10)

656-695: Ensure methods that do not modify class state are marked as const.

The class interface looks good. However, methods like GetHash, GetOutpoint, GetValue, GetScriptPubkey, etc., should be marked as const to indicate they do not modify the class state.

+    uint256 GetHash() const;
+    COutPoint GetOutpoint() const;
+    CAmount GetValue() const;
+    CScript GetScriptPubkey() const;
+    size_t GetMarginalSpendSize(std::vector<CTransparentTxout>& previousInputs) const;
+    bool IsMine(const CCoinControl* coinControl) const;
+    bool IsSpendable() const;
+    bool IsLocked() const;
+    bool IsAbandoned() const;
+    bool IsCoinTypeCompatible(const CCoinControl* coinControl) const;
+    bool IsLLMQInstantSendLocked() const;
+    bool IsCoinBase() const;
+    bool IsFromMe() const;
+    unsigned int GetDepthInMainChain() const;
+    unsigned int GetDepthInMempool() const;

---

652-654: Consider documenting the purpose of AbstractTxoutType.

The enum AbstractTxoutType looks good. Documenting its purpose and potential future extensions would be beneficial.

---

723-723: LGTM!

The method AddToSpends(const COutPoint &outpoint, const uint256 &wtxid) looks good.

---

725-725: LGTM!

The method AddToSpends(const uint256 &wtxid) looks good.

---

728-728: LGTM!

The method GetTransparentTxouts() const looks good.

---

1093-1093: LGTM!

The method SignTransparentInputs looks good.

---

1094-1095: LGTM!

The method CheckTransparentTransactionSanity looks good.

---

1102-1105: LGTM!

The method CreateTransaction with the old signature looks good.

---

709-709: LGTM!

The typedef TxSpends move to the public section looks good.

---

1097-1100: LGTM! But verify the function usage in the codebase.

The method signature modification looks good. Ensure that all function calls to CreateTransaction match the new signature.

src/wallet/wallet.cpp (27)

7-7: LGTM!

The inclusion of script/standard.h appears necessary for the new functionality related to transparent transactions.

---

88-90: LGTM!

The IsTransparentTxout method correctly identifies transparent outputs.

---

92-94: LGTM!

The GetHash method correctly returns the hash of the outpoint.

---

96-98: LGTM!

The GetOutpoint method correctly returns the outpoint.

---

100-103: LGTM!

The GetValue method correctly returns the value of the transaction output.

---

105-107: LGTM!

The GetScriptPubkey method correctly returns the scriptPubKey of the transaction output.

---

156-172: LGTM!

The IsMine method correctly checks if the transparent output belongs to the wallet.

---

174-192: LGTM!

The IsSpendable method correctly checks if the transparent output is spendable.

---

194-202: LGTM!

The IsLocked method correctly checks if the transparent output is locked.

---

204-212: LGTM!

The IsAbandoned method correctly checks if the transparent output is abandoned.

---

214-231: LGTM!

The IsCoinTypeCompatible method correctly checks if the transparent output is compatible with the specified coin type.

---

233-241: LGTM!

The IsLLMQInstantSendLocked method correctly checks if the transparent output is locked by LLMQ InstantSend.

---

243-251: LGTM!

The IsCoinBase method correctly checks if the transparent output is from a coinbase transaction.

---

253-270: LGTM!

The IsFromMe method correctly checks if the transparent output is from the wallet.

---

272-280: LGTM!

The GetDepthInMainChain method correctly returns the depth of the transparent output in the main chain.

---

282-295: LGTM!

The GetDepthInMempool method correctly returns the depth of the transparent output in the mempool.

---

88-154: LGTM!

The changes to the CreateTransaction method correctly include handling for transparent transaction outputs.

Also applies to: 4714-4716, 4719-4866

---

407-407: LGTM!

The changes to the handling of the mnemonic container and seed are minor and appear correct.

---

469-469: LGTM!

The changes to the handling of the mnemonic container and seed are minor and appear correct.

---

494-499: LGTM!

The changes to the derivation of child keys are minor and appear correct.

---

4535-4554: LGTM!

The GetFee method correctly calculates the fee for a transaction, considering various coin control parameters.

---

4556-4570: LGTM!

The GetTransparentTxouts method correctly retrieves transparent transaction outputs from the wallet.

---

4572-4615: LGTM!

The SignTransparentInputs method correctly signs the transparent inputs of a transaction.

---

4618-4694: LGTM!

The CheckTransparentTransactionSanity method correctly checks the sanity of a transparent transaction.

---

4696-4866: LGTM!

The CreateTransaction method correctly creates a transaction, considering transparent transaction outputs.

---

4868-4932: LGTM!

The GetAvailableInputs method correctly retrieves available inputs for a transaction, considering various coin control parameters.

---

4942-5202: LGTM!

The GetInputsForTx method correctly retrieves inputs for a transaction, considering various coin control parameters and constraints.

[coderabbitai]

Correctness and Logic: Verify initial opcode.

The method correctly identifies a subset of pay-to-public-key scripts but does not account for the initial opcodes that push the public key onto the stack. A more robust check would involve verifying that the script starts with an appropriate push operation followed by OP_CHECKSIG.

bool CScript::IsPayToPublicKey() const {
    if (size() != 35) return false;

    opcodetype opcode;
    const_iterator pc = begin();
+   // Expect the script to start with a push operation of the public key (33 bytes) followed by OP_CHECKSIG
+   if (!GetOp(pc, opcode) || opcode != OP_PUSHDATA1) return false;
    GetOp(pc, opcode);
    return opcode == OP_CHECKSIG;
}

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

	bool CScript::IsPayToPublicKey() const {
	if (size() != 35) return false;
	opcodetype opcode;
	const_iterator pc = begin();
	GetOp(pc, opcode);
	GetOp(pc, opcode);
	return opcode == OP_CHECKSIG;
	}
	bool CScript::IsPayToPublicKey() const {
	if (size() != 35) return false;
	opcodetype opcode;
	const_iterator pc = begin();
	// Expect the script to start with a push operation of the public key (33 bytes) followed by OP_CHECKSIG
	if (!GetOp(pc, opcode) || opcode != OP_PUSHDATA1) return false;
	GetOp(pc, opcode);
	return opcode == OP_CHECKSIG;
	}

---

Performance: Directly access relevant opcodes.

The method iterates through the script opcodes without checking the first opcode directly. Since the script format for a pay-to-public-key is relatively fixed, directly accessing the relevant opcodes could improve readability and performance.

bool CScript::IsPayToPublicKey() const {
    if (size() != 35) return false;

    opcodetype opcode;
    const_iterator pc = begin();
    if (!GetOp(pc, opcode) || opcode != OP_PUSHDATA1) return false;
    return GetOp(pc, opcode) && opcode == OP_CHECKSIG;
}

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

	bool CScript::IsPayToPublicKey() const {
	if (size() != 35) return false;
	opcodetype opcode;
	const_iterator pc = begin();
	GetOp(pc, opcode);
	GetOp(pc, opcode);
	return opcode == OP_CHECKSIG;
	}
	bool CScript::IsPayToPublicKey() const {
	if (size() != 35) return false;
	opcodetype opcode;
	const_iterator pc = begin();
	if (!GetOp(pc, opcode) || opcode != OP_PUSHDATA1) return false;
	return GetOp(pc, opcode) && opcode == OP_CHECKSIG;
	}

[aleflm]

This is a valid suggestion:

+   // Expect the script to start with a push operation of the public key (33 bytes) followed by OP_CHECKSIG
+   if (!GetOp(pc, opcode) || opcode != OP_PUSHDATA1) return false;

Second suggestion about optimization is not that valid. Any compiler will optimize this.

@psolstice I can change the code an submit or we can keep what we have. . But depends on your final decision.

[coderabbitai]

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

[coderabbitai]

Consider documenting the rationale behind the specific sizes.

The method GetMarginalSpendSize handles various output types and throws an exception for unsupported types. It would be beneficial to add comments explaining the basis for the specific sizes used in the switch-case structure.

// Add comments explaining the basis for the specific sizes used in the switch-case structure.

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between fbea637 and f603de6.

Files selected for processing (1)

• src/wallet/wallet.cpp (6 hunks)

Additional comments not posted (9)

src/wallet/wallet.cpp (9)

88-90: LGTM!

The method correctly identifies transparent transaction outputs.

---

92-94: LGTM!

The method correctly returns the hash of the transaction output.

---

96-98: LGTM!

The method correctly returns the outpoint of the transaction output.

---

100-103: LGTM!

The method correctly returns the value of the transaction output after asserting it's not null.

---

105-107: LGTM!

The method correctly returns the script public key of the transaction output.

---

109-154: Consider documenting the rationale behind the specific sizes.

The method handles various output types and throws an exception for unsupported types. It would be beneficial to add comments explaining the basis for the specific sizes used in the switch-case structure.

---

156-172: LGTM!

The method correctly determines if the transaction output belongs to the wallet.

---

174-192: LGTM!

The method correctly determines if the transaction output is spendable.

---

194-202: LGTM!

The method correctly determines if the transaction output is locked.

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f603de6 and 3423301.

Files selected for processing (1)

• src/wallet/wallet.cpp (6 hunks)

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

• src/wallet/wallet.cpp

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 3423301 and 11e8a40.

Files selected for processing (1)

• src/wallet/rpcwallet.cpp (1 hunks)

Additional comments not posted (1)

src/wallet/rpcwallet.cpp (1)

3084-3085: LGTM! Verify the impact of the new locking mechanism.

The addition of the LOCK2 statement enhances thread safety by ensuring that both cs_main and pwallet->cs_wallet are locked simultaneously. This change is crucial for preventing race conditions when accessing shared resources.

Ensure that this change does not introduce deadlocks or other concurrency issues elsewhere in the codebase.

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 11e8a40 and 988f1c6.

Files selected for processing (2)

• qa/rpc-tests/dip3-deterministicmns.py (3 hunks)
• qa/rpc-tests/test_framework/mn_utils.py (1 hunks)

Additional context used

Ruff

qa/rpc-tests/dip3-deterministicmns.py

64-64: lock_mn_collateral may be undefined, or defined from star imports

(F405)

---

100-100: lock_mn_collateral may be undefined, or defined from star imports

(F405)

---

196-196: unlock_mn_collateral may be undefined, or defined from star imports

(F405)

Additional comments not posted (5)

qa/rpc-tests/test_framework/mn_utils.py (2)

49-51: Ensure correct usage of lockunspent.

The lock_mn_collateral function locks the collateral using lockunspent(False, ...). This is correct for locking, but ensure that the collateral is unlocked elsewhere when needed.

---

52-54: Ensure correct usage of lockunspent.

The unlock_mn_collateral function unlocks the collateral using lockunspent(True, ...). This is correct for unlocking, but ensure that the collateral is locked initially as needed.

qa/rpc-tests/dip3-deterministicmns.py (3)

100-100: Verify the import of lock_mn_collateral.

The function lock_mn_collateral is used here. Ensure it is correctly imported from mn_utils.py. The static analysis tool flagged a potential issue due to star imports.

Tools

Ruff

100-100: lock_mn_collateral may be undefined, or defined from star imports

(F405)

---

64-64: Verify the import of lock_mn_collateral.

The function lock_mn_collateral is used here. Ensure it is correctly imported from mn_utils.py. The static analysis tool flagged a potential issue due to star imports.

Verification successful

---

The import of lock_mn_collateral is correct.

The function lock_mn_collateral is defined in mn_utils.py and is imported in dip3-deterministicmns.py via a star import. There are no issues with its import.

• Definition found in: qa/rpc-tests/test_framework/mn_utils.py
• Imported via: from test_framework.mn_utils import *

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the import of `lock_mn_collateral` in `dip3-deterministicmns.py`.

# Test: Ensure `lock_mn_collateral` is defined in `mn_utils.py`.
ast-grep --lang python --pattern 'def lock_mn_collateral($_, $_)' qa/rpc-tests/test_framework/mn_utils.py

Length of output: 452

---

Script:

#!/bin/bash
# Search for the definition of `lock_mn_collateral` in the entire repository.
rg 'def lock_mn_collateral' --glob '*.py' -A 2

# Check how `lock_mn_collateral` is imported in `dip3-deterministicmns.py`.
rg 'import' qa/rpc-tests/dip3-deterministicmns.py -A 2

Length of output: 720

Tools

Ruff

64-64: lock_mn_collateral may be undefined, or defined from star imports

(F405)

---

196-196: Verify the import of unlock_mn_collateral.

The function unlock_mn_collateral is used here. Ensure it is correctly imported from mn_utils.py. The static analysis tool flagged a potential issue due to star imports.

Verification successful

---

unlock_mn_collateral is correctly imported via star import. The function unlock_mn_collateral is defined in mn_utils.py and is imported into dip3-deterministicmns.py using a star import (from test_framework.mn_utils import *). While the function is accessible, consider using explicit imports to avoid potential namespace issues.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the import of `unlock_mn_collateral` in `dip3-deterministicmns.py`.

# Test: Ensure `unlock_mn_collateral` is defined in `mn_utils.py`.
ast-grep --lang python --pattern 'def unlock_mn_collateral($_, $_)' qa/rpc-tests/test_framework/mn_utils.py

Length of output: 454

---

Script:

#!/bin/bash
# Search for the definition of `unlock_mn_collateral` in `mn_utils.py`.
rg 'def unlock_mn_collateral' qa/rpc-tests/test_framework/mn_utils.py

# Check the imports in `dip3-deterministicmns.py` to see if `unlock_mn_collateral` is imported.
rg 'unlock_mn_collateral' qa/rpc-tests/dip3-deterministicmns.py -A 3

Length of output: 370

---

Script:

#!/bin/bash
# Search for import statements in `dip3-deterministicmns.py` to verify the import of `unlock_mn_collateral`.
rg 'import' qa/rpc-tests/dip3-deterministicmns.py

Length of output: 384

Tools

Ruff

196-196: unlock_mn_collateral may be undefined, or defined from star imports

(F405)

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 988f1c6 and ae24627.

Files selected for processing (1)

• qa/rpc-tests/txn_doublespend.py (1 hunks)

Additional context used

Ruff

qa/rpc-tests/txn_doublespend.py

39-39: find_output may be undefined, or defined from star imports

(F405)

---

44-44: find_output may be undefined, or defined from star imports

(F405)

Additional comments not posted (2)

qa/rpc-tests/txn_doublespend.py (2)

39-39: Verify the definition or import of find_output.

The find_output function may be undefined or imported via a wildcard import. Ensure it is properly defined or imported to avoid runtime errors.

Tools

Ruff

39-39: find_output may be undefined, or defined from star imports

(F405)

---

44-44: LGTM! But verify the find_output usage.

The logic for unlocking the transaction output is correct and consistent with the test's objectives.

However, ensure that find_output is correctly defined or imported to avoid runtime errors.

Verification successful

---

find_output is correctly defined and accessible.

The function find_output is defined in qa/rpc-tests/test_framework/util.py, confirming its availability for use in the test script. No issues with its usage are anticipated.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the definition or import of `find_output`.

# Test: Search for the definition of `find_output`. Expect: At least one definition or import statement.
rg --type python $'def find_output' --or $'import find_output' --or $'from $_ import find_output'

Length of output: 525

---

Script:

#!/bin/bash
# Description: Verify the definition or import of `find_output`.

# Test: Search for the definition or import of `find_output`. Expect: At least one definition or import statement.
rg --type python -e 'def find_output' -e 'import find_output' -e 'from $_ import find_output'

Length of output: 126

---

Script:

#!/bin/bash
# Description: Verify the definition or import of `find_output`.

# Test: Search for the definition or import of `find_output` in Python files.
rg -e 'def find_output' -e 'import find_output' -e 'from $_ import find_output' --glob '*.py'

Length of output: 168

Tools

Ruff

44-44: find_output may be undefined, or defined from star imports

(F405)

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ae24627 and bd31b27.

Files selected for processing (2)

• src/wallet/test/createtransaction_tests.cpp (1 hunks)
• src/wallet/wallet.cpp (7 hunks)

Additional comments not posted (30)

src/wallet/test/createtransaction_tests.cpp (10)

14-18: LGTM!

The function correctly converts a CTxDestination to a string representation of a Bitcoin address.

---

20-24: LGTM!

The function correctly extracts a CTxDestination from a CScript and converts it to a string.

---

39-44: LGTM!

The function correctly generates a random destination script.

---

77-82: LGTM!

The function correctly creates a mock transparent output based on ownership.

---

84-90: LGTM!

The function correctly generates a vector of mock transparent outputs for testing.

---

92-98: LGTM!

The function correctly generates a vector of fake recipients for testing.

---

110-112: LGTM!

The function correctly asserts the scriptPubKey of a transaction output.

---

114-116: LGTM!

The function correctly asserts the value of a transaction output.

---

118-124: LGTM!

The function correctly asserts ownership of a transaction output.

---

126-138: LGTM!

The macros provide well-defined assertions for transaction inputs, outputs, and success/failure conditions.

src/wallet/wallet.cpp (20)

89-91: LGTM!

The method correctly checks if a transaction output is transparent.

---

93-95: LGTM!

The method correctly returns the hash of the transaction output.

---

97-99: LGTM!

The method correctly returns the outpoint of the transaction output.

---

101-104: LGTM!

The method correctly returns the value of the transaction output and ensures it is not null.

---

106-108: LGTM!

The method correctly returns the script public key of the transaction output.

---

110-155: Skip comment on documenting specific sizes.

The past comment about documenting the rationale behind specific sizes is still valid.

---

157-173: LGTM!

The method correctly checks if the transaction output belongs to the wallet.

---

175-193: LGTM!

The method correctly checks if the transaction output is spendable.

---

195-203: LGTM!

The method correctly checks if the transaction output is locked.

---

205-213: LGTM!

The method correctly checks if the transaction output is abandoned.

---

215-230: LGTM!

The method correctly checks if the transaction output is compatible with the specified coin type.

---

232-240: LGTM!

The method correctly checks if the transaction output is locked by LLMQ InstantSend.

---

242-250: LGTM!

The method correctly checks if the transaction output is a coinbase transaction.

---

252-260: LGTM!

The method correctly checks if the transaction output is from the wallet.

---

262-270: LGTM!

The method correctly returns the depth of the transaction output in the main chain.

---

272-285: LGTM!

The method correctly returns the depth of the transaction output in the mempool.

---

4525-4544: LGTM!

The method correctly calculates the transaction fee based on the transaction size and coin control parameters.

---

4546-4560: LGTM!

The method correctly retrieves all transparent transaction outputs from the wallet.

---

4562-4605: LGTM!

The method correctly signs transparent inputs for a transaction, with appropriate error handling.

---

4608-4684: LGTM!

The method correctly checks the sanity of a transparent transaction, including size, fee, and output standards.

[aleflm]

After reviewing the code and running the tests, this look good to me. Thanks.

[aleflm]

I went through the code one more time. I didn't see any deal breaker. Just added a few comments here and there to make it more readable.

[aleflm]

Although we might not apply this comment right now. But it worth keep this in mind.

I personally don't like to have test variables in production code. IMHO we can use something like test macro to eliminate test code from production code.

For example this came up in my search:

https://github.com/apache/incubator-resilientdb/blob/0541fe43bd1bbd69ecac5a1bc0d0cdd5ac04bc56/platform/statistic/stats.cpp#L44-L46

#ifdef TEST_MODE
  monitor_sleep_time_ = 1;
#endif