bdk/src/wallet.rs (2)

224-230: Use real timestamp for unconfirmed tx application

Passing 0 loses useful recency info used by policies/UX. Use current UNIX time.

 pub fn apply_transaction(&self, tx_hex: String) -> Result<(), Error> {
     let mut wallet = self.get_wallet()?;
     let tx: Transaction = deserialize_hex(tx_hex.as_str()).context("deserialize tx")?;
-    wallet.apply_unconfirmed_txs(vec![(Arc::new(tx), 0)]);
+    let now = std::time::SystemTime::now()
+        .duration_since(std::time::UNIX_EPOCH)
+        .unwrap_or_default()
+        .as_secs();
+    wallet.apply_unconfirmed_txs(vec![(Arc::new(tx), now)]);
     self.persist(&mut wallet)?;
     Ok(())
 }

Please confirm apply_unconfirmed_txs expects seconds as u64 in your bdk_wallet version. Based on learnings

---

211-222: Use incremental sync in normal operation

full_scan() every sync is expensive; prefer sync() for steady-state and full_scan() only on restore/import.

If bdk_electrum::BdkElectrumClient::sync() is available, consider:

let request = { let wallet = self.get_wallet()?; wallet.start_sync() };
let update = self.backend.electrum.sync(request).context("sync")?;

Based on learnings

bdk/src/types.rs (2)

38-68: Consider deriving Clone for FFI records

Cloning these lightweight records often simplifies UniFFI consumers.

-#[derive(uniffi::Record)]
+#[derive(uniffi::Record, Clone)]
 pub struct Balance { ... }
 
-#[derive(uniffi::Record)]
+#[derive(uniffi::Record, Clone)]
 pub struct WalletTransaction { ... }
 
-#[derive(uniffi::Record)]
+#[derive(uniffi::Record, Clone)]
 pub struct WalletTransactionOutput { ... }
 
-#[derive(uniffi::Record)]
+#[derive(uniffi::Record, Clone)]
 pub struct WalletSendResult { ... }

---

3-19: Error wrapping is fine; optional: add From<PoisonError<_>>

You already map poisoned mutexes at call sites; adding a blanket From improves consistency.

impl<T> From<std::sync::PoisonError<T>> for Error {
    fn from(e: std::sync::PoisonError<T>) -> Self {
        Error::Generic(e.to_string())
    }
}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/onchain/wallet_test.go (1)

290-301: Remove or document commented-out test case.

The commented-out Xpub test case should either be removed if it's no longer needed, or documented with a comment explaining why it's disabled and when it might be re-enabled.

internal/onchain/bitcoin-wallet/wallet.go (5)

47-55: Ensure DB path portability and directory creation

Use filepath.Join and create DataDir before opening the SQLite file to avoid cross‑platform and runtime issues.

Apply this diff:

-	backend, err := bdk.NewBackend(
-		convertNetwork(cfg.Network),
-		url,
-		cfg.DataDir+"/bdk.sqlite",
-	)
+	// Ensure data dir exists and build a portable path
+	if err := os.MkdirAll(cfg.DataDir, 0o700); err != nil {
+		return nil, fmt.Errorf("ensure data dir: %w", err)
+	}
+	dbPath := filepath.Join(cfg.DataDir, "bdk.sqlite")
+	backend, err := bdk.NewBackend(
+		convertNetwork(cfg.Network),
+		url,
+		dbPath,
+	)

Add imports:

 import (
 	"errors"
 	"fmt"
 	"time"
 
 	"github.com/BoltzExchange/boltz-client/v2/internal/onchain"
 	"github.com/BoltzExchange/boltz-client/v2/internal/onchain/bitcoin-wallet/bdk"
 	"github.com/BoltzExchange/boltz-client/v2/pkg/boltz"
+	"os"
+	"path/filepath"
 )

---

71-88: Make read‑only explicit when no mnemonic

Currently sets Readonly=false when mnemonic provided, but not explicitly true when absent. Clarify state to avoid ambiguity.

 	if credentials.Mnemonic != "" {
 		creds.Mnemonic = &credentials.Mnemonic
 		info.Readonly = false
+	} else {
+		info.Readonly = true
 	}

---

90-92: Naming mismatch: returns an xpub, not a descriptor

Method is named DeriveDefaultDescriptor but calls DeriveDefaultXpub and returns an xpub. Consider renaming to DeriveDefaultXpub (or return a descriptor).

---

114-124: Apply bumped transaction to keep wallet state consistent

After broadcasting the bumped tx, also apply it locally (mirrors SendToAddress behavior).

 	newTxId, err := w.txProvider.BroadcastTransaction(txHex)
 	if err != nil {
 		return "", err
 	}
-	return newTxId, nil
+	if err := w.Wallet.ApplyTransaction(txHex); err != nil {
+		return "", err
+	}
+	return newTxId, nil

---

167-169: Free FFI resources on Disconnect

Invoke Destroy on the underlying BDK wallet for deterministic release instead of relying solely on finalizers.

 func (w *Wallet) Disconnect() error {
-	return nil
+	w.Wallet.Destroy()
+	return nil
 }

internal/onchain/bitcoin-wallet/bdk/bdk.go (1)

582-593: Optional: prefer io.ReadFull for exact reads

reader.Read is not guaranteed to fill the buffer. io.ReadFull avoids partial reads.

-	read_length, err := reader.Read(buffer)
-	if err != nil && err != io.EOF {
-		panic(err)
-	}
-	if read_length != int(length) {
-		panic(fmt.Errorf("bad read length when reading string, expected %d, read %d", length, read_length))
-	}
+	if _, err := io.ReadFull(reader, buffer); err != nil {
+		panic(err)
+	}

Note: This file is generated; consider updating the generator template instead.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/onchain/wallet_test.go (1)

290-301: Clean up commented-out test block

This has been flagged before. Either delete it or convert into a t.Run with t.Skip and a brief rationale.

bdk/src/wallet.rs (3)

213-224: Avoid holding wallet mutex across network I/O in sync()

Build the request under lock, drop it before full_scan, then re-lock to apply.

Apply:

 pub fn sync(&self) -> Result<(), Error> {
-    let mut wallet = self.get_wallet()?;
-    let request = wallet.start_full_scan();
-    let update = self
+    let request = {
+        let mut wallet = self.get_wallet()?;
+        wallet.start_full_scan()
+    };
+    let update = self
         .backend
         .electrum
         .full_scan(request, 20, 50, false)
         .context("full scan")?;
-    wallet.apply_update(update).context("apply update")?;
-    self.persist(&mut wallet)?;
+    let mut wallet = self.get_wallet()?;
+    wallet.apply_update(update).context("apply update")?;
+    self.persist(&mut wallet)?;
     Ok(())
 }

---

319-323: Pagination order bug (take before skip)

Call skip() before take() to page correctly.

Apply:

-    .transactions()
-    .take(limit as usize)
-    .skip(offset as usize)
+    .transactions()
+    .skip(offset as usize)
+    .take(limit as usize)

---

62-85: Wrong coin type in derivation path (mainnet-only path used for testnet/regtest)

Use BIP-84 coin type 1 for Testnet/Regtest.

Apply:

 fn derive_default_xpub(network: Network, mnemonic: String) -> Result<String, Error> {
     let secp = Secp256k1::new();
-    let derivation_path = DerivationPath::from_str("m/84h/0h/0h").context("derivation path")?;
+    let coin_type = match network {
+        Network::Bitcoin => 0u32,
+        Network::Testnet | Network::Regtest => 1u32,
+    };
+    let derivation_path = DerivationPath::from_str(
+        &format!("m/84h/{}h/0h", coin_type)
+    ).context("derivation path")?;

internal/onchain/bitcoin-wallet/wallet.go (2)

148-159: Verify: GetSendFee may have unintended side effects.

GetSendFee calls SendToAddress, which might build, sign, and potentially broadcast a transaction or reserve coins. Past review suggested using a dedicated estimate-only endpoint (e.g., dry-run via TxBuilder) to compute fees without side effects.

Confirm whether BDK's SendToAddress binding modifies wallet state (e.g., reserves UTXOs, persists pending tx) when called but not broadcast. If it does, consider implementing a dedicated fee estimation method using BDK's TxBuilder in dry-run mode.

---

56-67: Fix nil dereference in panic path.

If network is nil, the default branch will panic on nil dereference. Use the pointer directly in the error message.

Apply this diff:

 func convertNetwork(network *boltz.Network) bdk.Network {
 	switch network {
 	case boltz.MainNet:
 		return bdk.NetworkBitcoin
 	case boltz.TestNet:
 		return bdk.NetworkTestnet
 	case boltz.Regtest:
 		return bdk.NetworkRegtest
 	default:
-		panic(fmt.Sprintf("unexpected network %v", *network))
+		panic(fmt.Sprintf("unexpected network %v", network))
 	}
 }

internal/onchain/bitcoin-wallet/bdk/bdk.go (1)

1244-1252: Critical bug: FfiDestroyerError.Destroy type mismatch causes panic.

The switch matches ErrorGeneric by value (line 1246), but errors are stored as pointers (see line 1186: err: &ErrorGeneric{...}). This will always fall through to the panic in the default case.

Apply this diff:

 func (_ FfiDestroyerError) Destroy(value *Error) {
 	switch variantValue := value.err.(type) {
-	case ErrorGeneric:
-		variantValue.destroy()
+	case *ErrorGeneric:
+		variantValue.destroy()
 	default:
 		_ = variantValue
 		panic(fmt.Sprintf("invalid error value `%v` in FfiDestroyerError.Destroy", value))
 	}
 }

internal/onchain/wallet_test.go (2)

22-24: Currency coverage mismatch

walletTest iterates only Liquid, but comments imply both currencies. Either add BTC here or update the comment to avoid confusion.

Apply (if you want both):

- for _, currency := range []boltz.Currency{boltz.CurrencyLiquid} {
+ for _, currency := range []boltz.Currency{boltz.CurrencyBtc, boltz.CurrencyLiquid} {

Or adjust the comment at Line 80 accordingly.

Also applies to: 76-81

---

126-129: Remove debug print from assertions

fmt.Println in assertions adds noise to test output.

Apply:

-                            fmt.Println(o.Address, searchAddress)
                             return o.Address == searchAddress

Makefile (1)

114-119: Good fix on lib name; consider binding generation flow

Nice correction to check/copy libbdk.a. Optionally, wire bdk-bindings into build if bindings need regeneration during CI.

Example:

-build: download-gdk build-bolt12 build-lwk build-bdk
+build: download-gdk build-bolt12 build-lwk build-bdk bdk-bindings

bdk/Cargo.toml (1)

6-15: Correct version target: upgrade bdk_wallet to 2.1.0 (not 2.2.x)

The latest published version of bdk_wallet is 2.1.0, and the features ["keys-bip39","file_store","rusqlite","std"] are present and valid. Consider updating from 2.0.0 to 2.1.0. The overall MSRV for bdk_wallet is 1.63.0 and bdk_electrum requires MSRV 1.75.0, both of which are consistent with your setup.

internal/onchain/bitcoin-wallet/bdk/bdk.go (1)

1156-1156: Fix typo in comment.

"Convience" should be "Convenience".

-// Convience method to turn *Error into error
+// Convenience method to turn *Error into error

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/onchain/wallet_test.go (1)

290-301: Commented-out test block: either enable or convert to t.Skip with rationale.

Keeping dead code commented confuses maintenance. Turn it into a skipped test with a brief reason, or remove it.

bdk/src/wallet.rs (3)

62-85: Use testnet/regtest coin type (1) for default xpub derivation.

Currently hardcoded to mainnet coin type (0). Select coin type by network.

 fn derive_default_xpub(network: Network, mnemonic: String) -> Result<String, Error> {
     let secp = Secp256k1::new();
-    let derivation_path = DerivationPath::from_str("m/84h/0h/0h").context("derivation path")?;
+    let coin_type = match network {
+        Network::Bitcoin => 0,
+        Network::Testnet | Network::Regtest => 1,
+    };
+    let derivation_path = DerivationPath::from_str(&format!("m/84h/{}h/0h", coin_type))
+        .context("derivation path")?;

Based on learnings

---

213-224: Don’t hold wallet mutex across network I/O in sync().

Build the request under the lock, drop it, perform full_scan, then re-lock to apply/persist. Reduces contention.

-    let mut wallet = self.get_wallet()?;
-    let request = wallet.start_full_scan();
-    let update = self
+    let request = {
+        let mut wallet = self.get_wallet()?;
+        wallet.start_full_scan()
+    };
+    let update = self
         .backend
         .electrum
         .full_scan(request, 20, 50, false)
         .context("full scan")?;
-    wallet.apply_update(update).context("apply update")?;
-    self.persist(&mut wallet)?;
+    let mut wallet = self.get_wallet()?;
+    wallet.apply_update(update).context("apply update")?;
+    self.persist(&mut wallet)?;

Based on learnings

---

319-323: Fix pagination order (skip before take) and treat limit=0 as “no limit”.

Current take-then-skip yields wrong pages; limit=0 returns no items.

-    let transactions = wallet
-        .transactions()
-        .take(limit as usize)
-        .skip(offset as usize)
+    let effective_limit = if limit == 0 { usize::MAX } else { limit as usize };
+    let transactions = wallet
+        .transactions()
+        .skip(offset as usize)
+        .take(effective_limit)

internal/onchain/bitcoin-wallet/bdk/bdk.go (1)

1244-1252: Panic bug: wrong type in FfiDestroyerError.Destroy.

Errors are stored as pointers; matching by value misses and panics.

 func (_ FfiDestroyerError) Destroy(value *Error) {
+    if value == nil || value.err == nil {
+        return
+    }
     switch variantValue := value.err.(type) {
-    case ErrorGeneric:
-        variantValue.destroy()
+    case *ErrorGeneric:
+        variantValue.destroy()
     default:
         _ = variantValue
         panic(fmt.Sprintf("invalid error value `%v` in FfiDestroyerError.Destroy", value))
     }
 }

internal/onchain/bitcoin-wallet/wallet.go (1)

56-66: Fix nil deref in panic path.

Default branch dereferences a possibly nil pointer. Print the pointer instead.

-    default:
-        panic(fmt.Sprintf("unexpected network %v", *network))
+    default:
+        // avoid dereferencing nil in panic path
+        panic(fmt.Sprintf("unexpected network %v", network))

internal/test/test.go (1)

71-73: Don’t ignore DeriveDefaultXpub error.

If derivation fails, tests silently proceed with an empty descriptor. Fail fast.

-  if currency == boltz.CurrencyBtc {
-    creds.CoreDescriptor, _ = bdk.DeriveDefaultXpub(bdk.NetworkRegtest, WalletMnemonic)
-  }
+  if currency == boltz.CurrencyBtc {
+    desc, err := bdk.DeriveDefaultXpub(bdk.NetworkRegtest, WalletMnemonic)
+    if err != nil { panic("DeriveDefaultXpub failed: " + err.Error()) }
+    creds.CoreDescriptor = desc
+  }

internal/onchain/wallet_test.go (1)

244-251: Avoid fixed sleeps; wait until tx is in mempool.

Replace time.Sleep with require.Eventually to reduce flakes.

-    // Wait a bit to ensure transaction is in mempool
-    time.Sleep(2 * time.Second)
+    // Wait until wallet sees the unconfirmed tx
+    require.Eventually(t, func() bool {
+        _ = wallet.Sync()
+        txs, _ := wallet.GetTransactions(50, 0)
+        for _, tx := range txs { if tx.Id == txId && tx.BlockHeight == 0 { return true } }
+        return false
+    }, 10*time.Second, 250*time.Millisecond)

bdk/Cargo.toml (1)

17-19: Add an explicit bin target for uniffi-bindgen.

Helps “cargo run --bin uniffi-bindgen” workflows and some generators. Optional but nice.

 [lib]
 crate-type = ["staticlib", "cdylib", "rlib"]
 name = "bdk"
+
+[[bin]]
+name = "uniffi-bindgen"
+path = "uniffi-bindgen.rs"

Makefile (2)

63-65: Prefer passing the .so to uniffi-bindgen-go.

Dynamic lib tends to be more portable for metadata extraction than .a.

-bdk-bindings: build-bdk bindgen-go
-	cd $(BDK_BINDINGS_PATH) && uniffi-bindgen-go --out-dir ../internal/onchain/bitcoin-wallet/ --library ./target/release/libbdk.a
+bdk-bindings: build-bdk bindgen-go
+	cd $(BDK_BINDINGS_PATH) && uniffi-bindgen-go --out-dir ../internal/onchain/bitcoin-wallet/ --library ./target/release/libbdk.so

---

114-120: Avoid stale BDK artifacts: build unconditionally.

Guarded build won’t rebuild after source changes if libbdk.a exists. Cargo is incremental; let it decide.

-build-bdk:
-ifeq ("$(wildcard internal/onchain/bitcoin-wallet/bdk/libbdk.a)","")
-	@$(call print, "Building bdk")
-	cd $(BDK_BINDINGS_PATH) && cargo build --release --lib
-	cp $(BDK_BINDINGS_PATH)/target/release/libbdk.a $(BDK_BINDINGS_PATH)/target/release/libbdk.so internal/onchain/bitcoin-wallet/bdk/
-endif
+build-bdk:
+	@$(call print, "Building bdk")
+	cd $(BDK_BINDINGS_PATH) && cargo build --release --lib
+	cp $(BDK_BINDINGS_PATH)/target/release/libbdk.a $(BDK_BINDINGS_PATH)/target/release/libbdk.so internal/onchain/bitcoin-wallet/bdk/ || true

bdk/src/wallet.rs (1)

163-165: Round fee-rate conversion to avoid underpaying by truncation.

Use precise 250x conversion and round.

-fn parse_fee_rate(sat_per_vbyte: f64) -> FeeRate {
-    FeeRate::from_sat_per_kwu((sat_per_vbyte * 1000.0 / 4.0) as u64)
-}
+fn parse_fee_rate(sat_per_vbyte: f64) -> FeeRate {
+    let sp_kwu = (sat_per_vbyte * 250.0).round() as u64;
+    FeeRate::from_sat_per_kwu(sp_kwu)
+}

Based on learnings

bdk/src/types.rs (1)

47-54: Document field units/semantics for cross-language consumers.

Please add brief docs indicating:

• timestamp is Unix seconds.
• block_height is 0 (or similar) when unconfirmed.

This avoids subtle bugs in bindings and callers.

 #[derive(uniffi::Record)]
 pub struct WalletTransaction {
     pub id: String,
-    pub timestamp: u64,
+    /// Unix timestamp in seconds
+    pub timestamp: u64,
     pub outputs: Vec<WalletTransactionOutput>,
-    pub block_height: u32,
+    /// 0 if unconfirmed
+    pub block_height: u32,
     pub balance_change: i64,
     pub is_consolidation: bool,
 }

internal/onchain/bitcoin-wallet/wallet.go (2)

45-54: Avoid double scheme in Electrum URL.

If cfg.Electrum.Url already contains tcp:// or ssl://, this will produce tcp://tcp://... Consider normalizing.

-    url = cfg.Electrum.Url
-    if cfg.Electrum.SSL {
-        url = "ssl://" + url
-    } else {
-        url = "tcp://" + url
-    }
+    u := cfg.Electrum.Url
+    if !strings.HasPrefix(u, "tcp://") && !strings.HasPrefix(u, "ssl://") {
+        if cfg.Electrum.SSL {
+            u = "ssl://" + u
+        } else {
+            u = "tcp://" + u
+        }
+    }
+    url = u

(Add import "strings")

---

48-49: Use filepath.Join for portability.

String concat can be brittle across platforms.

-        cfg.DataDir+"/bdk.sqlite",
+        filepath.Join(cfg.DataDir, "bdk.sqlite"),

(Add import "path/filepath")

internal/onchain/bitcoin-wallet/bdk/bdk.go (2)

1256-1260: Add NetworkSignet to mirror Rust and avoid future mismatches.

Rust exposes a Signet variant; define the Go constant for parity even if unused today.

 const (
     NetworkBitcoin Network = 1
     NetworkTestnet Network = 2
     NetworkRegtest Network = 3
+    NetworkSignet  Network = 4
 )

---

67-69: Guard potential length overflow in ToGoBytes.

C.int(cb.inner.len) may overflow on 32‑bit targets. Add a bound check.

 func (cb GoRustBuffer) ToGoBytes() []byte {
-    return C.GoBytes(unsafe.Pointer(cb.inner.data), C.int(cb.inner.len))
+    if cb.inner.len > C.uint64_t(math.MaxInt32) {
+        panic("buffer too large for GoBytes")
+    }
+    return C.GoBytes(unsafe.Pointer(cb.inner.data), C.int(cb.inner.len))
 }

internal/onchain/bitcoin-wallet/bdk/bdk.h (2)

8-8: Guard stdbool include for C++/toolchains missing it.

Fixes portability and the reported clang error.

-#include <stdbool.h>
+#ifndef __cplusplus
+#  include <stdbool.h>
+#endif

---

3-5: Remove unprofessional autogenerated comment.

Please replace with a neutral “Autogenerated by UniFFI. Do not edit.” notice.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/onchain/wallet_test.go (2)

290-301: Address the commented-out Xpub/BTC test case.

This echoes michael1011's previous question: why is this test case commented out? If Xpub import for BTC is not yet supported, consider removing the commented code or adding a TODO comment explaining when it will be implemented. If it should work, please uncomment and verify.

---

224-255: Verify broadcast and txid semantics in BumpTransactionFee test.

The test variable is named newTxId but the backend may return raw transaction hex. Additionally, the test doesn't verify that:

1. The bump actually broadcasts (no Sync() call after bumping)
2. The replacement transaction exists in the mempool or wallet
3. The old transaction is no longer present

Consider the approach in internal/onchain/wallet/wallet_test.go (lines 29-51) which uses Eventually to poll until the bump succeeds, or follow the previous review suggestion to decode the hex if needed and sync to verify broadcast.

Apply this diff to improve the test:

 	// Try to bump the fee
-	newTxId, err := wallet.BumpTransactionFee(txId, 3)
-	require.NoError(t, err)
-	require.NotEmpty(t, newTxId)
-	require.NotEqual(t, txId, newTxId)
+	require.Eventually(t, func() bool {
+		newTxId, err := wallet.BumpTransactionFee(txId, 3)
+		if err != nil {
+			return false
+		}
+		require.NotEmpty(t, newTxId)
+		require.NotEqual(t, txId, newTxId)
+		
+		// Sync and verify the replacement is in the wallet
+		require.NoError(t, wallet.Sync())
+		transactions, err := wallet.GetTransactions(30, 0)
+		require.NoError(t, err)
+		
+		// Verify new tx exists and old tx is gone
+		hasNew := false
+		hasOld := false
+		for _, tx := range transactions {
+			if tx.Id == newTxId {
+				hasNew = true
+			}
+			if tx.Id == txId {
+				hasOld = true
+			}
+		}
+		return hasNew && !hasOld
+	}, 15*time.Second, 250*time.Millisecond)
 
 	test.MineBlock()

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/onchain/bitcoin-wallet/bdk/bdk.h (1)

3-4: Unprofessional banner remains in generated header.

The informal banner text is still present. Replace with a neutral autogeneration notice.

internal/onchain/bitcoin-wallet/wallet.go (2)

95-106: Nil dereference in panic path remains.

The default case dereferences the network pointer in the panic message, which will panic with a nil dereference if network is nil.

Apply this diff:

 	default:
-		panic(fmt.Sprintf("unexpected network %v", *network))
+		panic(fmt.Sprintf("unexpected network %v", network))

---

157-166: External-call hazard: ApplyTransaction failure loses broadcast txid.

If ApplyTransaction fails after a successful broadcast, returning "", err hides the txid and can trigger unsafe retries.

Return the txid even when ApplyTransaction fails:

 func (w *Wallet) broadcastTransaction(txHex string) (string, error) {
 	txId, err := w.backend.cfg.ChainProvider.BroadcastTransaction(txHex)
 	if err != nil {
 		return "", err
 	}
 	if err := w.Wallet.ApplyTransaction(txHex); err != nil {
-		return "", err
+		return txId, fmt.Errorf("broadcast succeeded but apply failed: %w", err)
 	}
 	return txId, nil
 }

bdk/src/wallet.rs (2)

65-65: Derivation path hardcoded to mainnet coin type.

The path m/84h/0h/0h always uses coin type 0 (mainnet). Testnet and Regtest should use coin type 1.

Apply this diff:

 fn derive_default_xpub(network: Network, mnemonic: String) -> Result<String, Error> {
   let secp = Secp256k1::new();
-  let derivation_path = DerivationPath::from_str("m/84h/0h/0h").context("derivation path")?;
+  let coin_type = match network {
+      Network::Bitcoin => 0,
+      Network::Testnet | Network::Regtest => 1,
+      Network::Signet => 1,
+  };
+  let derivation_path = DerivationPath::from_str(
+      &format!("m/84h/{}h/0h", coin_type)
+  ).context("derivation path")?;

---

312-320: Pagination order incorrect: take before skip.

Calling take(limit) before skip(offset) produces wrong pages.

Apply this diff:

     let transactions = wallet
         .transactions()
-        .take(if limit > 0 {
-            limit as usize
-        } else {
-            usize::MAX
-        })
         .skip(offset as usize)
+        .take(if limit > 0 {
+            limit as usize
+        } else {
+            usize::MAX
+        })

internal/onchain/bitcoin-wallet/bdk/bdk.go (3)

55-58: Missing overflow check in AsReader.

unsafe.Slice expects an int but receives uint64 without bounds checking. If cb.inner.len exceeds math.MaxInt, the cast will overflow.

Apply this diff:

 func (cb GoRustBuffer) AsReader() *bytes.Reader {
+	n := int(cb.inner.len)
+	if n < 0 || uint64(n) != uint64(cb.inner.len) {
+		panic("RustBuffer length overflows int")
+	}
-	b := unsafe.Slice((*byte)(cb.inner.data), C.uint64_t(cb.inner.len))
+	b := unsafe.Slice((*byte)(cb.inner.data), n)
 	return bytes.NewReader(b)
 }

---

67-69: Missing overflow check in ToGoBytes.

C.GoBytes expects C.int but receives cb.inner.len cast directly. If the length exceeds math.MaxInt32, the cast will overflow.

Apply this diff:

 func (cb GoRustBuffer) ToGoBytes() []byte {
+	n := int(cb.inner.len)
+	if n < 0 || uint64(n) != uint64(cb.inner.len) {
+		panic("RustBuffer length overflows int")
+	}
-	return C.GoBytes(unsafe.Pointer(cb.inner.data), C.int(cb.inner.len))
+	return C.GoBytes(unsafe.Pointer(cb.inner.data), C.int(n))
 }

---

1244-1252: Type switch matches by value instead of pointer.

The case ErrorGeneric matches by value, but errors are stored as *ErrorGeneric pointers. This will always fall through to the panic.

Apply this diff:

 func (_ FfiDestroyerError) Destroy(value *Error) {
 	switch variantValue := value.err.(type) {
-	case ErrorGeneric:
+	case *ErrorGeneric:
 		variantValue.destroy()
 	default:
 		_ = variantValue
 		panic(fmt.Sprintf("invalid error value `%v` in FfiDestroyerError.Destroy", value))
 	}
 }

bdk/src/wallet.rs (1)

152-154: Fee rate conversion may underpay due to truncation.

Manual conversion to sat/kwu truncates fractional sat/vB and may underpay.

Consider rounding up to preserve user intent:

 fn parse_fee_rate(sat_per_vbyte: f64) -> FeeRate {
-    FeeRate::from_sat_per_kwu((sat_per_vbyte * 1000.0 / 4.0) as u64)
+    // Round up to avoid underpaying
+    let sat_per_kwu = (sat_per_vbyte * 1000.0 / 4.0).ceil() as u64;
+    FeeRate::from_sat_per_kwu(sat_per_kwu)
 }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

bdk/src/wallet.rs (2)

207-217: Major: Wallet mutex held during network I/O blocks other operations.

Despite being marked as addressed in a previous review, the lock is still held across the full_scan network call (lines 208-215). This blocks all other wallet operations during synchronization, which can take several seconds.

Apply this diff to release the lock during network I/O:

 pub fn sync(&self, chain_client: Arc<ChainClient>) -> Result<(), Error> {
-    let mut wallet = self.get_wallet()?;
-    let request = wallet.start_full_scan();
-    let update = chain_client
+    let request = {
+        let mut wallet = self.get_wallet()?;
+        wallet.start_full_scan()
+    };
+    let update = chain_client
         .electrum
         .full_scan(request, 20, 50, false)
         .context("full scan")?;
+    let mut wallet = self.get_wallet()?;
     wallet.apply_update(update).context("apply update")?;
     self.persist(&mut wallet)?;
     Ok(())
 }

Based on learnings

---

62-85: Critical: Derivation path uses mainnet coin type for all networks.

Line 65 hardcodes m/84h/0h/0h, which always uses coin type 0 (mainnet). Per BIP44, testnet and regtest should use coin type 1. This was flagged in a previous review but remains unresolved.

Apply this diff to fix the coin-type selection:

 fn derive_default_xpub(network: Network, mnemonic: String) -> Result<String, Error> {
   let secp = Secp256k1::new();
-  let derivation_path = DerivationPath::from_str("m/84h/0h/0h").context("derivation path")?;
+  let coin_type = match network {
+      Network::Bitcoin => 0,
+      Network::Testnet | Network::Regtest => 1,
+  };
+  let derivation_path = DerivationPath::from_str(
+      &format!("m/84h/{}h/0h", coin_type)
+  ).context("derivation path")?;

bdk/src/wallet.rs (1)

152-154: Consider rounding up fractional fee rates to avoid underpaying.

The current cast to u64 truncates fractional sat/vB values. For example, 1.5 sat/vB becomes 375 sat/kwu (floored) instead of 376 (rounded up). While the math is correct, underpaying fees could cause transaction delays.

Apply this diff to round up:

 fn parse_fee_rate(sat_per_vbyte: f64) -> FeeRate {
-    FeeRate::from_sat_per_kwu((sat_per_vbyte * 1000.0 / 4.0) as u64)
+    FeeRate::from_sat_per_kwu((sat_per_vbyte * 1000.0 / 4.0).ceil() as u64)
 }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/onchain/wallet_test.go (1)

234-265: Address the concerns from previous reviews and improve test robustness.

This test still has the issues flagged in past review comments:

1. Line 258: The variable is named newTxId but the backend may return raw transaction hex rather than a txid. The test only checks newTxId != txId, which could pass trivially.
2. Line 254-255: Hard-coded sleep is fragile; prefer require.Eventually to poll mempool state.
3. Line 263: Mining a block without verifying that the bumped transaction was actually confirmed or that the original transaction was replaced.

Consider this more robust approach:

-	// Wait a bit to ensure transaction is in mempool
-	time.Sleep(2 * time.Second)
-
-	// Try to bump the fee
-	newTxId, err := wallet.BumpTransactionFee(txId, 3)
-	require.NoError(t, err)
-	require.NotEmpty(t, newTxId)
-	require.NotEqual(t, txId, newTxId)
-
-	test.MineBlock()
+	// Wait for transaction to appear in mempool
+	require.Eventually(t, func() bool {
+		require.NoError(t, wallet.Sync())
+		txs, _ := wallet.GetTransactions(0, 0)
+		return slices.ContainsFunc(txs, func(tx onchain.Transaction) bool {
+			return tx.Id == txId && tx.Confirmations == 0
+		})
+	}, 10*time.Second, 250*time.Millisecond)
+
+	// Bump the fee
+	newTxResult, err := wallet.BumpTransactionFee(txId, 3)
+	require.NoError(t, err)
+	require.NotEmpty(t, newTxResult)
+	
+	// If backend returns hex, decode to get txid; otherwise use as-is
+	// Verify the bumped transaction differs from original
+	require.NotEqual(t, txId, newTxResult)
+	
+	// Verify the bumped transaction is in mempool
+	require.Eventually(t, func() bool {
+		require.NoError(t, wallet.Sync())
+		txs, _ := wallet.GetTransactions(0, 0)
+		return slices.ContainsFunc(txs, func(tx onchain.Transaction) bool {
+			return tx.Id == newTxResult && tx.Confirmations == 0
+		})
+	}, 10*time.Second, 250*time.Millisecond)
+	
+	test.MineBlock()
+	
+	// Verify the bumped transaction is confirmed
+	require.Eventually(t, func() bool {
+		require.NoError(t, wallet.Sync())
+		txs, _ := wallet.GetTransactions(0, 0)
+		return slices.ContainsFunc(txs, func(tx onchain.Transaction) bool {
+			return tx.Id == newTxResult && tx.Confirmations > 0
+		})
+	}, 10*time.Second, 250*time.Millisecond)