Gettotalsupply #1502
Gettotalsupply #1502
Conversation
WalkthroughThe changes introduce new functions and modify existing functionality in the Changes
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/rpc/misc.cpp (3)
1769-1771: Consider using range-based for loops for improved readabilityThe iterator-based loops can be simplified using range-based for loops, which enhance readability and reduce the potential for errors.
Apply this change:
-for (std::vector<std::pair<CAddressIndexKey, CAmount> >::const_iterator it = addressIndex.begin(); it != addressIndex.end(); it++) { - nTotalAmount += it->second; +for (const auto& entry : addressIndex) { + nTotalAmount += entry.second; }🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 1769-1769: Prefer prefix ++/-- operators for non-primitive types.
(postfixOperator)
1769-1769: Prefer prefix increment operator for iteratorsUsing the prefix increment operator (
++it) is more efficient for iterators of non-primitive types because it avoids unnecessary copies.Apply this minor change:
-for (std::vector<std::pair<CAddressIndexKey, CAmount> >::const_iterator it = addressIndex.begin(); it != addressIndex.end(); it++) { +for (std::vector<std::pair<CAddressIndexKey, CAmount> >::const_iterator it = addressIndex.begin(); it != addressIndex.end(); ++it) {🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 1769-1769: Prefer prefix ++/-- operators for non-primitive types.
(postfixOperator)
1803-1805: Avoid variable shadowing by renaming the innertxvariableThe variable
txis redeclared within the inner scope, shadowing the outertxand potentially causing confusion.Rename the inner variable to prevent shadowing:
-CTransactionRef tx; +CTransactionRef prevTx; uint256 hashBlock; -if (!GetTransaction(txin.prevout.hash, tx, Params().GetConsensus(), hashBlock, true)) { +if (!GetTransaction(txin.prevout.hash, prevTx, Params().GetConsensus(), hashBlock, true)) { continue; } -amount += tx->vout[txin.prevout.n].nValue; +amount += prevTx->vout[txin.prevout.n].nValue;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/rpc/misc.cpp(3 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
src/rpc/misc.cpp
[performance] 1769-1769: Prefer prefix ++/-- operators for non-primitive types.
(postfixOperator)
[performance] 1778-1778: Prefer prefix ++/-- operators for non-primitive types.
(postfixOperator)
🔇 Additional comments (4)
src/rpc/misc.cpp (4)
1777-1780: Consider using range-based for loops for improved readability
Similar to the previous loop, you can simplify this loop using a range-based for loop.
🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 1778-1778: Prefer prefix ++/-- operators for non-primitive types.
(postfixOperator)
1778-1778: Prefer prefix increment operator for iterators
Same as above, using ++it is more efficient.
🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 1778-1778: Prefer prefix ++/-- operators for non-primitive types.
(postfixOperator)
1865-1882: Rename overloaded functions to avoid confusion with different return types
Similarly, the function getCVE17144amount() is overloaded with different return types. Renaming one of them will prevent confusion.
1797-1798:
Variable vInOutPoints is redeclared, causing shadowing and logical errors
The variable vInOutPoints is declared twice: once outside the if block and once inside it, which leads to the inner declaration shadowing the outer one. This causes the outer vInOutPoints to remain unused and may lead to incorrect behavior.
Remove the inner declaration to use the intended vInOutPoints:
- std::set<COutPoint> vInOutPoints;Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/rpc/misc.cpp(3 hunks)
🔇 Additional comments (1)
src/rpc/misc.cpp (1)
2017-2018: LGTM!
The RPC command registration follows the existing pattern and correctly maps the new functions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/rpc/misc.cpp (1)
2020-2021: Rename overloaded functions to avoid confusion with different return typesThe function
getzerocoinpoolbalance()is overloaded with different return types (CAmountandUniValue), which can cause confusion and potential errors.Consider renaming one of the functions to clarify their purposes. For example:
- Rename the RPC handler to
getzerocoinpoolbalanceRPC().- Update the command registration accordingly.
-UniValue getzerocoinpoolbalance(const JSONRPCRequest& request) +UniValue getzerocoinpoolbalanceRPC(const JSONRPCRequest& request) ... -{ "addressindex", "getzerocoinpoolbalance", &getzerocoinpoolbalance, false }, +{ "addressindex", "getzerocoinpoolbalance", &getzerocoinpoolbalanceRPC, false },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/rpc/misc.cpp(3 hunks)
🔇 Additional comments (3)
src/rpc/misc.cpp (3)
1785-1825: LGTM! Previous review comments have been addressed
The implementation now includes:
- Proper chain height validation
- Error handling for block reading
- Documentation explaining CVE-2018-17144
- Bounds checking for output index
1848-1850: 🛠️ Refactor suggestion
Document magic numbers and add validation
The total supply calculation uses magic numbers without proper documentation and lacks validation of the adjustment values.
Apply this diff to improve the code:
+ // Adjust total supply by:
+ // 1. Subtracting zerocoin pool balance (coins forged during Zerocoin attacks)
+ CAmount zeroPoolBalance = getzerocoinpoolbalance();
+ if (zeroPoolBalance == 0) {
+ throw std::runtime_error("Failed to calculate zerocoin pool balance");
+ }
+ total -= zeroPoolBalance;
+
+ // 2. Adding coins forged during CVE-2018-17144 attacks
+ CAmount cveAmount = getCVE17144amount();
+ if (cveAmount == 0) {
+ throw std::runtime_error("Failed to calculate CVE-17144 amount");
+ }
+ total += cveAmount;
+
+ // 3. Subtracting coins sent to unrecoverable address
+ // Reference: https://explorer.firo.org/tx/0b53178c1b22bae4c04ef943ee6d6d30f2483327fe9beb54952951592e8ce368
+ const CAmount BURNT_COINS = 16810168037465;
+ total -= BURNT_COINS;Likely invalid or redundant comment.
1762-1783:
Fix balance calculation and add error handling
The current implementation has potential issues:
- The balance calculation adds both mints and spends without considering signs, which could lead to incorrect totals.
- Missing error handling for GetAddressIndex failures.
Apply this diff to fix the calculation and add error handling:
CAmount getzerocoinpoolbalance()
{
CAmount nTotalAmount = 0;
// Iterate over all mints
std::vector<std::pair<CAddressIndexKey, CAmount> > addressIndex;
- if (GetAddressIndex(uint160(), AddressType::zerocoinMint, addressIndex)) {
+ if (!GetAddressIndex(uint160(), AddressType::zerocoinMint, addressIndex)) {
+ throw std::runtime_error("Failed to get zerocoin mint index");
+ }
for (auto& it : addressIndex) {
nTotalAmount += it.second;
}
addressIndex.clear();
// Iterate over all spends
- if (GetAddressIndex(uint160(), AddressType::zerocoinSpend, addressIndex)) {
+ if (!GetAddressIndex(uint160(), AddressType::zerocoinSpend, addressIndex)) {
+ throw std::runtime_error("Failed to get zerocoin spend index");
+ }
for (auto& it : addressIndex) {
- nTotalAmount += it.second;
+ nTotalAmount -= it.second; // Subtract spends
}
return nTotalAmount;
}Likely invalid or redundant comment.
No description provided.