-
-
Notifications
You must be signed in to change notification settings - Fork 355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor Public Transaction Creation #1348
base: master
Are you sure you want to change the base?
Conversation
be2ea19
to
dd93e7e
Compare
dd93e7e
to
4e79d51
Compare
7a76600
to
a149ca0
Compare
da2fa2e
to
54529b8
Compare
@@ -171,7 +171,7 @@ class CTxOut | |||
public: | |||
CAmount nValue; | |||
CScript scriptPubKey; | |||
int nRounds; | |||
int nRounds = -10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why -10 specifically?
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
@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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
TxBuilder
toSigmaTxBuilderSuperclass
aligns with the refactor's goal to specialize the transaction building process for Sigma transactions. This change enhances clarity and specificity in the codebase.- 22-22: Updating the type of the input parameter in the
GetInputs
method fromInputSigner
toSigmaTxBuilderInputSigner
is a logical step following the renaming ofInputSigner
toSigmaTxBuilderInputSigner
. This ensures type consistency and leverages the specialized functionality of the newSigmaTxBuilderInputSigner
class.src/wallet/txbuilder.h (2)
- 16-25: Renaming
InputSigner
toSigmaTxBuilderInputSigner
and updating its constructors enhances the clarity and specificity of the code, especially in the context of Sigma transactions. This change makes the purpose of the class more explicit and aligns with the overall refactor's goals.- 30-44: Renaming
TxBuilder
toSigmaTxBuilderSuperclass
and updating its constructor and method signatures are consistent with the refactor's aim to specialize the transaction building process for Sigma transactions. These changes improve code clarity and ensure that the class's functionality is explicitly tailored to Sigma transactions.src/wallet/coincontrol.h (2)
- 21-22: Adding
WITH_MINTS
andWITH_1000
to theCoinType
enum introduces more granular control over the types of coins that can be selected for transactions. This enhancement supports the refactor's goal of providing more flexibility and specificity in transaction creation and management.- 40-51: Introducing
fAllowUnconfirmed
,nMaxInputs
, andnMaxSize
to theCCoinControl
class provides users with more control over transaction creation, particularly in terms of allowing unconfirmed inputs and setting limits on the number of inputs and the maximum transaction size. These additions align with the refactor's objectives to enhance coin control features.src/wallet/sigmaspendbuilder.cpp (3)
- 21-21: Renaming the class
SigmaSpendSigner
toSigmaTxBuilderInputSigner
and adjusting its inheritance to reflect the changes made in the header files is a necessary step to maintain consistency across the codebase. This change aligns with the refactor's goal of specializing the transaction building process for Sigma transactions.- 111-111: The change in inheritance for the
SigmaSpendBuilder
class fromTxBuilder
toSigmaTxBuilderSuperclass
is consistent with the refactor's aim to specialize the transaction building process. This adjustment ensures thatSigmaSpendBuilder
leverages the specialized functionality provided bySigmaTxBuilderSuperclass
.- 132-132: Updating the
GetInputs
method signature inSigmaSpendBuilder
to useSigmaTxBuilderInputSigner
instead of the previousInputSigner
type is a logical continuation of the renaming and specialization efforts. This ensures that the method correctly utilizes the specialized input signer for Sigma transactions.src/sync.cpp (3)
- 21-21: Including the
<set>
header is necessary for the use of thestd::set
container in theInvLockOrders
declaration. This inclusion ensures that the necessary data structures are available for managing lock orders, which is crucial for detecting potential deadlocks.- 70-70: Modifying the declaration of
InvLockOrders
to use a different syntax for the set declaration (std::set<std::pair<void*, void*>>
) is a minor change that does not affect functionality. However, it's important to ensure that such changes are consistent across the codebase and that they follow the project's coding standards.- 177-177: Correcting the minor formatting issue in the
AssertLockNotHeldInternal
function improves code readability. While this change is minor, maintaining consistent formatting is important for code maintainability and readability.qa/rpc-tests/txn_clone.py (2)
- 99-99: Adjusting balance calculations and confirmations handling for transactions
tx1
andtx2
is crucial for accurately reflecting the state of the blockchain and the wallet balances after the transactions have been processed. These changes are necessary to ensure that the test script accurately tests the intended scenarios, especially in the context of transaction malleability and cloning.- 105-105: Sending
fund_bar_tx
to the miner and handling the clone oftx1
separately are important steps in setting up the test scenario. These actions ensure that the blockchain state is correctly prepared for testing the handling of transaction clones and their impact on wallet balances and confirmations.src/Makefile.test.include (1)
- 211-212: Adding
sigmatxbuilder_tests.cpp
andcreatetransaction_tests.cpp
while removingtxbuilder_tests.cpp
is a positive change, reflecting the refactoring and renaming in the codebase. Ensure that these new test files cover all the functionalities previously tested bytxbuilder_tests.cpp
and include additional tests for any new functionality introduced by the refactor.src/wallet/txbuilder.cpp (2)
- 22-30: Renaming
InputSigner
toSigmaTxBuilderInputSigner
and updating its constructors improves clarity and specificity, aligning the class name with its specialized purpose for Sigma transactions. This change enhances code readability and maintainability.- 42-43: The comment indicating legacy usage for
SigmaTxBuilderSuperclass::Build
is helpful for understanding the context in which this method is used. It's good practice to mark legacy code clearly, but also consider if there's a plan to refactor or replace this legacy code to align with the new architecture.src/test/test_bitcoin.cpp (1)
- 112-112: The addition of
pwalletMain->GenerateNewMnemonic();
before generating a new HD master key is a positive change, enhancing the realism of the wallet setup process in tests. This aligns more closely with actual wallet initialization scenarios, potentially improving test coverage and relevance.However, please ensure that this change does not adversely affect existing tests or introduce new assumptions that could impact test reliability.
src/primitives/transaction.h (3)
- 174-174: The change of the
nRounds
default value from0
to-10
in theCTxOut
class:
- Ensure that this change does not affect the compatibility with existing transactions, especially those that might rely on the default value of
nRounds
for certain logic or calculations.- Clarify the rationale behind choosing
-10
as the new default value. It's important to understand the significance of this specific value and how it impacts the transaction creation or processing logic.- Consider the potential impact on transaction privacy or mixing features. Since
nRounds
could be related to transaction mixing or privacy enhancements, changing its default value might have implications on these aspects.
- 171-177: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Ensure the copyright notice at the top of the file is up-to-date and accurately reflects the copyright years and holders.
- 174-174: The introduction of a non-standard default value (
-10
) fornRounds
inCTxOut
:
- Verify that this change is well-documented within the project's documentation or code comments. It's crucial for maintainers and developers to understand the purpose and implications of this default value.
- Assess the impact on external systems or libraries that interact with this codebase. Ensure that this change does not introduce unexpected behavior in systems that rely on the
CTxOut
structure.src/script/script.h (1)
- 665-665: The addition of the
IsPayToPublicKey()
method to theCScript
class is noted. However, the implementation details of this method are not provided in the snippet. Assuming the implementation is correct and efficient, this method could enhance the script validation capabilities by allowing easy identification of scripts that pay directly to a public key, which aligns with the PR's objectives of refining transaction creation and handling.Ensure that the method's implementation follows best practices for performance, especially considering the potential frequency of its invocation in transaction validation processes. Additionally, thorough testing should be conducted to verify its correctness in various scenarios, including edge cases.
src/wallet/test/createtransaction_tests.cpp (18)
- 14-18: Utility function
GetAddress
correctly converts aCTxDestination
to a Bitcoin address string usingCBitcoinAddress
. This is a straightforward and commonly used pattern in Bitcoin-related code.- 39-43: The function
GetRandomDest
generates a random script destination. It usesCKey::MakeNewKey
to create a new private key and then derives the public key destination. This is a standard method for generating random addresses in tests.- 77-82:
GetFakeTransparentTxout
is an overloaded version that simplifies creating a fake transparent output by deciding whether the output is "mine" based on theisMine
parameter. It delegates to the more specificGetFakeTransparentTxout
function with either a random wallet address or a random destination. This is a good use of function overloading to provide a simpler interface for common use cases.- 84-90: The function
GetFakeTransparentTxouts
generates a vector of fake transparent outputs for given values. It demonstrates good use of modern C++ features like range-based for loops andemplace_back
for efficient vector operations.- 92-98:
GetFakeRecipients
creates a vector of recipients for given values, usingCRecipient
structs. This function is straightforward and utilizes efficient vector operations similar toGetFakeTransparentTxouts
.- 110-112:
AssertVoutAddr
correctly asserts that a transaction's output script matches the expected recipient's script. This is a concise and effective way to validate transaction outputs in tests.- 114-115:
AssertVoutValue
is a simple assertion function that checks if a transaction output's value matches the expected value. It usesBOOST_ASSERT
effectively.- 118-123:
AssertHasKey
ensures that a specific output of a transaction is recognized as "mine" by the wallet. It correctly locks the wallet and keeps the reserve key. This function is essential for tests that involve transaction signing and ownership checks.- 126-135: The macros defined for assertions (
ASSERT_VIN_VALUE
,ASSERT_VOUT_ADDR
, etc.) provide a concise way to perform common assertions in the test cases. They improve readability and maintainability of the test code by abstracting repetitive assertion patterns.- 140-141: The test suite
createtransaction_tests
is correctly set up usingBOOST_FIXTURE_TEST_SUITE
withWalletTestingSetup
as the fixture. This setup ensures that each test case has a clean environment with necessary wallet and blockchain components initialized.- 247-270: The test case
selects_smallest_input_required
verifies that the wallet selects the smallest necessary input to cover the transaction amount and fees. This is an important test for ensuring efficient use of inputs. The test is clear and effectively uses utility functions and assertion macros. It's well-constructed and covers the intended scenario adequately.- 273-311: The test case
insufficient_funds
checks the wallet's behavior when there are insufficient funds to cover the transaction amount and fees. It tests both a scenario where the funds are indeed insufficient and a scenario where adding an additional input makes the transaction possible. This test case is crucial for ensuring robust error handling in the wallet. It's well-implemented and makes good use of utility functions and assertion macros.- 314-358: The test case
no_unconfirmed_inputs
tests the wallet's behavior when configured not to use unconfirmed inputs for transactions. It verifies that transactions fail when only unconfirmed inputs are available and succeed when confirmed inputs are used. This test case is important for ensuring that coin control settings are respected. It's well-constructed and effectively uses utility functions and assertion macros.- 361-387: The test case
takes_from_front_and_back
verifies the wallet's input selection strategy when it needs to take inputs from both the beginning and the end of the available inputs to cover the transaction amount. This scenario tests the wallet's ability to efficiently select inputs. The test is well-structured and makes effective use of utility functions and assertion macros.- 390-418: The test case
doesnt_select_used_inputs
ensures that the wallet does not select inputs that are already marked as spent. This is crucial for preventing double-spending attempts and ensuring transaction validity. The test is clear, concise, and effectively uses utility functions and assertion macros to validate the expected behavior.- 838-885: The test case
coincontrol_input_count_limit
verifies the wallet's behavior when a maximum input count is specified via coin control settings. It tests scenarios where the input count is sufficient and where it is not. This test case is crucial for ensuring that input count limits are respected during transaction creation. It's well-structured and effectively uses utility functions and assertion macros.- 974-1000: The test case
coincontrol_destination_address
verifies the wallet's ability to use a specific change address specified via coin control settings. It tests the scenario where a custom change destination is provided and ensures that the change is sent to the correct address. This test case is crucial for ensuring that coin control settings related to change addresses are respected. It's well-structured and effectively uses utility functions and assertion macros.- 1119-1172: The test case
coincontrol_require_all_inputs
tests the wallet's behavior when the requirement to use all selected inputs is specified via coin control settings. It covers scenarios where all inputs are required and where they are not. This test case is crucial for ensuring that the wallet correctly interprets and applies coin control settings related to input selection. It's well-constructed and effectively uses utility functions and assertion macros.src/wallet/wallet.h (2)
- 723-755: The methods
AddToSpends
,GetFee
,GetTransparentTxouts
, and templated methodsGetAvailableInputs
andGetInputsForTx
introduce significant new logic for handling transactions. Ensure that:
- The logic is correctly implemented and aligns with the intended functionality.
- Error handling is robust, especially for external calls and interactions.
- Performance implications are considered, particularly for loops and data structure access patterns.
- Unit tests are added or updated to cover these new functionalities.
- 1093-1105: The methods
SignTransparentInputs
,CheckTransparentTransactionSanity
, andCreateTransaction
have been updated to support transparent transactions. Ensure that:
- The changes do not introduce any security vulnerabilities, especially in the signing process.
- The transaction sanity checks are comprehensive and prevent invalid transactions from being created.
- The modifications to
CreateTransaction
do not adversely affect existing functionalities and are compatible with the rest of the wallet's operations.src/wallet/wallet.cpp (13)
- 7-7: The inclusion of new headers such as
script/standard.h
,walletexcept.h
,sigmaspendbuilder.h
, andlelantusjoinsplitbuilder.h
suggests enhancements and new functionalities related to transaction creation and handling. Ensure that these headers are necessary and correctly utilized in the wallet's refactored code.- 88-90: The method
IsTransparentTxout
correctly identifies if a transaction output is transparent based on its scriptPubKey. This is crucial for distinguishing between different types of transactions in the wallet.- 156-172: The method
IsMine
checks if a transaction output belongs to the wallet, considering both spendable and watch-only outputs. The use ofAssertLockHeld
ensures thread safety. Ensure that the locking conventions are consistently applied throughout the wallet code to prevent data races.- 214-231: The method
IsCoinTypeCompatible
checks for compatibility based on coin type, which is essential for filtering inputs in various transaction creation scenarios. The logic appears sound, but ensure that all coin types are correctly handled and that this method's behavior aligns with the intended use cases for coin control.- 233-242: The method
IsLLMQInstantSendLocked
checks for LLMQ InstantSend lock status. The locking mechanisms (AssertLockHeld
) used here are crucial for thread safety, especially given the potential complexity of querying InstantSend lock status. Ensure that the lock ordering is consistent with the rest of the application to avoid deadlocks.- 408-408: The modification to set the master key from the mnemonic container's seed is a critical part of securely handling wallet keys. Ensure that the seed data is correctly managed and that this change integrates well with the overall key management strategy.
- 470-470: Reserving space for the seed vector before setting the master key is a good practice to ensure memory efficiency. This change, along with the correct handling of the master key, contributes to the wallet's secure key management.
- 495-500: The logic for deriving child keys and updating the HD chain counters is correctly implemented, ensuring that keys already known to the wallet are skipped. This is crucial for maintaining the wallet's hierarchical deterministic structure and avoiding key reuse.
- 4536-4555: The method
GetFee
calculates the transaction fee based on the transaction size and coin control settings. The logic for overriding the fee rate and ensuring the fee does not fall below the required minimum is correctly implemented. Ensure that the fee calculation aligns with the network's fee policy and that the error handling for an unrelayable transaction due to a low fee is appropriate.- 4573-4617: The method
SignTransparentInputs
signs the transparent inputs of a transaction. It includes comprehensive error handling for various failure scenarios, such as non-standard inputs and signature issues. Ensure that the signing process is secure and that the error messages provide clear guidance for resolving potential issues.- 4619-4695: The method
CheckTransparentTransactionSanity
performs various checks to ensure the transaction's sanity, including size, input count, fee adequacy, and standardness of outputs. This comprehensive sanity checking is crucial for preventing the creation of invalid or problematic transactions. Ensure that all checks are aligned with the network's consensus rules and transaction policies.- 4709-4718: The
CreateTransaction
method's overload that does not takevTransparentTxouts
as an argument retrieves the transparent txouts and then calls the overloaded version that does. This separation of concerns allows for more flexible transaction creation workflows. Ensure that the retrieval of transparent txouts is efficient and that the subsequent call to the overloaded method correctly handles the transaction creation process.- 4943-5194: The
GetInputsForTx
method is a complex algorithm for selecting inputs for a transaction, considering various constraints such as size, fee, and coin control settings. The logic is intricate, aiming to optimize input selection for efficiency and cost-effectiveness. Given the complexity, ensure that thorough testing covers various scenarios to confirm the algorithm's correctness and efficiency.
CCriticalSection cs; | ||
CInstantSendDb db; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming TxBuilder
to SigmaTxBuilderSuperclass
and updating its constructor similarly improves clarity and specificity. However, the term "Superclass" in the name might be misleading if this class is not intended to be a base class for inheritance. Consider a more descriptive name that reflects its role without implying inheritance if that's not the intent.
- SigmaTxBuilderSuperclass
+ SigmaTxBuilder or SigmaTransactionBuilder
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CTransparentTxout
class has been introduced to handle transparent transactions. Ensure that:
- The class interface is clear and consistent.
- Member variables are appropriately encapsulated.
- Methods that do not modify class state are marked as
const
. - Consider moving the implementation of non-trivial methods to the corresponding
.cpp
file to improve readability and compilation times.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This PR refactors public transaction creation, which is a major change requiring careful review.
Summary by CodeRabbit
CInstantSendManager
to the public section to address duplicate declarations.