Implement new rpc calls to get anonymity sets #1507
Conversation
WalkthroughThe changes introduce two new RPC methods, Changes
Possibly related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 3
🧹 Outside diff range and nitpick comments (3)
src/rpc/server.cpp (1)
343-344: Consider future API versioning strategy.While categorizing these endpoints under "mobile" makes sense for now, consider implementing a more formal API versioning strategy if you plan to add more mobile-specific endpoints in the future. This would help manage API evolution and backward compatibility more effectively.
src/spark/state.cpp (1)
1333-1366: LGTM! Consider adding documentation.The implementation correctly retrieves anonymity set metadata with proper error handling and thread safety (called within LOCK(cs_main)). The logic follows established patterns in the codebase.
Consider adding documentation to describe:
- The purpose and usage of this method
- Parameter requirements/constraints
- Return value semantics
- Thread safety requirements
+/** + * Retrieves metadata about an anonymity set for a given coin group ID. + * Must be called within LOCK(cs_main). + * @param chain The blockchain + * @param maxHeight Maximum block height to consider + * @param coinGroupID The coin group ID to query + * @param blockHash_out The latest block hash for the anonymity set + * @param setHash_out The hash of the anonymity set + * @param size The size of the anonymity set + */ void CSparkState::GetAnonSetMetaData(src/rpc/misc.cpp (1)
1310-1363: LGTM! Consider improving error messages.The RPC implementation is well-structured with proper parameter validation, thread safety, and security checks.
Consider improving error messages to be more user-friendly and descriptive:
- throw std::runtime_error(std::string("An exception occurred while parsing parameters: ") + e.what()); + throw std::runtime_error(std::string("Invalid coinGroupId parameter. Must be a valid integer: ") + e.what()); - throw std::runtime_error(std::string("Please rerun Firo with -mobile ")); + throw std::runtime_error("This RPC call requires the -mobile option to be enabled when starting the node");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/rpc/misc.cpp(2 hunks)src/rpc/server.cpp(1 hunks)src/rpc/server.h(1 hunks)src/spark/state.cpp(1 hunks)src/spark/state.h(1 hunks)
🔇 Additional comments (4)
src/spark/state.h (1)
216-223: LGTM: Well-structured method declaration for retrieving anonymity set metadata.
The parameter types and naming convention are appropriate, and the use of reference parameters for outputs follows good C++ practices.
src/rpc/server.h (1)
217-218: LGTM: RPC function declarations follow established patterns.
The new function declarations are consistent with existing RPC patterns and have clear, descriptive names.
src/rpc/server.cpp (1)
343-344: LGTM: RPC commands properly registered with appropriate settings.
The commands are correctly categorized under "mobile" and have appropriate security settings with okSafe=false.
src/rpc/misc.cpp (1)
2059-2060: LGTM! RPC commands properly registered.
The new RPC methods are correctly registered in the commands array under the "mobile" category with appropriate access flags.
levonpetrosyan93
force-pushed
the
pagination
branch
from
abc7a95 to
fe5f6c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/spark/state.h (2)
216-222: LGTM! Consider adding documentation.The new method declaration follows good practices but would benefit from documentation explaining the parameters and return values.
Add documentation like:
/** * Retrieves metadata about an anonymity set. * @param chain The blockchain * @param maxHeight Maximum block height to consider * @param coinGroupID The coin group ID * @param blockHash_out Latest block hash for the set * @param setHash_out Hash of the anonymity set * @param size Size of the anonymity set */ void GetAnonSetMetaData(...);
224-230: LGTM! Consider adding documentation.The modified method signature is appropriate but would benefit from documentation.
Add documentation like:
/** * Retrieves coins for recovery based on block hash. * @param chain The blockchain * @param maxHeight Maximum block height to consider * @param coinGroupID The coin group ID * @param blockHash Block hash to start recovery from * @param coins Output vector of recovered coins with transaction context */ void GetCoinsForRecovery(...);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/rpc/misc.cpp(2 hunks)src/rpc/server.cpp(1 hunks)src/rpc/server.h(1 hunks)src/spark/state.cpp(1 hunks)src/spark/state.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/rpc/server.h
- src/rpc/server.cpp
🔇 Additional comments (5)
src/spark/state.cpp (2)
1333-1366: LGTM! Implementation follows established patterns.
The implementation correctly:
- Handles error cases for non-existent coin groups
- Traverses blocks to gather metadata
- Updates output parameters appropriately
1368-1411: LGTM! Implementation follows established patterns.
The implementation correctly:
- Handles error cases for non-existent coin groups
- Uses block hash parameter to find starting point
- Gathers coins and transaction context
src/rpc/misc.cpp (3)
1310-1363: LGTM! Implementation follows RPC conventions.
The RPC method correctly:
- Validates parameters
- Checks for -mobile flag
- Uses proper locking
- Returns data in expected format
2065-2066: LGTM! RPC registration follows conventions.
The new RPC methods are properly registered in the command table.
1365-1449: 🛠️ Refactor suggestion
Add index range validation and consider performance optimization.
While the implementation is functionally correct, it needs additional validation and could benefit from performance improvements.
- Add validation for index ranges:
try {
coinGroupId = std::stol(request.params[0].get_str());
latestBlock = request.params[1].get_str();
startIndex = std::stol(request.params[2].get_str());
endIndex = std::stol(request.params[3].get_str());
+
+ if (startIndex < 0) {
+ throw std::runtime_error("startIndex must be non-negative");
+ }
+ if (endIndex < startIndex) {
+ throw std::runtime_error("endIndex must be greater than or equal to startIndex");
+ }
+ if (endIndex - startIndex > 1000) {
+ throw std::runtime_error("Range too large. Maximum range is 1000");
+ }
}- Consider performance optimization:
std::vector<std::pair<spark::Coin, std::pair<uint256, std::vector<unsigned char>>>> coins;
+ // Pre-allocate vector with estimated size to avoid reallocations
+ coins.reserve(endIndex - startIndex + 1);Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/spark/state.cpp (2)
1333-1366: Add parameter validation for GetAnonSetMetaData.The implementation looks good but should validate input parameters for robustness.
Add validation at the start of the method:
void CSparkState::GetAnonSetMetaData( CChain *chain, int maxHeight, int coinGroupID, uint256& blockHash_out, std::vector<unsigned char>& setHash_out, int& size) { + if (!chain) { + throw std::invalid_argument("Chain parameter cannot be null"); + } + if (maxHeight < 0) { + throw std::invalid_argument("maxHeight must be non-negative"); + } if (coinGroups.count(coinGroupID) == 0) { return; } // ... rest of the implementation
1368-1415: Improve error handling and parameter validation.The implementation looks good but could benefit from improved error handling and validation.
- Add parameter validation:
void CSparkState::GetCoinsForRecovery( CChain *chain, int maxHeight, int coinGroupID, uint256& blockHash, std::vector<std::pair<spark::Coin, std::pair<uint256, std::vector<unsigned char>>>>& coins) { + if (!chain) { + throw std::invalid_argument("Chain parameter cannot be null"); + } + if (maxHeight < 0) { + throw std::invalid_argument("maxHeight must be non-negative"); + } coins.clear(); if (coinGroups.count(coinGroupID) == 0) { - throw std::runtime_error(std::string("There is no anonymity set with this id: " + coinGroupID)); + throw std::runtime_error(std::string("No anonymity set found with ID: ") + std::to_string(coinGroupID)); } // ... rest of the implementationsrc/rpc/misc.cpp (1)
1310-1363: Add range validation for coinGroupId parameter.The implementation looks good but should validate the coinGroupId parameter range.
Add validation after parsing the parameter:
try { coinGroupId = std::stol(request.params[0].get_str()); + if (coinGroupId <= 0) { + throw std::runtime_error("coinGroupId must be positive"); + } } catch (std::logic_error const & e) { throw std::runtime_error(std::string("An exception occurred while parsing parameters: ") + e.what()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/rpc/misc.cpp(2 hunks)src/spark/state.cpp(1 hunks)
🔇 Additional comments (2)
src/rpc/misc.cpp (2)
2075-2076: LGTM! RPC command registration is correct.
The new RPC commands are properly registered in the commands array with appropriate category and safe mode settings.
1365-1459: 🛠️ Refactor suggestion
Add comprehensive parameter validation.
The implementation needs additional validation for the parameters.
Add validation after parsing parameters:
try {
coinGroupId = std::stol(request.params[0].get_str());
latestBlock = request.params[1].get_str();
startIndex = std::stol(request.params[2].get_str());
endIndex = std::stol(request.params[3].get_str());
+ if (coinGroupId <= 0) {
+ throw std::runtime_error("coinGroupId must be positive");
+ }
+ if (startIndex < 0) {
+ throw std::runtime_error("startIndex must be non-negative");
+ }
+ if (endIndex <= startIndex) {
+ throw std::runtime_error("endIndex must be greater than startIndex");
+ }
+ if (endIndex - startIndex > 1000) {
+ throw std::runtime_error("Range too large. Maximum range is 1000");
+ }
} catch (std::logic_error const & e) {
throw std::runtime_error(std::string("An exception occurred while parsing parameters: ") + e.what());
}Additionally, consider pre-allocating the coins vector to avoid reallocations:
std::vector<std::pair<spark::Coin, std::pair<uint256, std::vector<unsigned char>>>> coins;
+ coins.reserve(endIndex - startIndex + 1);Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/spark/state.h (2)
216-223: LGTM! Consider adding parameter documentation.The new GetAnonSetMetaData method signature looks good. Consider adding documentation comments to describe the parameters and their valid ranges.
+ /** + * Gets metadata about an anonymity set + * @param chain The blockchain + * @param maxHeight Maximum block height to consider + * @param coinGroupID The coin group ID + * @param blockHash_out Latest block hash for the set + * @param setHash_out Hash of the anonymity set + * @param size Size of the anonymity set + */ void GetAnonSetMetaData( CChain *chain, int maxHeight, int coinGroupID, uint256& blockHash_out, std::vector<unsigned char>& setHash_out, int& size);
224-232: LGTM! Consider adding parameter documentation.The new GetCoinsForRecovery method signature looks good. Consider adding documentation comments to describe the parameters and their valid ranges.
+ /** + * Gets coins from an anonymity set with index-based pagination + * @param chain The blockchain + * @param maxHeight Maximum block height to consider + * @param coinGroupID The coin group ID + * @param startIndex Start index for pagination (inclusive) + * @param endIndex End index for pagination (exclusive) + * @param blockHash Block hash to retrieve coins from + * @param coins Retrieved coins with their transaction context + */ void GetCoinsForRecovery( CChain *chain, int maxHeight, int coinGroupID, int startIndex, int endIndex, uint256& blockHash, std::vector<std::pair<spark::Coin, std::pair<uint256, std::vector<unsigned char>>>>& coins);src/spark/state.cpp (1)
1368-1425: Consider additional input validation and performance optimization.The GetCoinsForRecovery implementation looks good but could benefit from:
- Input validation for index ranges
- Vector pre-allocation for better performance
void CSparkState::GetCoinsForRecovery( CChain *chain, int maxHeight, int coinGroupID, int startIndex, int endIndex, uint256& blockHash, std::vector<std::pair<spark::Coin, std::pair<uint256, std::vector<unsigned char>>>>& coins) { coins.clear(); + if (startIndex < 0) { + throw std::runtime_error("startIndex must be non-negative"); + } + if (endIndex < startIndex) { + throw std::runtime_error("endIndex must be greater than or equal to startIndex"); + } + if (endIndex - startIndex > 1000) { + throw std::runtime_error("Range too large. Maximum range is 1000"); + } if (coinGroups.count(coinGroupID) == 0) { throw std::runtime_error(std::string("There is no anonymity set with this id: " + coinGroupID)); } SparkCoinGroupInfo &coinGroup = coinGroups[coinGroupID]; + // Pre-allocate vector with estimated size to avoid reallocations + coins.reserve(endIndex - startIndex + 1);src/rpc/misc.cpp (1)
1365-1452: Consider additional error handling for base64 decoding.The getsparkanonymitysetsector implementation looks good but could benefit from additional error handling around base64 decoding.
std::string strHash = DecodeBase64(latestBlock); + if (strHash.empty()) { + throw std::runtime_error("Failed to decode blockHash from base64"); + } std::vector<unsigned char> vec(strHash.begin(), strHash.end()); if (vec.size() != 32) throw std::runtime_error(std::string("Provided blockHash data is not correct."));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/rpc/misc.cpp(2 hunks)src/spark/state.cpp(1 hunks)src/spark/state.h(1 hunks)
🔇 Additional comments (3)
src/spark/state.cpp (1)
1333-1366: LGTM! Implementation handles edge cases properly.
The GetAnonSetMetaData implementation:
- Validates coin group existence
- Properly initializes output parameters
- Efficiently calculates set size
src/rpc/misc.cpp (2)
1310-1363: LGTM! Clear error handling and response format.
The getsparkanonymitysetmeta implementation:
- Validates input parameters
- Checks for -mobile flag
- Returns well-structured response with blockHash, setHash and size
2068-2069: LGTM! RPC commands properly registered.
The new RPC commands are correctly registered in the command table under the "mobile" category.
Introducing two new RPC calls.
getsparkanonymitysetmetareturns all metadata related to anonymity set. set size, latest block hash and anonymity set hashgetsparkanonymitysetsectorreturns a sector of anonymity set based on provided range, set id and latest block hash