Skip to content

refactor(dash-spv): checkpoint rewrite #375

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ZocoLini wants to merge 1 commit into
base: v0.42-dev
Choose a base branch
from
+377 −917
Open

refactor(dash-spv): checkpoint rewrite #375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 2 additions & 16 deletions dash-spv-ffi/FFI_API.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ This document provides a comprehensive reference for all FFI (Foreign Function I

**Auto-generated**: This documentation is automatically generated from the source code. Do not edit manually.

**Total Functions**: 66
**Total Functions**: 65

## Table of Contents

Expand Down Expand Up @@ -125,15 +125,14 @@ Functions: 2

### Utility Functions

Functions: 19
Functions: 18

| Function | Description | Module |
|----------|-------------|--------|
| `dash_spv_ffi_array_destroy` | No description | types |
| `dash_spv_ffi_checkpoint_before_height` | Get the last checkpoint at or before a given height | checkpoints |
| `dash_spv_ffi_checkpoint_before_timestamp` | Get the last checkpoint at or before a given UNIX timestamp (seconds) | checkpoints |
| `dash_spv_ffi_checkpoint_latest` | Get the latest checkpoint for the given network | checkpoints |
| `dash_spv_ffi_checkpoints_between_heights` | Get all checkpoints between two heights (inclusive) | checkpoints |
| `dash_spv_ffi_client_clear_storage` | Clear all persisted SPV storage (headers, filters, metadata, sync state) | client |
| `dash_spv_ffi_client_get_stats` | Get current runtime statistics for the SPV client | client |
| `dash_spv_ffi_client_get_tip_hash` | Get the current chain tip hash (32 bytes) if available | client |
Expand Down Expand Up @@ -949,19 +948,6 @@ Get the latest checkpoint for the given network. # Safety - `out_height` must b

---

#### `dash_spv_ffi_checkpoints_between_heights`

```c
dash_spv_ffi_checkpoints_between_heights(network: FFINetwork, start_height: u32, end_height: u32,) -> FFIArray
```

**Description:**
Get all checkpoints between two heights (inclusive). Returns an `FFIArray` of `FFICheckpoint` items. The caller owns the memory and must free the array buffer using `dash_spv_ffi_array_destroy` when done.

**Module:** `checkpoints`

---

#### `dash_spv_ffi_client_clear_storage`

```c
Expand Down
50 changes: 19 additions & 31 deletions dash-spv-ffi/include/dash_spv_ffi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,25 +42,6 @@ typedef enum FFIMempoolStrategy {

typedef struct FFIDashSpvClient FFIDashSpvClient;

/**
* FFI-safe array that transfers ownership of memory to the C caller.
*
* # Safety
*
* This struct represents memory that has been allocated by Rust but ownership
* has been transferred to the C caller. The caller is responsible for:
* - Not accessing the memory after it has been freed
* - Calling `dash_spv_ffi_array_destroy` to properly deallocate the memory
* - Ensuring the data, len, and capacity fields remain consistent
*/
typedef struct FFIArray {
void *data;
uintptr_t len;
uintptr_t capacity;
uintptr_t elem_size;
uintptr_t elem_align;
} FFIArray;

typedef struct FFIClientConfig {
void *inner;
uint32_t worker_threads;
Expand Down Expand Up @@ -186,6 +167,25 @@ typedef struct FFIResult {
const char *error_message;
} FFIResult;

/**
* FFI-safe array that transfers ownership of memory to the C caller.
*
* # Safety
*
* This struct represents memory that has been allocated by Rust but ownership
* has been transferred to the C caller. The caller is responsible for:
* - Not accessing the memory after it has been freed
* - Calling `dash_spv_ffi_array_destroy` to properly deallocate the memory
* - Ensuring the data, len, and capacity fields remain consistent
*/
typedef struct FFIArray {
void *data;
uintptr_t len;
uintptr_t capacity;
uintptr_t elem_size;
uintptr_t elem_align;
} FFIArray;

/**
* FFI-safe representation of an unconfirmed transaction
*
Expand Down Expand Up @@ -264,18 +264,6 @@ int32_t dash_spv_ffi_checkpoint_before_timestamp(FFINetwork network,
uint8_t *out_hash)
;

/**
* Get all checkpoints between two heights (inclusive).
*
* Returns an `FFIArray` of `FFICheckpoint` items. The caller owns the memory and
* must free the array buffer using `dash_spv_ffi_array_destroy` when done.
*/

struct FFIArray dash_spv_ffi_checkpoints_between_heights(FFINetwork network,
uint32_t start_height,
uint32_t end_height)
;

/**
* Create a new SPV client and return an opaque pointer.
*
Expand Down
121 changes: 18 additions & 103 deletions dash-spv-ffi/src/checkpoints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use crate::{set_last_error, FFIArray, FFIErrorCode};
use dash_spv::chain::checkpoints::{mainnet_checkpoints, testnet_checkpoints, CheckpointManager};
use crate::{set_last_error, FFIErrorCode};
use dash_spv::chain::CheckpointManager;
use dashcore::hashes::Hash;
use dashcore::Network;
use key_wallet_ffi::FFINetwork;

/// FFI representation of a checkpoint (height + block hash)
Expand All @@ -11,15 +10,6 @@ pub struct FFICheckpoint {
pub block_hash: [u8; 32],
}

fn manager_for_network(network: FFINetwork) -> Result<CheckpointManager, String> {
let net: Network = network.into();
match net {
Network::Dash => Ok(CheckpointManager::new(mainnet_checkpoints())),
Network::Testnet => Ok(CheckpointManager::new(testnet_checkpoints())),
_ => Err("Checkpoints are only available for Dash and Testnet".to_string()),
}
}

/// Get the latest checkpoint for the given network.
///
/// # Safety
Expand All @@ -31,26 +21,7 @@ pub unsafe extern "C" fn dash_spv_ffi_checkpoint_latest(
out_height: *mut u32,
out_hash: *mut u8, // expects at least 32 bytes
) -> i32 {
if out_height.is_null() || out_hash.is_null() {
set_last_error("Null output pointer provided");
return FFIErrorCode::NullPointer as i32;
}
let mgr = match manager_for_network(network) {
Ok(m) => m,
Err(e) => {
set_last_error(&e);
return FFIErrorCode::InvalidArgument as i32;
}
};
if let Some(cp) = mgr.last_checkpoint() {
*out_height = cp.height;
let hash = cp.block_hash.to_byte_array();
std::ptr::copy_nonoverlapping(hash.as_ptr(), out_hash, 32);
FFIErrorCode::Success as i32
} else {
set_last_error("No checkpoints available for network");
FFIErrorCode::NotImplemented as i32
}
dash_spv_ffi_checkpoint_before_height(network, u32::MAX, out_height, out_hash)
}

/// Get the last checkpoint at or before a given height.
Expand All @@ -69,22 +40,14 @@ pub unsafe extern "C" fn dash_spv_ffi_checkpoint_before_height(
set_last_error("Null output pointer provided");
return FFIErrorCode::NullPointer as i32;
}
let mgr = match manager_for_network(network) {
Ok(m) => m,
Err(e) => {
set_last_error(&e);
return FFIErrorCode::InvalidArgument as i32;
}
};
if let Some(cp) = mgr.last_checkpoint_before_height(height) {
*out_height = cp.height;
let hash = cp.block_hash.to_byte_array();
std::ptr::copy_nonoverlapping(hash.as_ptr(), out_hash, 32);
FFIErrorCode::Success as i32
} else {
set_last_error("No checkpoint at or before given height");
FFIErrorCode::ValidationError as i32
}

let mgr = CheckpointManager::new(network.into());

let (height, cp) = mgr.last_checkpoint_before_height(height);
*out_height = height;
let hash = cp.hash().to_byte_array();
std::ptr::copy_nonoverlapping(hash.as_ptr(), out_hash, 32);
FFIErrorCode::Success as i32
Comment on lines +44 to +50
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# First, find the relevant files
echo "=== Locating checkpoint-related files ==="
fd -t f "checkpoints\.rs" --type f

echo -e "\n=== Checking dash-spv-ffi/src/checkpoints.rs existence ==="
test -f dash-spv-ffi/src/checkpoints.rs && echo "File exists" || echo "File not found"

echo -e "\n=== Checking dash-spv/src/chain/checkpoints.rs existence ==="
test -f dash-spv/src/chain/checkpoints.rs && echo "File exists" || echo "File not found"

Repository: dashpay/rust-dashcore

Length of output: 318

🏁 Script executed:

#!/bin/bash

# Read the FFI checkpoints.rs file to see the context around lines 44-50 and 70-76
echo "=== FFI checkpoints.rs content ==="
if [ -f dash-spv-ffi/src/checkpoints.rs ]; then
    wc -l dash-spv-ffi/src/checkpoints.rs
    echo ""
    cat -n dash-spv-ffi/src/checkpoints.rs | head -100
fi

Repository: dashpay/rust-dashcore

Length of output: 3152

🏁 Script executed:

#!/bin/bash

# Search for CheckpointManager::new and unreachable! in the dash-spv directory
echo "=== Searching for CheckpointManager in dash-spv ==="
rg -n "CheckpointManager|unreachable!" dash-spv/src/ -A 3 -B 1

Repository: dashpay/rust-dashcore

Length of output: 6924

🏁 Script executed:

#!/bin/bash

# Read the full CheckpointManager::new implementation
echo "=== CheckpointManager::new implementation ==="
sed -n '92,120p' dash-spv/src/chain/checkpoints.rs | cat -n

Repository: dashpay/rust-dashcore

Length of output: 1251

🏁 Script executed:

#!/bin/bash

# Check for other potential panic paths in CheckpointManager methods
echo "=== Methods called from FFI (last_checkpoint_before_height/timestamp) ==="
sed -n '130,210p' dash-spv/src/chain/checkpoints.rs | cat -n

Repository: dashpay/rust-dashcore

Length of output: 3552

🏁 Script executed:

#!/bin/bash

# Verify the exact context and confirm there's no error handling pathway
echo "=== Checking FFI error handling for CheckpointManager::new ==="
sed -n '33,51p' dash-spv-ffi/src/checkpoints.rs | cat -n

echo -e "\n=== Second FFI function ==="
sed -n '59,77p' dash-spv-ffi/src/checkpoints.rs | cat -n

Repository: dashpay/rust-dashcore

Length of output: 1728

🏁 Script executed:

#!/bin/bash

# Check if CheckpointManager provides a fallible constructor alternative
echo "=== All public CheckpointManager methods ==="
rg -n "pub.*fn" dash-spv/src/chain/checkpoints.rs | grep -i "checkpoint"

Repository: dashpay/rust-dashcore

Length of output: 287

Prevent panics from crossing the FFI boundary.

CheckpointManager::new uses unreachable! when checkpoint decoding fails (dash-spv/src/chain/checkpoints.rs line 111). While the bytes are hardcoded, panics in library code will unwind across the extern "C" boundary and cause undefined behavior. Wrap the constructor call with catch_unwind to map panics to FFIErrorCode via set_last_error, or refactor CheckpointManager to support fallible construction.

This applies to both calls at lines 44 and 70 in this file.

🤖 Prompt for AI Agents 
In `@dash-spv-ffi/src/checkpoints.rs` around lines 44 - 50, Wrap calls to
CheckpointManager::new in catch_unwind in the FFI functions so panics from
checkpoint decoding cannot unwind across the extern "C" boundary; if
catch_unwind catches a panic, call set_last_error with a descriptive message and
return a non-success FFIErrorCode, otherwise proceed to call methods like
CheckpointManager::last_checkpoint_before_height and return
FFIErrorCode::Success as before. Apply this change to both places where
CheckpointManager::new is invoked in this file (the two constructor call sites
that then call last_checkpoint_before_height and the later use at the other call
site), mapping panic results to an error code via set_last_error instead of
letting the panic propagate. Ensure you preserve the existing success path
behavior (setting out_height/out_hash) when no panic occurs.

}

/// Get the last checkpoint at or before a given UNIX timestamp (seconds).
Expand All @@ -103,60 +66,12 @@ pub unsafe extern "C" fn dash_spv_ffi_checkpoint_before_timestamp(
set_last_error("Null output pointer provided");
return FFIErrorCode::NullPointer as i32;
}
let mgr = match manager_for_network(network) {
Ok(m) => m,
Err(e) => {
set_last_error(&e);
return FFIErrorCode::InvalidArgument as i32;
}
};
if let Some(cp) = mgr.last_checkpoint_before_timestamp(timestamp) {
*out_height = cp.height;
let hash = cp.block_hash.to_byte_array();
std::ptr::copy_nonoverlapping(hash.as_ptr(), out_hash, 32);
FFIErrorCode::Success as i32
} else {
set_last_error("No checkpoint at or before given timestamp");
FFIErrorCode::ValidationError as i32
}
}

/// Get all checkpoints between two heights (inclusive).
///
/// Returns an `FFIArray` of `FFICheckpoint` items. The caller owns the memory and
/// must free the array buffer using `dash_spv_ffi_array_destroy` when done.
#[no_mangle]
pub extern "C" fn dash_spv_ffi_checkpoints_between_heights(
network: FFINetwork,
start_height: u32,
end_height: u32,
) -> FFIArray {
match manager_for_network(network) {
Ok(mgr) => {
// Collect checkpoints within inclusive range
let mut out: Vec<FFICheckpoint> = Vec::new();
for &h in mgr.checkpoint_heights() {
if h >= start_height && h <= end_height {
if let Some(cp) = mgr.get_checkpoint(h) {
out.push(FFICheckpoint {
height: cp.height,
block_hash: cp.block_hash.to_byte_array(),
});
}
}
}
FFIArray::new(out)
}
Err(e) => {
set_last_error(&e);
// Return empty array on error
FFIArray {
data: std::ptr::null_mut(),
len: 0,
capacity: 0,
elem_size: std::mem::size_of::<FFICheckpoint>(),
elem_align: std::mem::align_of::<FFICheckpoint>(),
}
}
}
let mgr = CheckpointManager::new(network.into());

let (height, cp) = mgr.last_checkpoint_before_timestamp(timestamp);
*out_height = height;
let hash = cp.hash().to_byte_array();
std::ptr::copy_nonoverlapping(hash.as_ptr(), out_hash, 32);
FFIErrorCode::Success as i32
}
Binary file added dash-spv/checkpoints/mainnet.checkpoints
View file
Open in desktop
Binary file not shown.
Binary file added dash-spv/checkpoints/testnet.checkpoints
View file
Open in desktop
Binary file not shown.
Loading