feat(sdk)!: ban addresses failed in sdk #2351
Conversation
WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
# Conflicts: # packages/rs-dapi-client/src/dapi_client.rs
shumkov
changed the title
![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 (6)
packages/rs-dapi-client/Cargo.toml (1)
40-41: Consider adding version constraintsWhile using the exact version "6.1.0" works, consider using semantic versioning constraints to automatically receive compatible bug fixes:
-dashmap = "6.1.0" +dashmap = "~6.1"This will allow minor version updates within the 6.1.x range while preventing breaking changes.
packages/rs-sdk/src/platform/transition/broadcast.rs (1)
Line range hint
125-144: Enhance error messages for address banning scenarios.While the error handling is comprehensive for timeouts and state transition details, consider adding information about banned addresses when retries are exhausted.
Add address context to timeout error message:
Error::TimeoutReached( timeout, - format!("Timeout waiting for result of {} (tx id: {}) affecting object {}: {:?}", + format!("Timeout waiting for result of {} (tx id: {}) affecting object {} (available addresses: {}): {:?}", self.name(), self.transaction_id().map(hex::encode).unwrap_or("UNKNOWN".to_string()), self.unique_identifiers().join(","), + sdk.address_list().available().join(","), e), )packages/rs-sdk/src/platform/fetch_many.rs (1)
330-330: Consider using the default trait implementation.While the change to use
sdk.address_list()is correct, this implementation could potentially be simplified by using the default trait implementation offetch_many_with_metadata_and_proofinstead of maintaining a custom one. This would reduce code duplication and maintenance overhead.Consider removing the custom implementation if there are no specific requirements for the Document type that differ from the default implementation.
packages/rs-dapi-client/src/address_list.rs (2)
Line range hint
139-151: Possible Race Conditions inban_addressMethodThe
ban_addressmethod removes anAddressfromself.addresses, modifies it, and reinserts it. In a concurrent environment, this remove-modify-insert pattern can lead to race conditions, causing inconsistent states or lost updates if multiple threads access the method simultaneously. SinceDashSetis designed for concurrent access, directly removing and reinserting items without proper synchronization may result in unpredictable behavior.Suggestion:
Consider using an atomic update mechanism to modify the
Addressin place without removing it from theDashSet. One approach is to switch to aDashMap<Uri, Address>, where theUriis the key, allowing you to use methods likeentryorget_mutto safely mutate theAddressunder a lock. This ensures thread safety and prevents potential race conditions.Another option is to use the
DashSet::getmethod to obtain a reference to theAddressand employ interior mutability (e.g., usingAtomicUsizeforban_countandAtomicOption<DateTime<Utc>>forbanned_until) within theAddressstruct. This would allow you to modify the fields atomically without removing theAddressfrom the set.
Line range hint
153-165: Possible Race Conditions inunban_addressMethodSimilar to
ban_address, theunban_addressmethod removes anAddress, modifies it, and reinserts it intoself.addresses. This remove-modify-insert pattern is susceptible to race conditions in concurrent usage, potentially leading to inconsistent states or lost updates when accessed by multiple threads simultaneously.Suggestion:
Adopt an atomic update strategy as suggested for the
ban_addressmethod. Using aDashMap<Uri, Address>would allow safe in-place modifications of theAddresswithout removing it from the collection. Alternatively, implement interior mutability within theAddressstruct to atomically modify its state while it remains in theDashSet. This change will enhance thread safety and prevent race conditions.packages/rs-dapi-client/src/dapi_client.rs (1)
201-220: Refactor ban/unban logic to reduce code duplicationThe ban and unban logic share similar patterns, including error handling and logging. Consider refactoring this into a common helper function to improve maintainability and reduce duplication.
Also applies to: 228-250
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
packages/rs-dapi-client/Cargo.toml(1 hunks)packages/rs-dapi-client/src/address_list.rs(7 hunks)packages/rs-dapi-client/src/dapi_client.rs(6 hunks)packages/rs-drive-proof-verifier/src/error.rs(0 hunks)packages/rs-sdk/src/platform/fetch.rs(1 hunks)packages/rs-sdk/src/platform/fetch_many.rs(2 hunks)packages/rs-sdk/src/platform/fetch_unproved.rs(1 hunks)packages/rs-sdk/src/platform/transition/broadcast.rs(2 hunks)packages/rs-sdk/src/sdk.rs(1 hunks)packages/rs-sdk/src/sync.rs(6 hunks)
💤 Files with no reviewable changes (1)
- packages/rs-drive-proof-verifier/src/error.rs
🔇 Additional comments (14)
packages/rs-dapi-client/Cargo.toml (1)
40-41: LGTM! Good choice for concurrent operations
The addition of dashmap is well-aligned with the PR objectives. It's an excellent choice for replacing RwLock<HashSet> in mixed async/sync code contexts, providing better performance characteristics for concurrent operations.
packages/rs-sdk/src/platform/fetch_unproved.rs (1)
109-111: LGTM! The retry function update aligns with SDK-level address banning.
The change to include sdk.address_list() as the first parameter enables address banning during retries, which is consistent with the PR's objective of implementing address banning at the SDK level.
Let's verify that this pattern is consistently applied across the codebase:
✅ Verification successful
All retry function calls consistently use address_list as the first parameter
The verification confirms that all retry function calls across the codebase follow the expected pattern:
- All instances pass
address_listas the first parameter (either directly or viasdk.address_list()) - The function consistently takes three parameters in the order: address_list, settings, and closure
- This pattern is uniformly applied across different modules:
- platform/fetch_unproved.rs
- platform/fetch_many.rs
- platform/fetch.rs
- platform/transition/broadcast.rs
- sync.rs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of retry function with address_list
# Expect: All retry calls should follow the pattern: retry(sdk.address_list(), settings, closure)
# Search for retry function calls
ast-grep --pattern 'retry($_, $_, $_)'
Length of output: 3049
packages/rs-sdk/src/platform/transition/broadcast.rs (1)
55-58: LGTM! Verify error handling for address banning.
The addition of sdk.address_list() to the retry call enables proper address management during retries, aligning with the PR's objective to implement address banning at the SDK level.
Let's verify the error handling chain:
✅ Verification successful
LGTM! Error handling for address banning is properly implemented.
The verification shows that the error handling chain for address banning is correctly implemented:
ExecutionErrorproperly includes address information and retry counts- The retry mechanism in
sync.rshandles address banning/unbanning throughaddress_list.ban_address()andaddress_list.unban_address() - The broadcast implementation correctly passes the
address_listto the retry mechanism, enabling proper address management during retries - Error propagation includes address context for both successful and failed cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that ExecutionError properly propagates address information for banning
# Look for ExecutionError construction with address context in retry implementations
rg -A 3 "ExecutionError\s*{\s*inner:" packages/rs-sdk/src/
Length of output: 188
Script:
#!/bin/bash
# Let's try a different approach to find ExecutionError usage patterns
rg -A 5 "ExecutionError" packages/rs-sdk/src/
# Also search for the retry implementation to understand error handling
ast-grep --pattern 'pub async fn retry<$_>($_) -> $_'
Length of output: 7774
Script:
#!/bin/bash
# Let's examine the retry implementation in the rs-dapi-client crate
rg -A 10 "retry\(" packages/rs-sdk/src/
# Also check for address list usage in error handling
rg -A 5 "address_list" packages/rs-sdk/src/
Length of output: 17322
packages/rs-sdk/src/platform/fetch.rs (2)
198-198: LGTM: Enhanced retry mechanism with address list management
The change properly integrates address list management into the retry mechanism, enabling address banning at the SDK level as intended.
198-198: Verify thread-safety of AddressList access
Since AddressList is now shared between retries and the RwLock was removed, we should verify that the concurrent access is properly handled.
✅ Verification successful
Let me search for the AddressList implementation to verify its thread-safety.
Let me search for the AddressList struct definition and its thread-safety mechanisms.
AddressList implementation is thread-safe
The AddressList implementation is thread-safe by design:
- It uses
Arc<DashSet<Address>>internally where:Arcprovides thread-safe shared ownershipDashSetis a concurrent hash set implementation that allows multiple threads to access it safely
- All methods operate on
&selfusing the thread-safe DashSet operations - No additional synchronization is needed as the concurrent collection handles it internally
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check AddressList implementation for thread-safety mechanisms
# Look for DashMap or other concurrent collections usage in AddressList
# Search for AddressList implementation
ast-grep --pattern 'struct AddressList {
$$$
}'
# Search for concurrent collection usage
rg --type rust 'use.*dashmap|DashMap|Arc|Mutex|RwLock' -A 5
Length of output: 157932
Script:
#!/bin/bash
# Search for AddressList implementation
ast-grep --pattern 'struct AddressList {
$$$
}'
# Search for AddressList methods
ast-grep --pattern 'impl AddressList {
$$$
}'
# Search for AddressList usage in retry function
rg --type rust 'retry.*AddressList' -A 5
Length of output: 7416
Script:
#!/bin/bash
# Search for AddressList struct definition and imports
rg --type rust -A 10 'struct AddressList \{'
# Search for DashSet usage in AddressList
rg --type rust -B 5 'DashSet::new\(\)'
Length of output: 1229
packages/rs-sdk/src/platform/fetch_many.rs (1)
255-255: LGTM! Verify implementations handle the address list correctly.
The change to use sdk.address_list() directly aligns with the PR objectives for improved address management at the SDK level. This allows for better control over address banning during retries.
Let's verify that all FetchMany implementations handle the address list correctly:
packages/rs-dapi-client/src/dapi_client.rs (5)
8-8: Import Arc for shared ownership
The addition of use std::sync::Arc; is necessary for managing shared ownership in the updated code.
75-75: Simplify address_list ownership by removing Arc<RwLock>
Changing the address_list field to own AddressList directly simplifies concurrency management, assuming AddressList internally handles synchronization.
89-89: Directly initialize address_list in the constructor
Assigning address_list directly in the new constructor aligns with the updated struct definition and ensures consistency.
98-98: Update address_list method to return a reference
Returning a reference to AddressList simplifies access and reflects the updated ownership model.
143-146: Directly retrieve live address without locking
The code correctly retrieves a live address from address_list using get_live_address(), leveraging internal concurrency mechanisms.
packages/rs-sdk/src/sync.rs (2)
193-261: Ensure thread safety when accessing AddressList in asynchronous context
The address_list is being accessed and modified inside an asynchronous closure to ban and unban addresses based on execution results. Verify that the AddressList implementation is thread-safe and that its methods (ban_address and unban_address) can be safely called concurrently without causing data races or synchronization issues.
267-277:
Review retry logic for correct calculation of remaining retries
In the when closure of the backon::Retryable::retry function, the calculation of all_requests_sent and the update of settings.retries may lead to potential inaccuracies:
- Ensure that
all_requests_sentaccurately reflects the total number of requests made, including retries. - Verify that
settings.retriesis correctly updated to prevent negative values, which could cause unexpected behavior. - Confirm that the condition
all_requests_sent <= max_retriesproperly guards against exceeding the maximum allowed retries.
packages/rs-sdk/src/sdk.rs (1)
557-560: LGTM: Simplifying address_list method by returning a reference
Changing the address_list method to return a reference to AddressList directly simplifies the API and eliminates unnecessary error handling. This aligns with the refactoring goals and may improve performance by avoiding cloning.
| match self.address_list.unban_address(&address) { | ||
| Ok(_) => { | ||
| tracing::debug!( | ||
| ?address, | ||
| "unban successfully responded address {}", | ||
| address | ||
| ); | ||
| } | ||
| // The address might be already removed from the list | ||
| // by background process (i.e., SML update), and it's fine. | ||
| Err(AddressListError::AddressNotFound(_)) => { | ||
| tracing::debug!( | ||
| ?address, | ||
| "unable to unban address {address}. it's not in the list" | ||
| ); | ||
| } | ||
| })?; | ||
| Err(AddressListError::InvalidAddressUri(_)) => { | ||
| unreachable!("unban address doesn't return InvalidAddressUri") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle InvalidAddressUri error explicitly in unban logic
Using unreachable!() for InvalidAddressUri may cause unintended panics. Even if this error is unexpected, it's safer to handle it explicitly to prevent potential crashes.
Recommend updating the error handling as follows:
Err(AddressListError::InvalidAddressUri(_)) => {
tracing::error!(
?address,
"Failed to unban address due to invalid URI"
);
// Handle the error appropriately, possibly returning an error or continuing
}
There was a problem hiding this comment.
Agree, you never know what someone will change in the other function in future
There was a problem hiding this comment.
@lklimek, glad we are in agreement. Handling the InvalidAddressUri error explicitly will enhance the robustness of the code and prevent potential panics.
There was a problem hiding this comment.
Ok, I changed to boolean
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| match self.address_list.ban_address(&address) { | ||
| Ok(_) => { | ||
| tracing::warn!( | ||
| ?address, | ||
| ?error, | ||
| "received server error, banning address {address}" | ||
| ); | ||
| } | ||
| // The address might be already removed from the list | ||
| // by background process (i.e., SML update), and it's fine. | ||
| Err(AddressListError::AddressNotFound(_)) => { | ||
| tracing::debug!( | ||
| ?address, | ||
| ?error, | ||
| "unable to unban address {address}. it's not in the list" | ||
| ); | ||
| } | ||
| })?; | ||
| Err(AddressListError::InvalidAddressUri(_)) => { | ||
| unreachable!( | ||
| "unban address doesn't return InvalidAddressUri" | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Correct log message and handle errors explicitly in ban logic
-
The log message in
Err(AddressListError::AddressNotFound(_))incorrectly states "unable to unban address" when it should be "unable to ban address". Please correct this to accurately reflect the operation. -
Similar to the unban logic, using
unreachable!()forInvalidAddressUrimay cause unexpected panics. Handle this error variant explicitly to improve robustness.
Apply the following changes:
// Correct the log message
- "unable to unban address {address}. it's not in the list"
+ "unable to ban address {address}. it's not in the list"
// Handle `InvalidAddressUri` appropriately
Err(AddressListError::InvalidAddressUri(_)) => {
tracing::error!(
?address,
"Failed to ban address due to invalid URI"
);
// Handle the error appropriately
}Committable suggestion skipped: line range outside the PR's diff.
| address_list: &AddressList, | ||
| settings: RequestSettings, | ||
| future_factory_fn: FutureFactoryFn, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update function documentation to include the address_list parameter
The retry function signature now includes the address_list: &AddressList parameter, but the function's documentation does not reflect this change. Please update the documentation to describe the address_list parameter and explain how it is used within the retry mechanism for banning and unbanning addresses.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/rs-sdk/tests/fetch/evonode.rs (1)
Line range hint
26-52: Consider adding test coverage for address banningSince the PR introduces address banning at the SDK level, consider adding test cases to verify:
- Address gets banned after failures
- Banned addresses are not retried
- Address unbanning works correctly
Would you like me to help draft these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/rs-sdk/tests/fetch/evonode.rs(2 hunks)
🔇 Additional comments (2)
packages/rs-sdk/tests/fetch/evonode.rs (2)
8-8: LGTM: Import aligns with new dereferencing usage
22-23: LGTM: Correctly adapted to new AddressList API
The changes appropriately handle the removal of into_iter by using iter() with explicit dereferencing.
Let's verify the removal of into_iter implementation:
✅ Verification successful
Verified: IntoIterator implementation has been removed from AddressList
The search results confirm that the IntoIterator implementation for AddressList has been removed. The struct now only implements FromIterator and provides iter() method, which aligns with the changes made in the test file. The adaptation in the test code is correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that into_iter is completely removed from AddressList
# Expected: No matches should be found
rg "impl.*IntoIterator.*for.*AddressList"
Length of output: 43
Script:
#!/bin/bash
# Let's try a broader search to find any IntoIterator implementations
rg "impl.*IntoIterator" -A 3
# Also search for AddressList struct definition and its implementations
rg "struct AddressList|impl.*AddressList" -A 3
Length of output: 11217
packages/rs-sdk/src/sync.rs
Outdated
| // Ban or unban the address based on the result | ||
| match &result { | ||
| Ok(response) => { | ||
| // Unban the address if it was banned and node responded successfully this time |
There was a problem hiding this comment.
Not sure if it will happen - it should be already unbanned by lower layer (but we can keep it here as a precaution)
packages/rs-sdk/src/sync.rs
Outdated
| let result = (*func)(*settings).await; | ||
|
|
||
| // Ban or unban the address based on the result | ||
| match &result { |
There was a problem hiding this comment.
Maybe this logic should be converted into a function shared between dapi-client and sdk, to not repeat similar code? not sure about that, just an idea
| match self.address_list.unban_address(&address) { | ||
| Ok(_) => { | ||
| tracing::debug!( | ||
| ?address, | ||
| "unban successfully responded address {}", | ||
| address | ||
| ); | ||
| } | ||
| // The address might be already removed from the list | ||
| // by background process (i.e., SML update), and it's fine. | ||
| Err(AddressListError::AddressNotFound(_)) => { | ||
| tracing::debug!( | ||
| ?address, | ||
| "unable to unban address {address}. it's not in the list" | ||
| ); | ||
| } | ||
| })?; | ||
| Err(AddressListError::InvalidAddressUri(_)) => { | ||
| unreachable!("unban address doesn't return InvalidAddressUri") | ||
| } | ||
| } |
There was a problem hiding this comment.
Agree, you never know what someone will change in the other function in future
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
packages/rs-dapi-client/src/lib.rs (1)
Line range hint
71-89: Consider enhancing error handling integration.Given the new address banning functionality, consider integrating it more tightly with the
CanRetrytrait. This could provide a more cohesive approach to handling retries and address banning.Consider:
- Adding a method to
CanRetrythat indicates if a failure should trigger address banning- Documenting the relationship between retries and address banning in the trait documentation
packages/rs-dapi-client/src/dapi_client.rs (2)
142-146: Enhance error logging with additional contextConsider adding more context to the error log by including the specific error type or category. This would help in debugging and monitoring banned addresses.
tracing::warn!( ?address, ?error, + error_type = %std::any::type_name::<E>(), "ban address {address} due to error: {error}" );
275-284: Consider simplifying execution result constructionThe success case mapping could be more concise by using struct initialization shorthand.
let execution_result = result .map(|inner| { tracing::trace!(response = ?inner, "received {} response", response_name); ExecutionResponse { - inner, - retries, - address: address.clone(), + inner, + retries, + address, } })packages/rs-sdk/src/sync.rs (1)
Line range hint
365-376: Add test coverage for address banning behaviorWhile the current tests verify retry counts, they don't explicitly verify that addresses are correctly banned and unbanned.
Consider adding test cases that verify:
- Addresses are banned after failures
- Banned addresses are not reused in subsequent retries
- Addresses are correctly unbanned on success
Example addition:
#[tokio::test] async fn test_address_banning() { let address_list = AddressList::default(); let test_uri = Uri::from_static("http://localhost").into(); // Verify address is banned after failure let result = retry(&address_list, RequestSettings::default(), |s| { async move { Err(ExecutionError { inner: MockError::Generic, retries: 0, address: Some(test_uri.clone()), }) } }).await; assert!(result.is_err()); assert!(address_list.is_banned(&test_uri)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/rs-dapi-client/src/dapi_client.rs(6 hunks)packages/rs-dapi-client/src/lib.rs(1 hunks)packages/rs-sdk/src/sync.rs(8 hunks)packages/rs-sdk/tests/fetch/evonode.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-sdk/tests/fetch/evonode.rs
🔇 Additional comments (6)
packages/rs-dapi-client/src/dapi_client.rs (3)
Line range hint 76-99: LGTM: Simplified ownership model
The removal of Arc<RwLock<AddressList>> in favor of direct AddressList ownership simplifies the code and aligns with the PR objective of eliminating RwLock for mixed async/sync code.
130-132: Handle InvalidAddressUri error explicitly
Using unreachable!() for InvalidAddressUri may cause unintended panics. Even if this error is unexpected, it's safer to handle it explicitly.
Also applies to: 157-159
295-299: LGTM: Well-integrated address banning
The integration of ban_failed_address is well-placed after execution but before returning the result, ensuring proper address management without affecting the execution flow.
packages/rs-sdk/src/sync.rs (3)
9-10: LGTM: Import changes align with new functionality
The new imports support the address banning feature and enhanced error handling capabilities.
160-167: Update function documentation to include the address_list parameter
The function signature has been updated but the documentation doesn't reflect the new parameter.
194-199: Verify address banning integration
The address banning logic is correctly placed after obtaining the execution result. Let's verify the integration with the address list management system.
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information about the address banning integration:
Address banning integration is properly implemented
The implementation correctly uses the centralized ban_failed_address function from rs-dapi-client, which:
- Is properly imported and used in the retry logic
- Integrates with the configurable settings system via
ban_failed_addressflag - Follows the consistent error handling pattern used elsewhere in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of ban_failed_address across the codebase
# to ensure consistent error handling patterns
# Check for other occurrences of ban_failed_address
rg -A 3 "ban_failed_address"
# Check for any custom address banning implementations that might conflict
ast-grep --pattern 'fn $name($params) {
$$$
ban($$$)
$$$
}'
Length of output: 3268
| @@ -17,7 +17,7 @@ pub use address_list::Address; | |||
| pub use address_list::AddressList; | |||
| pub use address_list::AddressListError; | |||
| pub use connection_pool::ConnectionPool; | |||
| pub use dapi_client::{DapiClient, DapiClientError}; | |||
| pub use dapi_client::{ban_failed_address, DapiClient, DapiClientError}; | |||
There was a problem hiding this comment.
💡 Codebase verification
Documentation is present but could be more descriptive
The ban_failed_address function has basic documentation, but it could benefit from more detailed documentation:
- The current doc comment is brief and doesn't explain the function's parameters
- The conditions under which addresses are banned/unbanned aren't clearly documented
- Return type and possible error cases aren't documented
Consider adding:
- Parameter descriptions using
@paramor equivalent - More detailed explanation of the banning/unbanning logic
- Examples of usage
- Documentation of any panics or error conditions
🔗 Analysis chain
Verify documentation for the newly exported function.
The new ban_failed_address function is being exported as part of the public API, but its documentation might be missing.
Let's verify the documentation and implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for documentation and implementation of ban_failed_address
# Check for documentation
rg -A 5 "ban_failed_address" packages/rs-dapi-client/src/dapi_client.rs
# Check if the function is properly tested
rg -g "*.rs" "ban_failed_address.*test" packages/rs-dapi-client/
Length of output: 845
Script:
#!/bin/bash
# Let's check for documentation comments (///), implementation, and test coverage
# Check for documentation and full implementation context
rg -B 5 -A 15 "pub fn ban_failed_address" packages/rs-dapi-client/src/dapi_client.rs
# Check for test modules
ast-grep --pattern 'mod $_ {
#[test]
$$$
fn $_ban_failed_address$_() {
$$$
}
$$$
}'
Length of output: 1009
| /// Ban address in case of retryable error or unban it | ||
| /// if it was banned, and the request was successful. | ||
| pub fn ban_failed_address<R, E>( | ||
| address_list: &AddressList, | ||
| result: &ExecutionResult<R, E>, | ||
| applied_settings: &AppliedRequestSettings, | ||
| ) where | ||
| E: CanRetry + Display + Debug, | ||
| { | ||
| match &result { | ||
| Ok(response) => { | ||
| // Unban the address if it was banned and node responded successfully this time | ||
| if response.address.is_banned() { | ||
| match address_list.unban_address(&response.address) { | ||
| Ok(_) => { | ||
| tracing::debug!(address = ?response.address, "unban successfully responded address {}", response.address); | ||
| } | ||
| // The address might be already removed from the list | ||
| // by background process (i.e., SML update), and it's fine. | ||
| Err(AddressListError::AddressNotFound(_)) => { | ||
| tracing::debug!( | ||
| address = ?response.address, | ||
| "unable to unban address {} because it's not in the list anymore", | ||
| response.address | ||
| ); | ||
| } | ||
| Err(AddressListError::InvalidAddressUri(_)) => { | ||
| unreachable!("unban address doesn't return InvalidAddressUri") | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Err(error) => { | ||
| if error.can_retry() { | ||
| if let Some(address) = error.address.as_ref() { | ||
| if applied_settings.ban_failed_address { | ||
| match address_list.ban_address(address) { | ||
| Ok(_) => { | ||
| tracing::warn!( | ||
| ?address, | ||
| ?error, | ||
| "ban address {address} due to error: {error}" | ||
| ); | ||
| } | ||
| // The address might be already removed from the list | ||
| // by background process (i.e., SML update), and it's fine. | ||
| Err(AddressListError::AddressNotFound(_)) => { | ||
| tracing::debug!( | ||
| ?address, | ||
| ?error, | ||
| "unable to ban address {address} because it's not in the list anymore" | ||
| ); | ||
| } | ||
| Err(AddressListError::InvalidAddressUri(_)) => { | ||
| unreachable!("ban address doesn't return InvalidAddressUri") | ||
| } | ||
| } | ||
| } else { | ||
| tracing::debug!( | ||
| ?error, | ||
| ?address, | ||
| "we should ban the address {address} due to the error but banning is disabled" | ||
| ); | ||
| } | ||
| } else { | ||
| tracing::debug!( | ||
| ?error, | ||
| "we should ban an address due to the error but address is absent" | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Test coverage missing for address banning functionality
The code inspection reveals that while the ban_address and unban_address methods are implemented in address_list.rs, and used in dapi_client.rs, there are no visible test cases covering this critical functionality. This includes:
- No unit tests for the
ban_failed_addressfunction - No tests verifying the address banning/unbanning behavior
- No integration tests covering this feature
🔗 Analysis chain
Verify test coverage for address banning
Let's verify that the new address banning functionality has proper test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases covering the new ban_failed_address function
rg -l "ban_failed_address.*test" --type rust
# Look for test functions that verify address banning behavior
ast-grep --pattern 'fn $test_name(_: $_) {
$$$
ban_failed_address($$$)
$$$
}'
Length of output: 132
Script:
#!/bin/bash
# Let's try a broader search for test files and ban-related tests
rg -l "mod tests" packages/rs-dapi-client/src/
# Search for any test functions that might be testing address banning
rg -A 10 "#\[test\]" packages/rs-dapi-client/src/
# Look for any usage of ban_address or unban_address in tests
rg "(ban_address|unban_address)" packages/rs-dapi-client/src/ --type rust
Length of output: 654
Script:
#!/bin/bash
# Let's check the address_list.rs file for tests
rg -A 20 "mod tests" packages/rs-dapi-client/src/address_list.rs
# Look for test files in the tests directory
fd "test" packages/rs-dapi-client/tests/
# Check for integration tests that might cover this functionality
rg -l "address.*ban" packages/rs-dapi-client/tests/
Length of output: 161
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
packages/rs-dapi-client/src/address_list.rs (5)
57-61: Add more detailed documentation for the uri method.While the method is straightforward, consider adding more context about when this method should be used and any important considerations when working with the Uri.
/// Get [Uri] of a node. + /// Returns a reference to the underlying Uri that can be used for making connections. + /// This Uri is guaranteed to be valid as it was validated during Address creation. pub fn uri(&self) -> &Uri { &self.0 }
66-69: Add documentation for struct fields.The struct fields would benefit from detailed documentation explaining their purpose and constraints.
pub struct AddressStatus { + /// Number of times this address has been banned + /// Used to calculate exponential backoff duration ban_count: usize, + /// Timestamp until which the address is banned + /// None if the address is not currently banned banned_until: Option<chrono::DateTime<Utc>>, }
Line range hint
73-78: Protect against potential overflow in ban duration calculation.The exponential calculation could lead to overflow for large ban counts. Consider adding a maximum ban duration.
pub fn ban(&mut self, base_ban_period: &Duration) { + const MAX_COEFFICIENT: f64 = 100.0; // Example maximum let coefficient = (self.ban_count as f64).exp(); + let coefficient = coefficient.min(MAX_COEFFICIENT); let ban_period = Duration::from_secs_f64(base_ban_period.as_secs_f64() * coefficient); self.banned_until = Some(chrono::Utc::now() + ban_period); self.ban_count += 1; }
107-108: Consider using parking_lot::RwLock for better performance.The standard library's RwLock can be less performant and more prone to writer starvation compared to parking_lot's implementation.
+use parking_lot::RwLock; -use std::sync::RwLock; pub struct AddressList { - addresses: Arc<RwLock<HashMap<Address, AddressStatus>>>, + addresses: Arc<RwLock<HashMap<Address, AddressStatus>>>, // Now using parking_lot::RwLock base_ban_period: Duration, }
253-253: Address the TODO comment about FromStr implementation.The comment indicates that the FromStr implementation needs to be changed. Consider creating a tracking issue for this work.
Would you like me to help create a GitHub issue to track this TODO item?
packages/rs-dapi-client/src/dapi_client.rs (1)
254-285: Consider extracting result mapping to a separate functionThe result mapping logic (lines 261-279) is quite complex and could benefit from being extracted into a separate function for better readability and maintainability.
Consider refactoring like this:
- let execution_result = result - .map(|inner| { - tracing::trace!(response = ?inner, "received {} response", response_name); - ExecutionResponse { - inner, - retries, - address: address.clone(), - } - }) - .map_err(|inner| { - tracing::debug!(error = ?inner, "received error: {inner}"); - ExecutionError { - inner, - retries, - address: Some(address.clone()), - } - }); + let execution_result = map_transport_result(result, retries, address.clone(), response_name); + fn map_transport_result<R>( + result: Result<R, TransportError>, + retries: usize, + address: Address, + response_name: &str, + ) -> ExecutionResult<R, DapiClientError> { + result + .map(|inner| { + tracing::trace!(response = ?inner, "received {} response", response_name); + ExecutionResponse { + inner, + retries, + address: address.clone(), + } + }) + .map_err(|inner| { + tracing::debug!(error = ?inner, "received error: {inner}"); + ExecutionError { + inner, + retries, + address: Some(address), + } + }) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/rs-dapi-client/Cargo.toml(1 hunks)packages/rs-dapi-client/src/address_list.rs(7 hunks)packages/rs-dapi-client/src/dapi_client.rs(6 hunks)packages/rs-dapi-client/src/lib.rs(1 hunks)scripts/configure_test_suite_network.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- scripts/configure_test_suite_network.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-dapi-client/Cargo.toml
- packages/rs-dapi-client/src/lib.rs
🔇 Additional comments (4)
packages/rs-dapi-client/src/address_list.rs (1)
Line range hint 1-240: Verify thread safety of the new implementation.
The changes introduce concurrent access patterns that should be thoroughly tested.
✅ Verification successful
Thread safety implementation is correct and well-designed
The implementation uses appropriate synchronization mechanisms:
- Uses
Arc<RwLock<HashMap>>for thread-safe concurrent access - Read operations (
get_live_address,is_banned) use read locks - Write operations (
ban,unban,add,remove) use write locks - No potential for deadlocks as locks are not held across async boundaries
- Lock granularity is appropriate with a single lock protecting the entire collection
The design follows Rust's safety guarantees and standard concurrent programming patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential deadlock scenarios in tests
rg -l "AddressList.*\.(ban|unban|get_live_address)" -t rust | xargs rg "tokio::(test|spawn)"
# Search for existing concurrent tests
rg -l "tokio::test.*AddressList" -t rust
Length of output: 6290
Script:
#!/bin/bash
# Let's search for tests specifically related to AddressList
rg -l "AddressList" -t rust | xargs rg "test.*AddressList" -B 2 -A 5
# Let's also check for any concurrent operations in the implementation
ast-grep --pattern 'impl AddressList {
$$$
async fn $$$($$$) {
$$$
}
$$$
}'
# Check for any test files containing AddressList
fd "test.*\.rs" | xargs rg "AddressList"
Length of output: 199
Script:
#!/bin/bash
# Let's check for unit tests in the same file or module
rg -t rust "mod\s+tests" -A 10 packages/rs-dapi-client/src/address_list.rs
# Let's also check how AddressList is used in the codebase
rg "AddressList::" -A 3 -B 1
# Check for any mutex or lock usage patterns
rg "RwLock|Mutex" -A 2 packages/rs-dapi-client/src/address_list.rs
Length of output: 3394
packages/rs-dapi-client/src/dapi_client.rs (3)
104-165: LGTM: Well-structured address banning implementation
The new ban_failed_address function is well-implemented with:
- Comprehensive error handling
- Appropriate logging levels
- Clear handling of edge cases (e.g., addresses removed by background processes)
However, there's a missing test coverage concern that was previously raised.
207-211: LGTM: Simplified address retrieval
The address retrieval has been simplified while maintaining proper error handling.
76-76: Breaking change: Simplified ownership model
The ownership model has been simplified by removing Arc<RwLock<>> wrapper from address_list. While this is a good improvement for mixed async/sync code, it's a breaking change that affects the public API.
Let's verify the impact of this breaking change:
Also applies to: 90-90, 99-99
| pub fn get_live_address(&self) -> Option<Address> { | ||
| let mut guard = self.addresses.read().unwrap(); | ||
|
|
||
| self.unbanned().into_iter().choose(&mut rng) | ||
| } | ||
| let mut rng = SmallRng::from_entropy(); | ||
|
|
||
| /// Get all addresses that are not banned. | ||
| fn unbanned(&self) -> Vec<&Address> { | ||
| let now = chrono::Utc::now(); | ||
|
|
||
| self.addresses | ||
| guard | ||
| .iter() | ||
| .filter(|addr| { | ||
| addr.banned_until | ||
| .filter(|(addr, status)| { | ||
| status | ||
| .banned_until | ||
| .map(|banned_until| banned_until < now) | ||
| .unwrap_or(true) | ||
| }) | ||
| .collect() | ||
| } | ||
|
|
||
| /// Get number of available, not banned addresses. | ||
| pub fn available(&self) -> usize { | ||
| self.unbanned().len() | ||
| .choose(&mut rng) | ||
| .map(|(addr, _)| addr.clone()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize get_live_address implementation.
The current implementation holds the read lock while performing random selection, which could block other operations unnecessarily. Consider collecting valid addresses first.
pub fn get_live_address(&self) -> Option<Address> {
- let mut guard = self.addresses.read().unwrap();
-
let mut rng = SmallRng::from_entropy();
let now = chrono::Utc::now();
- guard
- .iter()
- .filter(|(addr, status)| {
- status
- .banned_until
- .map(|banned_until| banned_until < now)
- .unwrap_or(true)
- })
- .choose(&mut rng)
- .map(|(addr, _)| addr.clone())
+ // Release the lock quickly by collecting valid addresses
+ let valid_addresses: Vec<Address> = {
+ let guard = self.addresses.read().unwrap();
+ guard
+ .iter()
+ .filter(|(_, status)| {
+ status
+ .banned_until
+ .map(|banned_until| banned_until < now)
+ .unwrap_or(true)
+ })
+ .map(|(addr, _)| addr.clone())
+ .collect()
+ };
+
+ valid_addresses.into_iter().choose(&mut rng)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn get_live_address(&self) -> Option<Address> { | |
| let mut guard = self.addresses.read().unwrap(); | |
| self.unbanned().into_iter().choose(&mut rng) | |
| } | |
| let mut rng = SmallRng::from_entropy(); | |
| /// Get all addresses that are not banned. | |
| fn unbanned(&self) -> Vec<&Address> { | |
| let now = chrono::Utc::now(); | |
| self.addresses | |
| guard | |
| .iter() | |
| .filter(|addr| { | |
| addr.banned_until | |
| .filter(|(addr, status)| { | |
| status | |
| .banned_until | |
| .map(|banned_until| banned_until < now) | |
| .unwrap_or(true) | |
| }) | |
| .collect() | |
| } | |
| /// Get number of available, not banned addresses. | |
| pub fn available(&self) -> usize { | |
| self.unbanned().len() | |
| .choose(&mut rng) | |
| .map(|(addr, _)| addr.clone()) | |
| pub fn get_live_address(&self) -> Option<Address> { | |
| let mut rng = SmallRng::from_entropy(); | |
| let now = chrono::Utc::now(); | |
| // Release the lock quickly by collecting valid addresses | |
| let valid_addresses: Vec<Address> = { | |
| let guard = self.addresses.read().unwrap(); | |
| guard | |
| .iter() | |
| .filter(|(_, status)| { | |
| status | |
| .banned_until | |
| .map(|banned_until| banned_until < now) | |
| .unwrap_or(true) | |
| }) | |
| .map(|(addr, _)| addr.clone()) | |
| .collect() | |
| }; | |
| valid_addresses.into_iter().choose(&mut rng) | |
| } |
There was a problem hiding this comment.
Do you think allocating new vector is much faster then selecting a random element from 100?
There was a problem hiding this comment.
@shumkov, you make a valid point. Allocating a new vector may introduce unnecessary overhead when dealing with a relatively small number of addresses (around 100). Holding the read lock during random selection in this context is likely acceptable. Thank you for the clarification.
✏️ Learnings added
Learnt from: shumkov
PR: dashpay/platform#2351
File: packages/rs-dapi-client/src/address_list.rs:208-224
Timestamp: 2024-11-28T14:07:48.516Z
Learning: When working with small collections (e.g., around 100 items), avoid introducing additional allocations if the performance benefit is negligible.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/rs-dapi-client/src/address_list.rs (1)
Line range hint
63-90: LGTM: Well-implemented ban management with exponential backoffThe AddressStatus struct provides a clean separation of concerns for ban management. The exponential backoff in the ban duration is a good practice for handling problematic nodes.
Consider adding documentation for the exponential coefficient calculation to explain why e^ban_count was chosen as the multiplier.
packages/rs-sdk/src/sdk.rs (1)
1021-1025: Consider allowing custom initial addresses in mock modeWhile the current implementation correctly initializes an empty address list, consider allowing custom initial addresses to be provided in mock mode for more flexible testing scenarios.
Consider modifying the builder to accept optional initial addresses:
inner: SdkInstance::Mock { mock: mock_sdk.clone(), dapi, - address_list: AddressList::new(), + address_list: self.mock_addresses.unwrap_or_else(AddressList::new), version: self.version, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/rs-dapi-client/src/address_list.rs(7 hunks)packages/rs-sdk/src/sdk.rs(3 hunks)packages/rs-sdk/tests/fetch/evonode.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-sdk/tests/fetch/evonode.rs
🧰 Additional context used
📓 Learnings (1)
packages/rs-dapi-client/src/address_list.rs (1)
Learnt from: shumkov
PR: dashpay/platform#2351
File: packages/rs-dapi-client/src/address_list.rs:241-250
Timestamp: 2024-11-28T14:08:53.428Z
Learning: In the `IntoIterator` implementation for `AddressList` in `packages/rs-dapi-client/src/address_list.rs`, since `self` is consumed, holding a write lock while converting to an iterator does not block other operations.
🔇 Additional comments (6)
packages/rs-dapi-client/src/address_list.rs (4)
Line range hint 20-61: LGTM: Clean Address struct implementation
The simplified Address struct with proper trait implementations and encapsulated URI access follows Rust best practices.
Line range hint 190-238: LGTM: Well-implemented address management methods
The methods provide a complete set of operations for address management. The get_live_address() implementation appropriately handles random selection while holding the read lock, which is suitable for the expected size of the address list.
241-250: LGTM: Correct IntoIterator implementation
The implementation properly consumes self and handles the transfer of ownership. As discussed in previous reviews, holding the write lock is acceptable here since self is consumed.
Line range hint 107-188: Verify RwLock usage against PR objectives
The PR objectives mention removing RwLock as it was "unsuitable for mixed async and sync code", but the implementation still uses RwLock. Please clarify if this is intentional or if further changes are planned.
The current implementation with Arc looks correct for concurrent access, and the Entry API usage is idiomatic. However, if async code needs to interact with this list, consider using tokio::sync::RwLock instead.
Run this script to check for async code usage:
packages/rs-sdk/src/sdk.rs (2)
187-187: LGTM: Addition of address_list field to Mock variant
The addition of the address_list field to SdkInstance::Mock provides consistent address list handling between mock and real implementations.
557-562: LGTM: Simplified address_list method signature
The method now returns a direct reference to AddressList instead of a Result, which simplifies the API while maintaining functionality. The implementation correctly handles both real and mock SDK instances.
| #[cfg_attr(feature = "mocks", serde(with = "http_serde::uri"))] | ||
| uri: Uri, | ||
| } | ||
| pub struct Address(#[cfg_attr(feature = "mocks", serde(with = "http_serde::uri"))] Uri); |
There was a problem hiding this comment.
Do we still need the Address struct? Maybe we can just type Address = Uri ?
There was a problem hiding this comment.
Better to have newtype so we can implement traits we need and extend in future without breaking changes
| address: Some(address.clone()), | ||
| } | ||
| })?; | ||
| let retries = retries_counter.load(std::sync::atomic::Ordering::Acquire); |
There was a problem hiding this comment.
This operation will execute in one thread anyway, so maybe we can use Relaxed here?
There was a problem hiding this comment.
Tokio runtime is using a pool of threads to execute tasks so they might be in different threads, ins't?
|
|
||
| let retries = retries_counter.load(std::sync::atomic::Ordering::Acquire); | ||
| ban_failed_address::<R::Response, DapiClientError>( |
There was a problem hiding this comment.
the name is a bit confusing - it looks like the address will be banned after this line. Maybe ban_address_if_failed ? or update_banned_status? Or sth?
There was a problem hiding this comment.
update_address_banned_status ?
Issue being fixed or feature implemented
We should ban failed nodes in case if SDK verification logic failed too. Currently, it's happening only on DAPI client (transport) level.
What was done?
AddressListto removeRwLockwhich doesn't fit greatly for mixed async and sync code.AddressList.availableAddressList.into_iterwhich shouldn't be a case.How Has This Been Tested?
With existing tests
Breaking Changes
AddressList.into_iterandAddressList.availableremovedChecklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
AddressList.DapiClientstructure for more straightforward address list handling.Bug Fixes
Documentation
Chores