Fix block index issue with wallet scan #1495
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the Changes
Possibly related PRs
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: 0
🧹 Outside diff range and nitpick comments (3)
src/wallet/wallet.cpp (3)
2385-2389: Add documentation explaining the 200 block offsetThe magic number 200 should be documented to explain its purpose in the date-based block scanning logic. Consider adding a constant with a descriptive name and comment.
+ // Number of blocks to offset from target date to account for timestamp variance + static const int DATE_BLOCK_OFFSET = 200; + if (pindex->GetBlockTime() > targetTimestamp) { - if (pindex->nHeight >= 200) { - return chainActive[pindex->nHeight - 200]; + if (pindex->nHeight >= DATE_BLOCK_OFFSET) { + return chainActive[pindex->nHeight - DATE_BLOCK_OFFSET];
2385-2389: Add logging for debugging purposesConsider adding debug logging to help troubleshoot date-based scanning issues:
+ LogPrint("wallet", "GetBlockByDate: searching for block at date %s\n", dateStr); + LogPrint("wallet", "GetBlockByDate: found block at height %d\n", pindex->nHeight);
2385-2389: Consider adding unit testsThe new date-based block scanning functionality should be covered by unit tests to verify correct behavior with:
- Valid dates in different formats
- Invalid date strings
- Edge cases around the 200 block offset
- Chain height validation
Would you like me to help create unit tests for the new functionality?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/wallet/wallet.cpp(1 hunks)
🔇 Additional comments (3)
src/wallet/wallet.cpp (3)
2385-2389: 🛠️ Refactor suggestion
Add input validation for date string format
The date string parsing should validate the input format and handle invalid dates gracefully. Consider adding validation and error handling:
+ if (dateStr.empty() || dateStr.length() != 10) { // Expect DD-MM-YYYY
+ throw std::invalid_argument("Invalid date format, expected DD-MM-YYYY");
+ }
+
std::time_t targetTimestamp = parseDate(dateStr);
+ if (targetTimestamp == -1) {
+ throw std::invalid_argument("Failed to parse date string");
+ }2385-2389:
Add bounds checking for chainActive access
Direct array access to chainActive should include bounds checking to prevent potential crashes:
- return chainActive[chainActive.Tip()->nHeight];
+ const int tipHeight = chainActive.Tip()->nHeight;
+ if (tipHeight < 0) {
+ throw std::runtime_error("Invalid chain height");
+ }
+ return chainActive[tipHeight];Likely invalid or redundant comment.
2385-2389: 🛠️ Refactor suggestion
Add validation and error handling for block height check
The block height validation logic should handle edge cases more gracefully. Consider:
- if (pindex->nHeight >= 200) {
- return chainActive[pindex->nHeight - 200];
- } else {
- return chainActive[0];
- }
+ int targetHeight = std::max(0, pindex->nHeight - 200);
+ if (!chainActive[targetHeight]) {
+ throw std::runtime_error("Invalid block height");
+ }
+ return chainActive[targetHeight];
No description provided.