Refactor Public Transaction Creation #1348
Conversation
be2ea19
to
dd93e7e
Compare
dd93e7e
to
4e79d51
Compare
7a76600
to
a149ca0
Compare
da2fa2e
to
54529b8
Compare
src/primitives/transaction.h
Outdated
| @@ -171,7 +171,7 @@ class CTxOut | |||
| public: | |||
| CAmount nValue; | |||
| CScript scriptPubKey; | |||
| int nRounds; | |||
| int nRounds = -10; | |||
| @@ -113,6 +110,9 @@ class CInstantSendManager : public CRecoveredSigsListener | |||
| std::atomic_bool isNewInstantSendEnabled{false}; | |||
|
|
|||
| public: | |||
| CCriticalSection cs; | |||
| CInstantSendDb db; | |||
|
|
|||
There was a problem hiding this comment.
Is this the only way to implement the test? Wallet changes can certainly be done with an extra member function.
src/wallet/wallet.cpp
Outdated
| @@ -193,7 +405,7 @@ CPubKey CWallet::GetKeyFromKeypath(uint32_t nChange, uint32_t nChild, CKey& secr | |||
| MnemonicContainer mContainer = mnemonicContainer; | |||
| DecryptMnemonicContainer(mContainer); | |||
| SecureVector seed = mContainer.GetSeed(); | |||
| masterKey.SetMaster(&seed[0], seed.size()); | |||
| masterKey.SetMaster(&seed.at(0), seed.size()); | |||
There was a problem hiding this comment.
Using std::vector::data() method is the only possible improvement here. Other methods work but at() is just as bad or even worse than []
src/wallet/wallet.cpp
Outdated
| AssertLockHeld(wallet->cs_wallet); | ||
| AssertLockHeld(llmq::quorumInstantSendManager->cs); | ||
|
|
||
| return llmq::quorumInstantSendManager->db.GetInstantSendLockByTxid(GetHash()) != nullptr; |
There was a problem hiding this comment.
why not use quorumInstantSendManager->IsLocked() here?
src/wallet/wallet.cpp
Outdated
| // true, the wallet must be unlocked. nExtraPayloadSize should be set to the number of extra bytes in the transaction | ||
| // outside inputs/outputs (ie. LLMQ-related things). If fUseInstantSend is true, we will consider both locked and | ||
| // confirmed UTXOs to be eligible for input; if it is not, only confirmed UTXOs will be used as inputs. | ||
| bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletTx& wtxNew, CReserveKey* reservekey, |
There was a problem hiding this comment.
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
| * detect and report conflicts (double-spends or | ||
| * mutated transactions where the mutant gets mined). | ||
| */ | ||
| typedef std::multimap<COutPoint, uint256> TxSpends; |
There was a problem hiding this comment.
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?
| // vInputs may be returned with a 0-length; this will occur if there vRelevantTransactions is empty or if no | ||
| // transactions could cover the required fees. | ||
| template<typename AbstractTxout> | ||
| bool GetInputsForTx(const std::vector<AbstractTxout>& vRelevantTransactions, std::vector<AbstractTxout>& vInputs, |
|
@coderabbitai review |
WalkthroughThe 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
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 23
Configuration used: CodeRabbit UI
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
TxBuildertoSigmaTxBuilderSuperclassaligns 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
GetInputsmethod fromInputSignertoSigmaTxBuilderInputSigneris a logical step following the renaming ofInputSignertoSigmaTxBuilderInputSigner. This ensures type consistency and leverages the specialized functionality of the newSigmaTxBuilderInputSignerclass.src/wallet/txbuilder.h (2)
- 16-25: Renaming
InputSignertoSigmaTxBuilderInputSignerand 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
TxBuildertoSigmaTxBuilderSuperclassand 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_MINTSandWITH_1000to theCoinTypeenum 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, andnMaxSizeto theCCoinControlclass 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
SigmaSpendSignertoSigmaTxBuilderInputSignerand 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
SigmaSpendBuilderclass fromTxBuildertoSigmaTxBuilderSuperclassis consistent with the refactor's aim to specialize the transaction building process. This adjustment ensures thatSigmaSpendBuilderleverages the specialized functionality provided bySigmaTxBuilderSuperclass.- 132-132: Updating the
GetInputsmethod signature inSigmaSpendBuilderto useSigmaTxBuilderInputSignerinstead of the previousInputSignertype 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 thestd::setcontainer in theInvLockOrdersdeclaration. 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
InvLockOrdersto 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
AssertLockNotHeldInternalfunction 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
tx1andtx2is 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_txto the miner and handling the clone oftx1separately 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.cppandcreatetransaction_tests.cppwhile removingtxbuilder_tests.cppis a positive change, reflecting the refactoring and renaming in the codebase. Ensure that these new test files cover all the functionalities previously tested bytxbuilder_tests.cppand include additional tests for any new functionality introduced by the refactor.src/wallet/txbuilder.cpp (2)
- 22-30: Renaming
InputSignertoSigmaTxBuilderInputSignerand 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::Buildis 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
nRoundsdefault value from0to-10in theCTxOutclass:
- Ensure that this change does not affect the compatibility with existing transactions, especially those that might rely on the default value of
nRoundsfor certain logic or calculations.- Clarify the rationale behind choosing
-10as 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
nRoundscould 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) fornRoundsinCTxOut:
- 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
CTxOutstructure.src/script/script.h (1)
- 665-665: The addition of the
IsPayToPublicKey()method to theCScriptclass 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
GetAddresscorrectly converts aCTxDestinationto a Bitcoin address string usingCBitcoinAddress. This is a straightforward and commonly used pattern in Bitcoin-related code.- 39-43: The function
GetRandomDestgenerates a random script destination. It usesCKey::MakeNewKeyto 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:
GetFakeTransparentTxoutis an overloaded version that simplifies creating a fake transparent output by deciding whether the output is "mine" based on theisMineparameter. It delegates to the more specificGetFakeTransparentTxoutfunction 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
GetFakeTransparentTxoutsgenerates a vector of fake transparent outputs for given values. It demonstrates good use of modern C++ features like range-based for loops andemplace_backfor efficient vector operations.- 92-98:
GetFakeRecipientscreates a vector of recipients for given values, usingCRecipientstructs. This function is straightforward and utilizes efficient vector operations similar toGetFakeTransparentTxouts.- 110-112:
AssertVoutAddrcorrectly 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:
AssertVoutValueis a simple assertion function that checks if a transaction output's value matches the expected value. It usesBOOST_ASSERTeffectively.- 118-123:
AssertHasKeyensures 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_testsis correctly set up usingBOOST_FIXTURE_TEST_SUITEwithWalletTestingSetupas 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_requiredverifies 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_fundschecks 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_inputstests 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_backverifies 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_inputsensures 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_limitverifies 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_addressverifies 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_inputstests 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 methodsGetAvailableInputsandGetInputsForTxintroduce 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, andCreateTransactionhave 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
CreateTransactiondo 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, andlelantusjoinsplitbuilder.hsuggests 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
IsTransparentTxoutcorrectly 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
IsMinechecks if a transaction output belongs to the wallet, considering both spendable and watch-only outputs. The use ofAssertLockHeldensures thread safety. Ensure that the locking conventions are consistently applied throughout the wallet code to prevent data races.- 214-231: The method
IsCoinTypeCompatiblechecks 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
IsLLMQInstantSendLockedchecks 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
GetFeecalculates 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
SignTransparentInputssigns 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
CheckTransparentTransactionSanityperforms 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
CreateTransactionmethod's overload that does not takevTransparentTxoutsas 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
GetInputsForTxmethod 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.
| CCriticalSection cs; | ||
| CInstantSendDb db; |
There was a problem hiding this comment.
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.
| SigmaTxBuilderSuperclass::SigmaTxBuilderSuperclass(CWallet& wallet) noexcept : wallet(wallet) | ||
| { | ||
| } | ||
|
|
||
| TxBuilder::~TxBuilder() | ||
| SigmaTxBuilderSuperclass::~SigmaTxBuilderSuperclass() |
There was a problem hiding this comment.
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 SigmaTransactionBuilderCommittable 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.
| SigmaTxBuilderSuperclass::SigmaTxBuilderSuperclass(CWallet& wallet) noexcept : wallet(wallet) | |
| { | |
| } | |
| TxBuilder::~TxBuilder() | |
| SigmaTxBuilderSuperclass::~SigmaTxBuilderSuperclass() | |
| SigmaTxBuilder::SigmaTxBuilder(CWallet& wallet) noexcept : wallet(wallet) | |
| { | |
| } | |
| SigmaTxBuilder::~SigmaTxBuilder() |
| @@ -1196,6 +1196,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C | |||
| if (pfMissingInputs) { | |||
| *pfMissingInputs = true; | |||
| } | |||
| LogPrintf("%s(): Couldn't find input %s-%d for tx %s\n", __func__, txin.prevout.hash.GetHex(), txin.prevout.n, tx.GetHash().GetHex()); | |||
There was a problem hiding this comment.
The addition of the logging statement enhances the debugging capabilities by providing detailed information about missing transaction inputs. This is a valuable addition for troubleshooting and understanding the reasons behind transaction validation failures.
However, consider adding a brief comment above the logging statement to explain its purpose for future maintainability. This helps 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());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.
| LogPrintf("%s(): Couldn't find input %s-%d for tx %s\n", __func__, txin.prevout.hash.GetHex(), txin.prevout.n, tx.GetHash().GetHex()); | |
| // 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()); |
| #include "../base58.h" | ||
| #include "firo_params.h" | ||
| #include "univalue.h" | ||
|
|
||
| #include "coincontrol.h" | ||
| #include "policy/policy.h" | ||
| #include "hdmint/tracker.h" | ||
| #include "hdmint/wallet.h" | ||
|
|
||
| #include "primitives/mint_spend.h" | ||
|
|
||
| #include "bip47/paymentcode.h" | ||
|
|
||
| #include "../llmq/quorums_instantsend.h" | ||
|
|
||
| #include <algorithm> | ||
| #include <atomic> |
There was a problem hiding this comment.
📝 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.
| class CTransparentTxout; | ||
| enum AbstractTxoutType { | ||
| Transparent | ||
| }; | ||
|
|
||
| class CTransparentTxout { | ||
| public: | ||
| static bool IsTransparentTxout(const CTxOut& txout); | ||
|
|
||
| CTransparentTxout() = default; | ||
| CTransparentTxout(COutPoint outpoint, CTxOut txout): outpoint(outpoint), txout(txout), _isMockup(true) {} | ||
| CTransparentTxout(const CWallet* wallet, COutPoint outpoint, CTxOut txout): wallet(wallet), outpoint(outpoint), txout(txout) {}; | ||
|
|
||
| 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; | ||
|
|
||
| private: | ||
| const CWallet* wallet = nullptr; | ||
| COutPoint outpoint; | ||
| CTxOut txout; | ||
| bool _isMockup = false; | ||
|
|
||
| public: | ||
| bool _mockupIsMine = false; | ||
| bool _mockupIsMineWatchOnly = false; | ||
| bool _mockupIsSpent = false; | ||
| bool _mockupIsAbandoned = false; | ||
| bool _mockupIsLocked = false; | ||
| bool _mockupIsLLMQInstantSendLocked = false; | ||
| bool _mockupIsCoinBase = false; | ||
| unsigned int _mockupDepthInMainChain = 0; | ||
| }; |
There was a problem hiding this comment.
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
.cppfile to improve readability and compilation times.
|
|
||
| CReserveKey reservekey(pwalletMain); | ||
| pwalletMain->CreateTransaction(vRecipients, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, | ||
| &coinControl, true, 0, true, vTxouts); | ||
|
|
||
|
|
||
| ASSERT_FAILURE("Insufficient funds"); | ||
|
|
||
| fMasternodeMode = fMasternodeModeTemp; | ||
| } | ||
|
|
||
| vTxouts = GetFakeTransparentTxouts({1001 * COIN, 1000 * COIN}); | ||
|
|
||
| { | ||
| CCoinControl coinControl; | ||
| CWalletTx wtx; | ||
| CAmount nFeeRet = 0; | ||
| int nChangePosInOut = -1; | ||
| std::string strFailReason; | ||
|
|
||
| coinControl.nCoinType = CoinType::ONLY_NONDENOMINATED_NOT1000IFMN; | ||
|
|
||
| bool fMasternodeModeTemp = fMasternodeMode; | ||
| fMasternodeMode = true; | ||
|
|
||
| CReserveKey reservekey(pwalletMain); | ||
| pwalletMain->CreateTransaction(vRecipients, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, | ||
| &coinControl, true, 0, true, vTxouts); | ||
|
|
||
|
|
||
| ASSERT_SUCCESS(); | ||
| ASSERT_VIN_SIZE(1); | ||
| ASSERT_VIN_VALUE(0, 1001 * COIN); | ||
| ASSERT_VOUT_SIZE(2); | ||
| ASSERT_VOUT_ADDR_VALUE(0, 0, 999 * COIN); | ||
| ASSERT_VOUT_VALUE(1, 2 * COIN - nFeeRet); | ||
| ASSERT_HAS_KEY(1); | ||
|
|
||
| fMasternodeMode = fMasternodeModeTemp; | ||
| } | ||
|
|
||
| vRecipients = GetFakeRecipients({2000 * COIN}); | ||
|
|
||
| { | ||
| CCoinControl coinControl; | ||
| CWalletTx wtx; | ||
| CAmount nFeeRet = 0; | ||
| int nChangePosInOut = -1; | ||
| std::string strFailReason; | ||
|
|
||
| coinControl.nCoinType = CoinType::WITH_1000; | ||
|
|
||
| CReserveKey reservekey(pwalletMain); | ||
| pwalletMain->CreateTransaction(vRecipients, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, | ||
| &coinControl, true, 0, true, vTxouts); | ||
|
|
||
|
|
||
| ASSERT_SUCCESS(); | ||
| ASSERT_VIN_SIZE(2); | ||
| ASSERT_VIN_VALUE(0, 1001 * COIN); | ||
| ASSERT_VIN_VALUE(1, 1000 * COIN); | ||
| ASSERT_VOUT_SIZE(2); | ||
| ASSERT_VOUT_ADDR_VALUE(0, 0, 2000 * COIN); | ||
| ASSERT_VOUT_VALUE(1, 1 * COIN - nFeeRet); | ||
| ASSERT_HAS_KEY(1); | ||
| } | ||
|
|
||
| { | ||
| CCoinControl coinControl; | ||
| CWalletTx wtx; | ||
| CAmount nFeeRet = 0; | ||
| int nChangePosInOut = -1; | ||
| std::string strFailReason; | ||
|
|
||
| coinControl.Select(vTxouts.at(1).GetOutpoint()); | ||
| coinControl.nCoinType = CoinType::WITH_1000; | ||
|
|
||
| CReserveKey reservekey(pwalletMain); | ||
| pwalletMain->CreateTransaction(vRecipients, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, | ||
| &coinControl, true, 0, true, vTxouts); | ||
|
|
||
| ASSERT_FAILURE("Insufficient funds"); | ||
| } | ||
|
|
||
| { | ||
| CCoinControl coinControl; | ||
| CWalletTx wtx; | ||
| CAmount nFeeRet = 0; | ||
| int nChangePosInOut = -1; | ||
| std::string strFailReason; | ||
|
|
||
| coinControl.Select(vTxouts.at(1).GetOutpoint()); | ||
| coinControl.fAllowOtherInputs = true; | ||
|
|
||
| CReserveKey reservekey(pwalletMain); | ||
| pwalletMain->CreateTransaction(vRecipients, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, | ||
| &coinControl, true, 0, true, vTxouts); | ||
|
|
||
| ASSERT_FAILURE("Some coin control inputs could not be selected."); | ||
| } |
There was a problem hiding this comment.
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.
| BOOST_AUTO_TEST_CASE(coincontrol_transaction_size_limit) { | ||
| ACQUIRE_LOCKS(); | ||
|
|
||
| std::vector<CTransparentTxout> vTxouts = GetFakeTransparentTxouts({1 << 17, 1 << 15, 1 << 14, 1 << 13}); | ||
| std::vector<CRecipient> vRecipients = GetFakeRecipients({(1 << 17) + (1 << 14)}); | ||
|
|
||
| size_t n3TxSize = 0; | ||
| { | ||
| CCoinControl coinControl; | ||
| CWalletTx wtx; | ||
| CAmount nFeeRet = 0; | ||
| int nChangePosInOut = -1; | ||
| std::string strFailReason; | ||
|
|
||
| coinControl.nFeeRate = CFeeRate(1000); | ||
| coinControl.nMaxInputs = 3; | ||
|
|
||
| CReserveKey reservekey(pwalletMain); | ||
| pwalletMain->CreateTransaction(vRecipients, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, | ||
| &coinControl, true, 0, true, vTxouts); | ||
|
|
||
|
|
||
| ASSERT_SUCCESS(); | ||
| ASSERT_VIN_SIZE(3); | ||
| ASSERT_VIN_VALUE(0, 1 << 17); | ||
| ASSERT_VIN_VALUE(1, 1 << 13); | ||
| ASSERT_VIN_VALUE(2, 1 << 15); | ||
| ASSERT_VOUT_SIZE(2); | ||
| ASSERT_VOUT_ADDR_VALUE(0, 0, (1 << 17) + (1 << 14)); | ||
| ASSERT_VOUT_VALUE(1, (1 << 15) + (1 << 13) - (1 << 14) - nFeeRet); | ||
| ASSERT_HAS_KEY(1); | ||
|
|
||
| // nFeeRet is based on the transaction size with maximum-length signatures, which is what is used as the | ||
| // transaction size when calculating the transaction size for input selection. It may be slightly greater | ||
| // than the final transaction size. | ||
| BOOST_ASSERT(nFeeRet >= ::GetSerializeSize(*wtx.tx, SER_NETWORK, PROTOCOL_VERSION)); | ||
| n3TxSize = nFeeRet; | ||
| } | ||
|
|
||
| size_t n2TxSize = 0; | ||
| { | ||
| CCoinControl coinControl; | ||
| CWalletTx wtx; | ||
| CAmount nFeeRet = 0; | ||
| int nChangePosInOut = -1; | ||
| std::string strFailReason; | ||
|
|
||
| coinControl.nFeeRate = CFeeRate(1000); | ||
| coinControl.nMaxSize = n3TxSize - 1; | ||
|
|
||
| CReserveKey reservekey(pwalletMain); | ||
| pwalletMain->CreateTransaction(vRecipients, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, | ||
| &coinControl, true, 0, true, vTxouts); | ||
|
|
||
|
|
||
| ASSERT_SUCCESS(); | ||
| ASSERT_VIN_SIZE(2); | ||
| ASSERT_VIN_VALUE(0, 1 << 17); | ||
| ASSERT_VIN_VALUE(1, 1 << 15); | ||
| ASSERT_VOUT_SIZE(2); | ||
| ASSERT_VOUT_ADDR_VALUE(0, 0, (1 << 17) + (1 << 14)); | ||
| ASSERT_VOUT_VALUE(1, (1 << 15) - (1 << 14) - nFeeRet); | ||
| ASSERT_HAS_KEY(1); | ||
|
|
||
| BOOST_ASSERT(nFeeRet >= ::GetSerializeSize(*wtx.tx, SER_NETWORK, PROTOCOL_VERSION)); | ||
| n2TxSize = nFeeRet; | ||
| } | ||
|
|
||
| { | ||
| CCoinControl coinControl; | ||
| CWalletTx wtx; | ||
| CAmount nFeeRet = 0; | ||
| int nChangePosInOut = -1; | ||
| std::string strFailReason; | ||
|
|
||
| coinControl.nFeeRate = CFeeRate(1000); | ||
| coinControl.nMaxSize = n2TxSize - 1; | ||
|
|
||
| CReserveKey reservekey(pwalletMain); | ||
| pwalletMain->CreateTransaction(vRecipients, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, | ||
| &coinControl, true, 0, true, vTxouts); | ||
|
|
||
| ASSERT_FAILURE("Insufficient funds"); | ||
| } |
There was a problem hiding this comment.
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.
| BOOST_AUTO_TEST_CASE(coincontrol_minimum_total_fee) { | ||
| ACQUIRE_LOCKS(); | ||
|
|
||
| std::vector<CTransparentTxout> vTxouts = GetFakeTransparentTxouts({1 << 17, 1 << 15, 1 << 14, 1 << 13}); | ||
| std::vector<CRecipient> vRecipients = GetFakeRecipients({1 << 17}); | ||
|
|
||
| { | ||
| CCoinControl coinControl; | ||
| CWalletTx wtx; | ||
| CAmount nFeeRet = 0; | ||
| int nChangePosInOut = -1; | ||
| std::string strFailReason; | ||
|
|
||
| coinControl.nMinimumTotalFee = 1 << 13; | ||
|
|
||
| CReserveKey reservekey(pwalletMain); | ||
| pwalletMain->CreateTransaction(vRecipients, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, | ||
| &coinControl, true, 0, true, vTxouts); | ||
|
|
||
| ASSERT_SUCCESS(); | ||
| BOOST_ASSERT(nFeeRet == 1 << 13); | ||
| ASSERT_VIN_SIZE(2); | ||
| ASSERT_VIN_VALUE(0, 1 << 17); | ||
| ASSERT_VIN_VALUE(1, 1 << 13); | ||
| ASSERT_VOUT_SIZE(1); | ||
| ASSERT_VOUT_ADDR_VALUE(0, 0, 1 << 17); | ||
| } | ||
|
|
||
| { | ||
| CCoinControl coinControl; | ||
| CWalletTx wtx; | ||
| CAmount nFeeRet = 0; | ||
| int nChangePosInOut = -1; | ||
| std::string strFailReason; | ||
|
|
||
| coinControl.nMinimumTotalFee = 1 << 20; | ||
|
|
||
| CReserveKey reservekey(pwalletMain); | ||
| pwalletMain->CreateTransaction(vRecipients, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, | ||
| &coinControl, true, 0, true, vTxouts); | ||
|
|
||
| ASSERT_FAILURE("Insufficient funds"); | ||
| } | ||
|
|
||
| { | ||
| CCoinControl coinControl; | ||
| CWalletTx wtx; | ||
| CAmount nFeeRet = 0; | ||
| int nChangePosInOut = -1; | ||
| std::string strFailReason; | ||
|
|
||
| coinControl.nMinimumTotalFee = 100; | ||
|
|
||
| CReserveKey reservekey(pwalletMain); | ||
| pwalletMain->CreateTransaction(vRecipients, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, | ||
| &coinControl, true, 0, true, vTxouts); | ||
|
|
||
| ASSERT_SUCCESS(); | ||
| BOOST_ASSERT(nFeeRet > 100); | ||
| ASSERT_VIN_SIZE(2); | ||
| ASSERT_VIN_VALUE(0, 1 << 17); | ||
| ASSERT_VIN_VALUE(1, 1 << 13); | ||
| ASSERT_VOUT_SIZE(2); | ||
| ASSERT_VOUT_ADDR_VALUE(0, 0, 1 << 17); | ||
| ASSERT_VOUT_VALUE(1, (1 << 13) - nFeeRet); | ||
| ASSERT_HAS_KEY(1); | ||
| } |
There was a problem hiding this comment.
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.
| BOOST_AUTO_TEST_CASE(coincontrol_confirm_target) { | ||
| ACQUIRE_LOCKS(); | ||
|
|
||
| std::vector<CTransparentTxout> vTxouts = GetFakeTransparentTxouts({1 << 17, 1 << 15, 1 << 14, 1 << 13}); | ||
| std::vector<CRecipient> vRecipients = GetFakeRecipients({1 << 16}); | ||
|
|
||
| { | ||
| CCoinControl coinControl; | ||
| CWalletTx wtx; | ||
| CAmount nFeeRet = 0; | ||
| int nChangePosInOut = -1; | ||
| std::string strFailReason; | ||
|
|
||
| coinControl.nConfirmTarget = 5; | ||
|
|
||
| CReserveKey reservekey(pwalletMain); | ||
| pwalletMain->CreateTransaction(vRecipients, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, | ||
| &coinControl, true, 0, true, vTxouts); | ||
|
|
||
| ASSERT_FAILURE("Insufficient funds"); | ||
| } | ||
|
|
||
| vTxouts.at(0)._mockupDepthInMainChain = 5; | ||
|
|
||
| { | ||
| CCoinControl coinControl; | ||
| CWalletTx wtx; | ||
| CAmount nFeeRet = 0; | ||
| int nChangePosInOut = -1; | ||
| std::string strFailReason; | ||
|
|
||
| coinControl.nConfirmTarget = 5; | ||
|
|
||
| CReserveKey reservekey(pwalletMain); | ||
| pwalletMain->CreateTransaction(vRecipients, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, | ||
| &coinControl, true, 0, true, vTxouts); | ||
|
|
||
| ASSERT_SUCCESS(); | ||
| ASSERT_VIN_SIZE(1); | ||
| ASSERT_VIN_VALUE(0, 1 << 17); | ||
| ASSERT_VOUT_SIZE(2); | ||
| ASSERT_VOUT_ADDR_VALUE(0, 0, 1 << 16); | ||
| ASSERT_VOUT_VALUE(1, (1 << 17) - (1 << 16) - nFeeRet); | ||
| ASSERT_HAS_KEY(1); | ||
| } |
There was a problem hiding this comment.
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.
| BOOST_AUTO_TEST_CASE(multisig) { | ||
| ACQUIRE_LOCKS(); | ||
|
|
||
| std::vector<CRecipient> vRecipients = GetFakeRecipients({CENT}); | ||
|
|
||
| for (size_t n = 1; n < 5; n++) { | ||
| for (size_t m = n; m <= 5; m++) { | ||
| for (size_t nOurs = 0; nOurs <= m; nOurs++) { | ||
| std::vector<CTransparentTxout> vTxouts = | ||
| {GetFakeTransparentTxout(GetRandomMultisigAddress(nOurs, n, m), COIN, true)}; | ||
|
|
||
| CCoinControl coinControl; | ||
| CWalletTx wtx; | ||
| CAmount nFeeRet = 0; | ||
| int nChangePosInOut = -1; | ||
| std::string strFailReason; | ||
|
|
||
| CReserveKey reservekey(pwalletMain); | ||
| pwalletMain->CreateTransaction(vRecipients, wtx, reservekey, nFeeRet, nChangePosInOut, | ||
| strFailReason, &coinControl, true, 0, true, vTxouts); | ||
|
|
||
| if (nOurs >= n) { | ||
| ASSERT_SUCCESS(); | ||
| ASSERT_VIN_SIZE(1); | ||
| ASSERT_VIN_VALUE(0, COIN); | ||
| ASSERT_VOUT_SIZE(2); | ||
| ASSERT_VOUT_ADDR_VALUE(0, 0, CENT); | ||
| ASSERT_VOUT_VALUE(1, COIN - CENT - nFeeRet); | ||
| ASSERT_HAS_KEY(1); | ||
| } else { | ||
| ASSERT_FAILURE("Signing transaction failed"); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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 forcsanddb.Moving
CCriticalSection csandCInstantSendDb dbfrom private to public sections inCInstantSendManagerintroduces 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 functionGetAddressis well-implemented.The
GetAddressfunction correctly converts aCTxDestinationto a string representation of a Bitcoin address.
20-24: Utility functionGetAddressforCScriptis well-implemented.The
GetAddressfunction correctly extracts aCTxDestinationfrom aCScriptand converts it to a string representation of a Bitcoin address.
26-37: Encapsulate test-specific logic inGetFakeTransparentTxout.The function
GetFakeTransparentTxoutcreates a mock transparent output for testing. It directly modifies internal fields_mockupIsMineand_mockupDepthInMainChainofCTransparentTxout, 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 ofGetRandomWalletAddress.
GetRandomWalletAddressassumes thatpwalletMainis 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 ofGetRandomMultisigAddress.
GetRandomMultisigAddresscorrectly 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 toGetRandomWalletAddress, it relies on a globalpwalletMain, which could be refactored for better test isolation.Consider refactoring to accept a wallet parameter for improved modularity.
100-108: Enhance assertions inAssertVinValuewith descriptive error messages.The function
AssertVinValuechecks if a specific input in a transaction matches an expected value. It iterates over a vector ofCTransparentTxoutto find a matching outpoint. This is a clear and effective way to assert conditions in test cases. However, consider adding error messages to theBOOST_ASSERTcalls to provide more context when assertions fail.Enhance assertions with descriptive error messages for better debugging.
140-164: Add checks for specific error messages insends_moneytest case.The test case
sends_moneyverifies 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 inmultiple_recipientstest case.The test case
multiple_recipientscorrectly 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 inmints_changetest case.The test case
mints_changefocuses 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 ininstantsendtest case.The test case
instantsendfocuses 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 inchange_positiontest case.The test case
change_positionverifies 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 inwatch_only_addresstest case.The test case
watch_only_addresstests 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 incoincontrol_coin_typetest case.The test case
coincontrol_coin_typetests 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 incoincontrol_transaction_size_limittest case.The test case
coincontrol_transaction_size_limittests 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 incoincontrol_minimum_total_feetest case.The test case
coincontrol_minimum_total_feetests 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 incoincontrol_confirm_targettest case.The test case
coincontrol_confirm_targettests 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 inmultisigtest case.The test case
multisigtests 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.his appropriate for the new functionality.
88-90: LGTM!The
IsTransparentTxoutmethod correctly identifies transparent transaction outputs.
92-94: LGTM!The
GetHashmethod correctly returns the hash of the outpoint.
96-98: LGTM!The
GetOutpointmethod correctly returns the outpoint.
100-103: LGTM!The
GetValuemethod correctly returns the value of the transaction output and includes an appropriate assertion.
105-107: LGTM!The
GetScriptPubkeymethod correctly returns the scriptPubKey of the transaction output.
109-154: Consider documenting the rationale behind the specific sizes.The
GetMarginalSpendSizemethod 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
IsMinemethod correctly checks if the transaction output belongs to the wallet and handles both mockup and real scenarios.
174-192: LGTM!The
IsSpendablemethod correctly checks if the transaction output is spendable and handles both mockup and real scenarios.
194-202: LGTM!The
IsLockedmethod correctly checks if the transaction output is locked and handles both mockup and real scenarios.
204-212: LGTM!The
IsAbandonedmethod correctly checks if the transaction output is abandoned and handles both mockup and real scenarios.
214-231: LGTM!The
IsCoinTypeCompatiblemethod correctly checks if the transaction output is compatible with the specified coin type and includes various conditions to ensure compatibility.
233-241: LGTM!The
IsLLMQInstantSendLockedmethod correctly checks if the transaction output is locked by LLMQ InstantSend and handles both mockup and real scenarios.
243-251: LGTM!The
IsCoinBasemethod correctly checks if the transaction output is a coinbase transaction and handles both mockup and real scenarios.
253-270: LGTM!The
IsFromMemethod correctly checks if the transaction output is from the wallet and handles both mockup and real scenarios.
272-280: LGTM!The
GetDepthInMainChainmethod correctly returns the depth of the transaction output in the main chain and handles both mockup and real scenarios.
282-295: LGTM!The
GetDepthInMempoolmethod 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
GetFeemethod correctly calculates the fee for a transaction based on the coin control settings and transaction size.
4556-4570: LGTM!The
GetTransparentTxoutsmethod correctly retrieves all transparent transaction outputs from the wallet and ensures proper locking.
4572-4615: LGTM!The
SignTransparentInputsmethod correctly signs the transparent inputs of a transaction, with proper handling of signature creation and validation, and appropriate error handling.
4618-4694: LGTM!The
CheckTransparentTransactionSanitymethod correctly checks the sanity of a transparent transaction, including various checks for size, fee, and outputs.
4696-4866: LGTM!The
CreateTransactionmethod 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
GetAvailableInputsmethod 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
GetInputsForTxmethod 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
GetInputsForTxmethod forCTransparentTxoutis appropriate.
1dac305
to
fbea637
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (3)
src/wallet/txbuilder.h (1)
30-44: Update all references toTxBuildertoSigmaTxBuilderSuperclass.The class
TxBuilderhas been renamed toSigmaTxBuilderSuperclass, but some references toTxBuilderstill exist in the codebase. Ensure that all references are updated to maintain consistency.
src/wallet/test/sigmatxbuilder_tests.cpp: Update references toTestTxBuilderto reflect the new class nameSigmaTxBuilderSuperclass.The method
GetInputsis correctly usingSigmaTxBuilderInputSigner.Analysis chain
Verify the class renaming and method signature change.
The class
TxBuilderhas been renamed toSigmaTxBuilderSuperclass. Ensure that all references toTxBuilderhave been updated toSigmaTxBuilderSuperclass.The method
GetInputsnow accepts a vector ofstd::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
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_equalmay be undefined, or defined from star imports(F405)
101-101:
assert_equalmay be undefined, or defined from star imports(F405)
143-143:
assert_equalmay 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
GetInputsnow accepts a vector ofstd::unique_ptr<SigmaTxBuilderInputSigner>instead ofstd::unique_ptr<InputSigner>. Ensure that all calls to this method use the correct type.Verification successful
Verification successful. All calls to the
GetInputsmethod correctly usestd::unique_ptr<SigmaTxBuilderInputSigner>.
src/wallet/sigmaspendbuilder.cppsrc/wallet/test/sigmatxbuilder_tests.cppsrc/wallet/txbuilder.hsrc/wallet/txbuilder.cppsrc/wallet/sigmaspendbuilder.hScripts 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.hLength of output: 3498
10-10: Verify the change in inheritance.The class now inherits from
SigmaTxBuilderSuperclassinstead ofTxBuilder. Ensure thatSigmaTxBuilderSuperclassprovides the necessary functionality and that all inherited methods are correctly overridden.Verification successful
Verification complete: Inheritance and method signature change.
The class
SigmaSpendBuildercorrectly inherits fromSigmaTxBuilderSuperclass, and the methodGetInputshas the appropriate signature matching the superclass.
SigmaSpendBuilderinherits fromSigmaTxBuilderSuperclass.GetInputsmethod signature isCAmount 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
InputSignerhas been renamed toSigmaTxBuilderInputSigner. Ensure that all references toInputSignerhave been updated toSigmaTxBuilderInputSigner.Verification successful
Verify the class renaming.
The class
InputSignerhas been successfully renamed toSigmaTxBuilderInputSigneracross the codebase. All references toInputSignerhave been updated accordingly.
- Verified in the following files:
src/wallet/txbuilder.cppsrc/wallet/sigmaspendbuilder.hsrc/wallet/txbuilder.hsrc/wallet/test/sigmatxbuilder_tests.cppsrc/wallet/sigmaspendbuilder.cppScripts 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, andnMaxSizeenhance 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, andnMaxSizeare 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 insrc/wallet/wallet.cpp, tested insrc/wallet/test/createtransaction_tests.cpp.nMaxInputs: Initialized insrc/wallet/coincontrol.h, used insrc/wallet/wallet.cpp, tested insrc/wallet/test/createtransaction_tests.cpp.nMaxSize: Initialized insrc/wallet/coincontrol.h, used insrc/wallet/wallet.cpp, tested insrc/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 methodGetSelectedSize.The new method
GetSelectedSizeprovides additional functionality. Ensure that the method is correctly used throughout the codebase.Verification successful
Let's extract the context around the usage of
GetSelectedSizeinsrc/wallet/wallet.cppto verify its correct implementation.
The method
GetSelectedSizeis 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.cppLength 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
GetInputsare 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 toGetInputsare 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 toGetInputs.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
InputSignertoSigmaTxBuilderInputSigneris approved.Ensure that all references to
SigmaSpendSignerare compatible with the new base class.Verification successful
Verification successful.
All references to
SigmaSpendSignerare contained withinsrc/wallet/sigmaspendbuilder.cppand are compatible with the new base classSigmaTxBuilderInputSigner.
SigmaSpendSignerclass 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
TxBuildertoSigmaTxBuilderSuperclassis approved.Ensure that all references to
SigmaSpendBuilderare compatible with the new base class.Verification successful
Verified: The integration of
SigmaSpendBuilderwith the new base classSigmaTxBuilderSuperclassis correct and compatible with the existing logic.
- The class
SigmaSpendBuildercorrectly inherits fromSigmaTxBuilderSuperclass.- The methods
GetInputsandGetChangesinSigmaSpendBuilderproperly override methods fromSigmaTxBuilderSuperclass.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
tx2simplifies 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
tx2does 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_txare 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.pyLength 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.pyLength of output: 2326
99-99: Validate the correctness of the test logic.The removal of references to
tx2simplifies 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
tx2simplifies the test and focuses on the transaction from the "foo" account. The assertionassert_equal(self.nodes[1].getbalance("from0"), -(tx1["amount"]))correctly verifies the expected balance for the "from0" account onnode1.
- 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.pyLength 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.pyLength 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.pyLength of output: 9331
Tools
Ruff
99-99:
assert_equalmay be undefined, or defined from star imports(F405)
143-143: Validate the correctness of the test logic.The removal of references to
tx2simplifies 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
tx2simplifies 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 oftx1["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.pyLength 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.pyLength 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.pyLength of output: 4752
Tools
Ruff
143-143:
assert_equalmay 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
InputSignertoSigmaTxBuilderInputSigneris approved.Ensure that all references to
TestInputSignerare compatible with the new base class.Verification successful
Verification Successful!
The change in inheritance from
InputSignertoSigmaTxBuilderInputSignerfor theTestInputSignerclass 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
SigmaTxBuilderInputSignerpointers.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
TxBuildertoSigmaTxBuilderSuperclassis approved.Ensure that all references to
TestTxBuilderare 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
GetInputsare compatible with the new signature.Verification successful
Verify the compatibility of the new method signature for
GetInputs.The
GetInputsmethod signature has been updated to usestd::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.cppScripts 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
SigmaTxBuilderInputSignertype 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_TESTSvariable has been updated to includewallet/test/sigmatxbuilder_tests.cppandwallet/test/createtransaction_tests.cpp, and removewallet/test/txbuilder_tests.cpp. Ensure that the new tests provide adequate coverage and that the removal oftxbuilder_tests.cppdoes not negatively impact the testing suite.Verification successful
Verification successful.
The
BITCOIN_TESTSvariable has been correctly updated to includewallet/test/sigmatxbuilder_tests.cppandwallet/test/createtransaction_tests.cpp, and removewallet/test/txbuilder_tests.cpp. There are no remaining references totxbuilder_tests.cppin the repository, ensuring that the changes do not negatively impact the testing suite.
wallet/test/sigmatxbuilder_tests.cppexists.wallet/test/createtransaction_tests.cppexists.wallet/test/txbuilder_tests.cpphas 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 fLength 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
InputSignerhas been renamed toSigmaTxBuilderInputSigner, 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
TxBuilderhas been renamed toSigmaTxBuilderSuperclass, and its constructor and destructor have been updated accordingly. The changes are consistent and correctly implemented.
268-270: LGTM! Consistent renaming and updates.The
AdjustFeefunction has been updated to reflect the new class nameSigmaTxBuilderSuperclass. The changes are consistent and correctly implemented.
Line range hint
42-268:
Verify the logic in theBuildfunction.The
Buildfunction has been updated to useSigmaTxBuilderInputSigner. 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
TestingSetupconstructor 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 asconst.The class interface looks good. However, methods like
GetHash,GetOutpoint,GetValue,GetScriptPubkey, etc., should be marked asconstto 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 ofAbstractTxoutType.The enum
AbstractTxoutTypelooks 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() constlooks good.
1093-1093: LGTM!The method
SignTransparentInputslooks good.
1094-1095: LGTM!The method
CheckTransparentTransactionSanitylooks good.
1102-1105: LGTM!The method
CreateTransactionwith the old signature looks good.
709-709: LGTM!The typedef
TxSpendsmove 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
CreateTransactionmatch the new signature.src/wallet/wallet.cpp (27)
7-7: LGTM!The inclusion of
script/standard.happears necessary for the new functionality related to transparent transactions.
88-90: LGTM!The
IsTransparentTxoutmethod correctly identifies transparent outputs.
92-94: LGTM!The
GetHashmethod correctly returns the hash of the outpoint.
96-98: LGTM!The
GetOutpointmethod correctly returns the outpoint.
100-103: LGTM!The
GetValuemethod correctly returns the value of the transaction output.
105-107: LGTM!The
GetScriptPubkeymethod correctly returns the scriptPubKey of the transaction output.
156-172: LGTM!The
IsMinemethod correctly checks if the transparent output belongs to the wallet.
174-192: LGTM!The
IsSpendablemethod correctly checks if the transparent output is spendable.
194-202: LGTM!The
IsLockedmethod correctly checks if the transparent output is locked.
204-212: LGTM!The
IsAbandonedmethod correctly checks if the transparent output is abandoned.
214-231: LGTM!The
IsCoinTypeCompatiblemethod correctly checks if the transparent output is compatible with the specified coin type.
233-241: LGTM!The
IsLLMQInstantSendLockedmethod correctly checks if the transparent output is locked by LLMQ InstantSend.
243-251: LGTM!The
IsCoinBasemethod correctly checks if the transparent output is from a coinbase transaction.
253-270: LGTM!The
IsFromMemethod correctly checks if the transparent output is from the wallet.
272-280: LGTM!The
GetDepthInMainChainmethod correctly returns the depth of the transparent output in the main chain.
282-295: LGTM!The
GetDepthInMempoolmethod correctly returns the depth of the transparent output in the mempool.
88-154: LGTM!The changes to the
CreateTransactionmethod 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
GetFeemethod correctly calculates the fee for a transaction, considering various coin control parameters.
4556-4570: LGTM!The
GetTransparentTxoutsmethod correctly retrieves transparent transaction outputs from the wallet.
4572-4615: LGTM!The
SignTransparentInputsmethod correctly signs the transparent inputs of a transaction.
4618-4694: LGTM!The
CheckTransparentTransactionSanitymethod correctly checks the sanity of a transparent transaction.
4696-4866: LGTM!The
CreateTransactionmethod correctly creates a transaction, considering transparent transaction outputs.
4868-4932: LGTM!The
GetAvailableInputsmethod correctly retrieves available inputs for a transaction, considering various coin control parameters.
4942-5202: LGTM!The
GetInputsForTxmethod correctly retrieves inputs for a transaction, considering various coin control parameters and constraints.
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| 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.
| 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; | |
| } |
| size_t CTransparentTxout::GetMarginalSpendSize(std::vector<CTransparentTxout>& previousInputs) const { | ||
| assert(!txout.IsNull()); | ||
|
|
||
| txnouttype outType; | ||
| std::vector<std::vector<unsigned char>> vSolutions; | ||
| if (!Solver(txout.scriptPubKey, outType, vSolutions)) | ||
| return 0; | ||
|
|
||
| // This is the size of scriptPubKey for the input. | ||
| size_t sigDataSize = 0; | ||
| switch (outType) { | ||
| case TX_MULTISIG: | ||
| sigDataSize = 1 + 73 * vSolutions.at(0).at(0); | ||
| break; | ||
|
|
||
| case TX_PUBKEY: | ||
| sigDataSize = 101; | ||
| break; | ||
|
|
||
| case TX_SCRIPTHASH: | ||
| case TX_PUBKEYHASH: | ||
| case TX_EXCHANGEADDRESS: | ||
| sigDataSize = 107; | ||
| break; | ||
|
|
||
| case TX_NONSTANDARD: | ||
| case TX_NULL_DATA: | ||
| case TX_ZEROCOINMINT: | ||
| case TX_ZEROCOINMINTV3: | ||
| case TX_LELANTUSMINT: | ||
| case TX_LELANTUSJMINT: | ||
| case TX_WITNESS_V0_KEYHASH: | ||
| case TX_WITNESS_V0_SCRIPTHASH: | ||
| default: | ||
| throw std::runtime_error("Unsupported outType"); | ||
| } | ||
|
|
||
| return | ||
| GetSizeOfCompactSize(previousInputs.size() + 1) - | ||
| GetSizeOfCompactSize(previousInputs.size()) + | ||
| 32 + // txid | ||
| 4 + // vout | ||
| GetSizeOfCompactSize(sigDataSize) + | ||
| sigDataSize + | ||
| 4; // sequence | ||
| } |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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
LOCK2statement enhances thread safety by ensuring that bothcs_mainandpwallet->cs_walletare 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.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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_collateralmay be undefined, or defined from star imports(F405)
100-100:
lock_mn_collateralmay be undefined, or defined from star imports(F405)
196-196:
unlock_mn_collateralmay 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 oflockunspent.The
lock_mn_collateralfunction locks the collateral usinglockunspent(False, ...). This is correct for locking, but ensure that the collateral is unlocked elsewhere when needed.
52-54: Ensure correct usage oflockunspent.The
unlock_mn_collateralfunction unlocks the collateral usinglockunspent(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 oflock_mn_collateral.The function
lock_mn_collateralis used here. Ensure it is correctly imported frommn_utils.py. The static analysis tool flagged a potential issue due to star imports.Tools
Ruff
100-100:
lock_mn_collateralmay be undefined, or defined from star imports(F405)
64-64: Verify the import oflock_mn_collateral.The function
lock_mn_collateralis used here. Ensure it is correctly imported frommn_utils.py. The static analysis tool flagged a potential issue due to star imports.Verification successful
The import of
lock_mn_collateralis correct.The function
lock_mn_collateralis defined inmn_utils.pyand is imported indip3-deterministicmns.pyvia 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.pyLength 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 2Length of output: 720
Tools
Ruff
64-64:
lock_mn_collateralmay be undefined, or defined from star imports(F405)
196-196: Verify the import ofunlock_mn_collateral.The function
unlock_mn_collateralis used here. Ensure it is correctly imported frommn_utils.py. The static analysis tool flagged a potential issue due to star imports.Verification successful
unlock_mn_collateralis correctly imported via star import. The functionunlock_mn_collateralis defined inmn_utils.pyand is imported intodip3-deterministicmns.pyusing 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.pyLength 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 3Length 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.pyLength of output: 384
Tools
Ruff
196-196:
unlock_mn_collateralmay be undefined, or defined from star imports(F405)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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_outputmay be undefined, or defined from star imports(F405)
44-44:
find_outputmay 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 offind_output.The
find_outputfunction 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_outputmay be undefined, or defined from star imports(F405)
44-44: LGTM! But verify thefind_outputusage.The logic for unlocking the transaction output is correct and consistent with the test's objectives.
However, ensure that
find_outputis correctly defined or imported to avoid runtime errors.Verification successful
find_outputis correctly defined and accessible.The function
find_outputis defined inqa/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_outputmay be undefined, or defined from star imports(F405)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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
CTxDestinationto a string representation of a Bitcoin address.
20-24: LGTM!The function correctly extracts a
CTxDestinationfrom aCScriptand 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.
This PR refactors public transaction creation, which is a major change requiring careful review.