suggestion(dash-spv-ffi): unused ffi functions removed from dash-spv-ffi crate #377
Conversation
📝 WalkthroughWalkthroughThe PR drastically shrinks the FFI public surface: ~25+ exported C functions, several FFI types/enums (FFIArray, FFIUnconfirmedTransaction, CoreSDKHandle, DashSpvValidationMode, etc.), memory-management helpers, broadcast/checkpoint APIs, and many tests are removed; remaining API centers on core client lifecycle and simplified config setters. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
I think it's a good plan. If we don't need/use it in there at the moment i agree there is no reason to keep it. Easy enough to bring it back and even improve on it if we actually need it and know how and where we actually want to use it.
ZocoLini
force-pushed
the
chore/ffi-cleanup-example
branch
2 times, most recently
from
abfa99e to
8714ec8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dash-spv-ffi/tests/unit/test_error_handling.rs (1)
109-133: Correct misleading comment at line 130 about "public API".The comment "Clear using public API" is incorrect—
clear_last_error()is an internal Rust function (no#[no_mangle]decorator) and is not exported to C callers.The actual design pattern is that errors are automatically cleared on successful FFI operations (confirmed by test assertions at lines 170 and 178). However, C callers have no way to manually clear the error state between failed operations, which the comment misrepresents. Update the comment to clarify that this tests the internal clearing mechanism, not a public API function:
// Clear using internal function (not public API) clear_last_error();dash-spv-ffi/src/client.rs (1)
537-571: Restore the client handle before early returns.If the initial
sync_progress()call fails, the function returns without puttingspv_clientback intoclient.inner, leaving the client in aNonestate and dropping the running instance.🐛 Proposed fix
- let start_height = match spv_client.sync_progress().await { - Ok(progress) => progress.header_height, - Err(e) => { - tracing::error!("Failed to get initial height: {}", e); - return Err(e); - } - }; + let start_height = match spv_client.sync_progress().await { + Ok(progress) => progress.header_height, + Err(e) => { + tracing::error!("Failed to get initial height: {}", e); + let mut guard = client.inner.lock().unwrap(); + *guard = Some(spv_client); + return Err(e); + } + };
🤖 Fix all issues with AI agents
In `@dash-spv-ffi/src/bin/ffi_cli.rs`:
- Around line 196-199: The loop's completion check can hang if filter sync is
disabled because filters_complete is computed unconditionally; update the
condition that sets filters_complete to only require filter heights when
filter_sync_available is true (e.g., compute filters_complete as
filter_sync_available && prog.filter_header_height >= prog.header_height &&
prog.last_synced_filter_height >= prog.filter_header_height), and then use that
guarded filters_complete together with SYNC_COMPLETED.load(Ordering::SeqCst) in
the if check so the loop can complete when filters are not available.
🧹 Nitpick comments (3)
dash-spv-ffi/src/types.rs (1)
45-52: Consider narrowing visibility of the internal string destructor.This helper is only used inside the crate, but it’s publicly exported with an FFI-style name. Making it
pub(crate)avoids suggesting a public API surface that was intentionally reduced.♻️ Proposed tweak
-pub unsafe fn dash_spv_ffi_string_destroy(s: FFIString) { +pub(crate) unsafe fn dash_spv_ffi_string_destroy(s: FFIString) { if !s.ptr.is_null() { let _ = CString::from_raw(s.ptr); } }dash-spv-ffi/tests/test_platform_integration_safety.rs (1)
1-8: Module docstring claims coverage that was removed.The module docstring at lines 3-8 claims to test "Memory safety (double-free, use-after-free)" but the
test_memory_safety_patternsfunction (lines 207-227) now only tests buffer overflow prevention. The use-after-free and double-free tests appear to have been removed as part of this cleanup.Consider updating the docstring to reflect the actual test coverage, or add a note that these scenarios are covered elsewhere.
📝 Suggested docstring update
//! Comprehensive safety tests for platform_integration FFI functions //! //! Tests focus on: //! - Null pointer handling //! - Buffer overflow prevention -//! - Memory safety (double-free, use-after-free) +//! - Memory safety (buffer overflow prevention) //! - Thread safety //! - Error propagationdash-spv-ffi/tests/unit/test_async_operations.rs (1)
243-282: Redundant innerunsafeblock after addingunsafeto function signature.Adding
unsafeto theextern "C"function signature is a good practice to document that it performs unsafe operations. However, the innerunsafeblock at line 248 is now redundant since the entire function body is already an unsafe context.♻️ Remove redundant inner unsafe block
unsafe extern "C" fn reentrant_callback( _success: bool, _error: *const c_char, user_data: *mut c_void, ) { - let data = unsafe { &*(user_data as *const ReentrantData) }; + let data = &*(user_data as *const ReentrantData); let count = data.count.fetch_add(1, Ordering::SeqCst);
| let headers_done = SYNC_COMPLETED.load(Ordering::SeqCst); | ||
| let filters_complete = if disable_filter_sync || !prog.filter_sync_available { | ||
| false | ||
| } else { | ||
| prog.filter_header_height >= prog.header_height | ||
| && prog.last_synced_filter_height >= prog.filter_header_height | ||
| }; | ||
| if headers_done && (filters_complete || disable_filter_sync) { | ||
| let filters_complete = prog.filter_header_height >= prog.header_height | ||
| && prog.last_synced_filter_height >= prog.filter_header_height; | ||
| if headers_done && filters_complete { |
There was a problem hiding this comment.
Prevent a potential hang when filter sync is unavailable.
If filter_sync_available is false, filter heights can stay at 0 while header_height advances, so filters_complete never becomes true and the polling loop can run forever. Gate the condition on filter_sync_available (or otherwise allow completion without filters).
🐛 Proposed fix
- let filters_complete = prog.filter_header_height >= prog.header_height
- && prog.last_synced_filter_height >= prog.filter_header_height;
+ let filters_complete = !prog.filter_sync_available
+ || (prog.filter_header_height >= prog.header_height
+ && prog.last_synced_filter_height >= prog.filter_header_height);📝 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.
| let headers_done = SYNC_COMPLETED.load(Ordering::SeqCst); | |
| let filters_complete = if disable_filter_sync || !prog.filter_sync_available { | |
| false | |
| } else { | |
| prog.filter_header_height >= prog.header_height | |
| && prog.last_synced_filter_height >= prog.filter_header_height | |
| }; | |
| if headers_done && (filters_complete || disable_filter_sync) { | |
| let filters_complete = prog.filter_header_height >= prog.header_height | |
| && prog.last_synced_filter_height >= prog.filter_header_height; | |
| if headers_done && filters_complete { | |
| let headers_done = SYNC_COMPLETED.load(Ordering::SeqCst); | |
| let filters_complete = !prog.filter_sync_available | |
| || (prog.filter_header_height >= prog.header_height | |
| && prog.last_synced_filter_height >= prog.filter_header_height); | |
| if headers_done && filters_complete { |
🤖 Prompt for AI Agents
In `@dash-spv-ffi/src/bin/ffi_cli.rs` around lines 196 - 199, The loop's
completion check can hang if filter sync is disabled because filters_complete is
computed unconditionally; update the condition that sets filters_complete to
only require filter heights when filter_sync_available is true (e.g., compute
filters_complete as filter_sync_available && prog.filter_header_height >=
prog.header_height && prog.last_synced_filter_height >=
prog.filter_header_height), and then use that guarded filters_complete together
with SYNC_COMPLETED.load(Ordering::SeqCst) in the if check so the loop can
complete when filters are not available.
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
bot
added
the
merge-conflict
ZocoLini
force-pushed
the
chore/ffi-cleanup-example
branch
from
8714ec8 to
c107192
Compare
github-actions
bot
removed
the
merge-conflict
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dash-spv-ffi/src/client.rs (1)
537-597: Missing required FFI attributes:#[no_mangle] extern "C"andunsafekeywordThe function lacks the required FFI annotations. All other FFI functions in client.rs use
#[no_mangle] pub unsafe extern "C", butclient_test_syncis defined aspub fnwithout these attributes. Additionally, the function signature is incorrect for FFI boundaries: it takes a safe reference&FFIDashSpvClientinstead of a raw pointer*mut FFIDashSpvClientlike other FFI functions, and it lacks theunsafekeyword required when crossing the FFI boundary.Per the coding guidelines for
dash-spv-ffi/src/**/*.rs: Use#[no_mangle] extern "C"attribute when implementing new FFI functions in Rust. The function needs:
#[no_mangle]attributeunsafekeyword- Change parameter from
&FFIDashSpvClientto*mut FFIDashSpvClient- Appropriate null-pointer checks and casting
dash-spv-ffi/FFI_API.md (1)
753-771: Type Definitions section has stale entries and incorrect enum variants.Several inconsistencies with the actual code:
FFIUnconfirmedTransaction- listed but removed per PR changesCoreSDKHandle- listed but related FFI functions were removedFFITransaction- listed but not found intypes.rsFFIMempoolStrategylists "Selective" variant which doesn't exist; code only hasFetchAllandBloomFilter📝 Suggested corrections
### Core Types - `FFIDashSpvClient` - SPV client handle - `FFIClientConfig` - Client configuration - `FFISyncProgress` - Synchronization progress - `FFIDetailedSyncProgress` - Detailed sync progress - `FFISpvStats` - SPV statistics -- `FFITransaction` - Transaction information -- `FFIUnconfirmedTransaction` - Unconfirmed transaction - `FFIEventCallbacks` - Event callback structure -- `CoreSDKHandle` - Platform SDK integration handle +- `FFIWalletManager` - Wallet manager handle ### Enumerations - `FFINetwork` - Network type (Dash, Testnet, Regtest, Devnet) - `FFIValidationMode` - Validation mode (None, Basic, Full) -- `FFIMempoolStrategy` - Mempool strategy (FetchAll, BloomFilter, Selective) +- `FFIMempoolStrategy` - Mempool strategy (FetchAll, BloomFilter) +- `FFIMempoolRemovalReason` - Mempool removal reason (Expired, Replaced, DoubleSpent, Confirmed, Manual) - `FFISyncStage` - Synchronization stage
🤖 Fix all issues with AI agents
In `@dash-spv-ffi/src/types.rs`:
- Around line 45-52: The destructor dash_spv_ffi_string_destroy is not exported
as a C ABI symbol, so C callers (e.g. receivers of
FFIDetailedSyncProgress::stage_message via
dash_spv_ffi_client_sync_to_tip_with_progress) cannot call it to free the
buffer; change its declaration to a public C ABI function by adding #[no_mangle]
pub extern "C" fn dash_spv_ffi_string_destroy(s: FFIString) { ... } while
preserving the existing safety doc and body (CString::from_raw(s.ptr) if not
null) so the function is callable from C to free the FFIString memory.
| /// # Safety | ||
| /// - `s.ptr` must be a pointer previously returned by `FFIString::new` or compatible. | ||
| /// - It must not be used after this call. | ||
| pub unsafe fn dash_spv_ffi_string_destroy(s: FFIString) { | ||
| if !s.ptr.is_null() { | ||
| let _ = CString::from_raw(s.ptr); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing #[no_mangle] extern "C" prevents C callers from freeing FFIString.
FFIString is still used in FFIDetailedSyncProgress::stage_message (line 130), which is passed to C callbacks via dash_spv_ffi_client_sync_to_tip_with_progress. Without a C-callable destructor, C consumers cannot free the string buffer, causing memory leaks.
Per coding guidelines, FFI types must have corresponding _destroy() functions with #[no_mangle] extern "C" attributes for explicit memory management.
🔧 Proposed fix to expose as C ABI
/// # Safety
/// - `s.ptr` must be a pointer previously returned by `FFIString::new` or compatible.
/// - It must not be used after this call.
+#[no_mangle]
+pub unsafe extern "C" fn dash_spv_ffi_string_destroy(s: FFIString) {
-pub unsafe fn dash_spv_ffi_string_destroy(s: FFIString) {
if !s.ptr.is_null() {
let _ = CString::from_raw(s.ptr);
}
}📝 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.
| /// # Safety | |
| /// - `s.ptr` must be a pointer previously returned by `FFIString::new` or compatible. | |
| /// - It must not be used after this call. | |
| pub unsafe fn dash_spv_ffi_string_destroy(s: FFIString) { | |
| if !s.ptr.is_null() { | |
| let _ = CString::from_raw(s.ptr); | |
| } | |
| } | |
| /// # Safety | |
| /// - `s.ptr` must be a pointer previously returned by `FFIString::new` or compatible. | |
| /// - It must not be used after this call. | |
| #[no_mangle] | |
| pub unsafe extern "C" fn dash_spv_ffi_string_destroy(s: FFIString) { | |
| if !s.ptr.is_null() { | |
| let _ = CString::from_raw(s.ptr); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@dash-spv-ffi/src/types.rs` around lines 45 - 52, The destructor
dash_spv_ffi_string_destroy is not exported as a C ABI symbol, so C callers
(e.g. receivers of FFIDetailedSyncProgress::stage_message via
dash_spv_ffi_client_sync_to_tip_with_progress) cannot call it to free the
buffer; change its declaration to a public C ABI function by adding #[no_mangle]
pub extern "C" fn dash_spv_ffi_string_destroy(s: FFIString) { ... } while
preserving the existing safety doc and body (CString::from_raw(s.ptr) if not
null) so the function is callable from C to free the FFIString memory.
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!
| /// # Safety | ||
| /// - `s.ptr` must be a pointer previously returned by `FFIString::new` or compatible. | ||
| /// - It must not be used after this call. | ||
| pub unsafe fn dash_spv_ffi_string_destroy(s: FFIString) { | ||
| if !s.ptr.is_null() { | ||
| let _ = CString::from_raw(s.ptr); | ||
| } | ||
| } |
| .help("Disable masternode list synchronization"), | ||
| ) | ||
| .arg( | ||
| Arg::new("no-filters") |
There was a problem hiding this comment.
Why do you want to remove this?
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
bot
added
the
merge-conflict
This are all the functions that are not being used in any place in platform repo, branch feat/iOSSupport along with structs or functions that ended up being unused after the removals. For sure there are more thing that are not being used in the crate but I wanted to focus on the ones not used when this is added as a dependency.
IMO, if we need something in a future to implement a feature though ffi we can recover it from this PR or re-implemente it with the API we had at that moment in time. Having this many things makes everything harder to work with by adding a lot fo noise and probably useless changes
Test updated removing references to the functions dropped. Discovered a LOT of stuff being tested more than once and tests that are basically useless IMO but didnt touch those, just making a note about things that can be improved
Summary by CodeRabbit
Documentation
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.