Mweb_wallet.cpp > Observations
DaCryptoRaccoon opened this issue · 6 comments
The code in mweb_wallet.cpp is related to the wallet system that involves upgrading output coins and performing various wallet operations. During the review, several potential issues were identified:
Lack of null checks: The code fails to perform null checks on pointers before accessing their members. This can lead to crashes or undefined behavior if the pointers are null. It is crucial to validate pointers to ensure they are valid before using them.
Unused function result: The RewindOutputs() function returns a vector of mw::Coin objects but is not utilized in the Wallet::UpgradeCoins() function. This suggests a possible inconsistency or incomplete implementation.
Uniqueness of identifiers: The code assumes that coin.output_id uniquely identifies each coin. If there are any duplicate output IDs, it may lead to collisions or data corruption when updating the m_coins map. Ensuring the uniqueness of identifiers is important for maintaining data integrity.
Unused WalletBatch object: In the Wallet::RewindOutput() function, a WalletBatch object is created but not used for batch operations. This could be an oversight, and it is essential to use the batch object correctly to ensure proper data handling.
Null pointer assumption: The Wallet::GetStealthAddress(const mw::Coin&, StealthAddress&) function assumes that coin.address is a valid pointer without performing a null check. Validating the pointer's existence is crucial to avoid potential crashes or accessing invalid memory.
Overwriting existing data: The LoadToWallet() and SaveToWallet() functions update the m_coins map without checking if the coin.output_id already exists in the map. This can lead to unintended data overwriting or corruption.
Correct handling of Reset(): The GetCoin() function calls coin.Reset() before returning false when the coin is not found. It is important to ensure that the Reset() function correctly handles resetting the coin object to avoid any unintended side effects.
Importance of these issues:
These issues are important to address for several reasons:
Robustness and stability: Proper null checks and handling of pointers help prevent crashes and undefined behavior, making the code more robust and stable.
Data integrity: Ensuring the uniqueness of identifiers and avoiding overwriting existing data is crucial for maintaining the integrity of the wallet's data. Collisions or data corruption could lead to incorrect balances or other inconsistencies.
Correctness of operations: Utilizing the correct objects, such as the WalletBatch object, for intended batch operations ensures that the desired operations are performed correctly and consistently.
Security: Careful validation of pointers and data can help prevent potential security vulnerabilities, such as null pointer dereferences or memory-related issues.
By addressing these issues, we can improve the reliability, integrity, and security of the code, leading to a more robust and trustworthy wallet system.
In the Wallet::UpgradeCoins() function, you're using a raw pointer CWalletTx* wtx without performing a null check. It's a good practice to validate the pointer before accessing its members to avoid potential crashes or undefined behavior.
The RewindOutputs() function returns a vector of mw::Coin objects, but it's not used in the Wallet::UpgradeCoins() function. You may want to consider utilizing the returned vector to process the rewound coins.
In the Wallet::RewindOutput() function, you're using the coin.output_id as the key to update m_coins. Ensure that the coin.output_id uniquely identifies each coin to prevent potential collisions or data corruption.
In the Wallet::RewindOutput() function, you're constructing a WalletBatch object without assigning it to a variable or using it for batch operations. Make sure you're using the batch object correctly for writing MWEBCoin and Tx data.
The Wallet::GetStealthAddress(const mw::Coin&, StealthAddress&) function first checks if the coin has an address using coin.HasAddress(). However, in the subsequent code, you assume that coin.address is a valid pointer without performing a null check. It's important to ensure the validity of the pointer before dereferencing it.
In the Wallet::LoadToWallet() function, you're using m_coins[coin.output_id] = coin; to update the m_coins map. It's essential to verify that the coin.output_id doesn't already exist in the map to avoid overwriting existing data unintentionally.
In the Wallet::SaveToWallet() function, you're using a loop to iterate over the coins vector and update the m_coins map. Similar to the previous point, you should check if each coin.output_id already exists in the map to prevent data corruption.
The Wallet::GetCoin() function assigns coin.Reset() before returning false when the coin is not found. Ensure that the Reset() function handles resetting the coin object correctly.
Any updates on this?
Closing. ChatGPT/AI generated code review isn't very useful, and if anything can lead to hours spent attempting to go through these 'suggestions'.
Chat GPT formatted the text it didn't review the code.
I have dyslexia and use it to help format text.
The issues above were found while testing.
Dissapointed.. to say the leist with this response.
Are you saying Litecoin are going to ignore the issues rasied?
If so this is very concerning.
Also I have seen other issues closed out when there was no solution found or before merges for fixed were done. This is very bad practice.
You had mentioned to me in the past that it was generated by ChatGPT, and the suggestions are exactly what I would expect something like ChatGPT to come up with: A lack of knowledge about the context of the code, and instead just general comments about things to watch for, best practices, etc... Not actual issues with the code.
There's a low SNR in this review, and a handful of inaccuracies (for example, Unused WalletBatch object: In the Wallet::RewindOutput() function, a WalletBatch object is created but not used for batch operations is just wrong, because the WalletBatch is used on the same line it's created: https://github.com/litecoin-project/litecoin/blob/master/src/mweb/mweb_wallet.cpp#L73)
It's also full of comments about checking for null pointers before using them, but in all cases mentioned, we already guarantee them to be non-null. Though some may still prefer to add defensive null checks, that's not the coding style bitcoin/litecoin adheres to, and leads to a lot of additional code complexity for no functional gain.
I've read through and considered all of the listed points, and I appreciate the suggestions, but since they're largely just style suggestions and "best practices" guidelines, it shouldn't be alarming that we aren't individually addressing each of those bulletpoints.
@DaCryptoRaccoon another point to add:
While ChatGPT can generate a grammatically correct response (per your comments) it has no ability to provide an optimal solution for any query. Its the average of all opinions on how to handle problems...it has knows no idea what you need.
If you are in the sw dev world, you know style decisions are key to maintaining the codebase and we rely heavily on @DavidBurkett and @losh11 decision process on maintaining the codebase.
Are you using MWEB @DaCryptoRaccoon ?