Refactor Public Transaction Creation #1348
Open
+2,219
−394
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
force-pushed
the
createtransaction-public
branch
from
be2ea19
to
dd93e7e
Compare
ghost
force-pushed
the
createtransaction-public
branch
from
dd93e7e
to
4e79d51
Compare
ghost
force-pushed
the
createtransaction-public
branch
from
7a76600
to
a149ca0
Compare
psolstice
force-pushed
the
createtransaction-public
branch
from
da2fa2e
to
54529b8
Compare
psolstice
requested changes
Feb 3, 2024
| @@ -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 []
| 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 a series of enhancements and refactorings across the system's components, focusing on transaction handling, script validation, and wallet functionality. Key modifications include the introduction of new test files, adjustments in class accessibility and inheritance, the addition of a method to identify pay-to-public-key scripts, and improvements in coin control features. These changes aim to streamline transaction creation and signing processes while bolstering the system's overall 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 Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
Comment on lines
+113
to
+114
| 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.
Comment on lines
+34
to
+38
| 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.
Suggested change
| 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.
Suggested change
| 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()); |
Comment on lines
28
to
40
| #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.
Comment on lines
+651
to
+695
| 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.
Comment on lines
+888
to
+971
| 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.
Comment on lines
+1003
to
+1069
| 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.
Comment on lines
+1072
to
+1116
| 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.
Comment on lines
+1175
to
+1206
| 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.
Comment on lines
+109
to
+154
| 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.
The method GetMarginalSpendSize calculates the additional size a given input will add to a transaction. It handles various output types and throws an exception for unsupported types. Consider documenting the rationale behind the specific sizes assigned to different transaction types for maintainability.
// It would be beneficial to add comments explaining the basis for the specific sizes used in the switch-case structure.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors public transaction creation, which is a major change requiring careful review.
Summary by CodeRabbit
CInstantSendManagerto the public section to address duplicate declarations.