Fix GH-683 critical issues from code review

[kauri-hero]

• Fix all 'costume' typos to 'custom' in method names
• Fix test function name typo 'app_arp' to 'app_rpc'
• Improve hash test efficiency using HashSet (O(n) instead of O(n²))
• Add overflow protection for ErrorStats::increment using saturating_add
• Replace magic numbers with named constants in hash implementation
• Rename dup() method to clone_boxed() for clarity
• Make ValidationFailureClockReal a unit struct
• Replace unwrap() with expect() in tests for better error messages
• Fix error message inconsistency in payable_dao.rs
• Fix comment references to 'app_rpc_web3_error_kind'

Fix Critical Issues from Code Review of PR #684

Summary

This PR addresses the critical issues identified in the code review of PR #684 (branch GH-683). The fixes focus on improving code quality, fixing typos, enhancing performance, and making the code more idiomatic Rust.

Changes Made

🔧 Critical Fixes

1. Fixed all "costume" → "custom" typos (Critical)

   • Updated method names across 5 files: custom_serialize, custom_deserialize, custom_hash
   • This was a pervasive typo that affected the entire error handling module

2. Fixed test function name typo

   • Renamed hashing_for_app_arp_error_kind_works → hashing_for_app_rpc_error_kind_works

3. Improved hash test efficiency

   • Replaced O(n²) nested loop algorithm with HashSet-based O(n) approach
   • Cleaner, more efficient, and more idiomatic implementation

4. Added overflow protection

   • ErrorStats::increment now uses saturating_add to prevent panic on u16 overflow
   • Ensures robust error tracking even with many attempts

5. Replaced magic numbers with named constants

   • Added descriptive constants for hash discriminants:

     const HASH_DECODER: u8 = 0;
     const HASH_INTERNAL: u8 = 1;
     // ... etc

6. Renamed dup() → clone_boxed()

   • More idiomatic Rust naming convention
   • Clearer intent of the method

7. Made ValidationFailureClockReal a unit struct

   • Changed from empty struct {} to unit struct ;
   • Follows Rust best practices for zero-sized types

8. Improved test error messages

   • Replaced .unwrap() with .expect("descriptive message") in tests
   • Better debugging experience when tests fail

9. Fixed inconsistent error messages

   • Corrected "app_rpc_web3_error_kind" → "errors" in payable_dao.rs

10. Fixed comment inconsistencies

    • Updated comments that incorrectly referenced "app_rpc_web3_error_kind"

Files Modified

10 files changed, 68 insertions(+), 59 deletions(-)

• node/src/accountant/db_access_objects/payable_dao.rs
• node/src/blockchain/errors/blockchain_db_error/app_rpc_web3_error_kind.rs
• node/src/blockchain/errors/blockchain_db_error/masq_error_kind.rs
• node/src/blockchain/errors/blockchain_db_error/mod.rs
• node/src/blockchain/errors/blockchain_loggable_error/app_rpc_web3_error.rs
• node/src/blockchain/errors/blockchain_loggable_error/masq_error.rs
• node/src/blockchain/errors/blockchain_loggable_error/mod.rs
• node/src/blockchain/errors/common_methods.rs
• node/src/blockchain/errors/test_utils.rs
• node/src/blockchain/errors/validation_status.rs

Items Not Addressed (Lower Priority)

The following items were identified in the code review but are considered lower priority and can be addressed in a follow-up PR if needed:

Performance Optimizations

• Optimize deserializer: Currently does double serialization (Value → String → Type). Could be optimized to deserialize directly from Value
• Make add_attempt more efficient: Currently takes ownership and returns Self. Could use &mut self for better performance

Code Consistency

• Fix IO vs Io naming inconsistency: Mixed casing throughout codebase. Should standardize on one convention
• Align error naming: ServerUnreachable vs Unreachable - inconsistent naming between error types

Architectural Considerations

• Consider enum keys for HashMap: Current HashMap<Box<dyn BlockchainDbError>, ErrorStats> has trait object overhead
• Add bounds checking for PreviousAttempts: HashMap could grow unbounded - may need max entries limit or TTL

These items don't affect correctness or immediate functionality but could improve performance and maintainability in the future.

Testing

All changes maintain backward compatibility and don't alter the fundamental behavior of the error handling system. The improvements focus on:

• Code quality and idioms
• Performance (hash test from O(n²) to O(n))
• Safety (overflow protection)
• Maintainability (named constants, better error messages)

Related Issues

• Addresses code review feedback from PR GH-683: Well structualized blockchain errors  #684
• Part of the blockchain error restructuring effort (Dev sharing: Blockchain errors #683)

---

Ready for review and merge into GH-683 branch

[bertllll]

@kauri-hero

I like some of the changes but I wouldn't say all of them. Maybe half makes sense, otherwise it is unnecessary storming from the AI.

[kauri-hero]

@kauri-hero

I like some of the changes but I wouldn't say all of them. Maybe half makes sense, otherwise it is unnecessary storming from the AI.

Yes totally understood - this was just an experiment in getting feedback from Cursor review.
I believe its because of the way the prompt was formed, asking for 20 items to suggest fixing.

do only what is valuable, then we can delete this branch and PR forever