fix(ffi): free transaction byte buffers correctly#749
fix(ffi): free transaction byte buffers correctly#749thepastaclaw wants to merge 1 commit intodashpay:v0.42-devfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR refactors FFI transaction buffer memory management to use a hidden length prefix. Transaction serialization now allocates a buffer prefixed with a ChangesFFI Transaction Buffer Memory Management
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@key-wallet-ffi/FFI_API.md`:
- Around line 1308-1311: The docs for the build-and-sign transaction function
refer to a parameter named `fee_rate` but the actual function signature uses
`fee_per_kb`; update all occurrences of `fee_rate` in the function description
and the Safety section to `fee_per_kb` (including the parameter list and
explanatory text) so the documentation matches the FFI signature, then
regenerate FFI_API.md to pick up the corrected source docs; reference the
documented function's parameter `fee_per_kb` and the Safety section entries for
`fee_rate` to locate and replace the mismatched names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32e6f687-4825-4656-87a3-c169bd9cad5c
📒 Files selected for processing (3)
key-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/transaction_tests.rs
| Build and sign a transaction using the wallet's managed info This is the recommended way to build transactions. It handles: - UTXO selection using coin selection algorithms - Fee calculation - Change address generation - Transaction signing # Safety - `manager` must be a valid pointer to an FFIWalletManager - `wallet` must be a valid pointer to an FFIWallet - `account_index` must be a valid BIP44 account index present in the wallet - `outputs` must be a valid pointer to an array of FFITxOutput with at least `outputs_count` elements - `fee_rate` must be a valid variant of FFIFeeRate - `fee_out` must be a valid, non-null pointer to a `u64`; on success it receives the calculated transaction fee in duffs - `tx_bytes_out` must be a valid pointer to store the transaction bytes pointer - `tx_len_out` must be a valid pointer to store the transaction length - `error` must be a valid pointer to an FFIError - On success, the returned transaction bytes must be freed by calling `transaction_bytes_free(*tx_bytes_out)`. | ||
|
|
||
| **Safety:** | ||
| - `manager` must be a valid pointer to an FFIWalletManager - `wallet` must be a valid pointer to an FFIWallet - `account_index` must be a valid BIP44 account index present in the wallet - `outputs` must be a valid pointer to an array of FFITxOutput with at least `outputs_count` elements - `fee_rate` must be a valid variant of FFIFeeRate - `fee_out` must be a valid, non-null pointer to a `u64`; on success it receives the calculated transaction fee in duffs - `tx_bytes_out` must be a valid pointer to store the transaction bytes pointer - `tx_len_out` must be a valid pointer to store the transaction length - `error` must be a valid pointer to an FFIError - The returned transaction bytes must be freed with `transaction_bytes_free` | ||
| - `manager` must be a valid pointer to an FFIWalletManager - `wallet` must be a valid pointer to an FFIWallet - `account_index` must be a valid BIP44 account index present in the wallet - `outputs` must be a valid pointer to an array of FFITxOutput with at least `outputs_count` elements - `fee_rate` must be a valid variant of FFIFeeRate - `fee_out` must be a valid, non-null pointer to a `u64`; on success it receives the calculated transaction fee in duffs - `tx_bytes_out` must be a valid pointer to store the transaction bytes pointer - `tx_len_out` must be a valid pointer to store the transaction length - `error` must be a valid pointer to an FFIError - On success, the returned transaction bytes must be freed by calling `transaction_bytes_free(*tx_bytes_out)`. |
There was a problem hiding this comment.
Fix parameter-name mismatch in docs (fee_rate vs fee_per_kb).
Line 1308 still documents fee_rate, but the function signature uses fee_per_kb. This can confuse FFI callers.
📝 Proposed source-doc fix (then regenerate `FFI_API.md`)
--- a/key-wallet-ffi/src/transaction.rs
+++ b/key-wallet-ffi/src/transaction.rs
@@
-/// - `fee_rate` must be a valid variant of FFIFeeRate
+/// - `fee_per_kb` is the requested fee rate in duffs per kilobyte🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@key-wallet-ffi/FFI_API.md` around lines 1308 - 1311, The docs for the
build-and-sign transaction function refer to a parameter named `fee_rate` but
the actual function signature uses `fee_per_kb`; update all occurrences of
`fee_rate` in the function description and the Safety section to `fee_per_kb`
(including the parameter list and explanatory text) so the documentation matches
the FFI signature, then regenerate FFI_API.md to pick up the corrected source
docs; reference the documented function's parameter `fee_per_kb` and the Safety
section entries for `fee_rate` to locate and replace the mismatched names.
There was a problem hiding this comment.
Addressed in a7d48fb: updated the source safety docs to use fee_per_kb and regenerated key-wallet-ffi/FFI_API.md.
There was a problem hiding this comment.
@thepastaclaw, thanks for the update! The fee_rate → fee_per_kb correction in the source docs and the regenerated FFI_API.md look good. The documentation now accurately matches the function signature.
🐇✨
(\(\
(•ᴗ•)
/づ♡
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #749 +/- ##
=============================================
+ Coverage 71.89% 71.91% +0.02%
=============================================
Files 320 320
Lines 69473 69465 -8
=============================================
+ Hits 49947 49957 +10
+ Misses 19526 19508 -18
|
Transaction-building FFI functions (`wallet_build_and_sign_transaction`, `wallet_build_and_sign_asset_lock_transaction`) hand back the serialized transaction as `Vec<u8>` -> `Box<[u8]>` -> `Box::into_raw(...) as *mut u8`. The previous `transaction_bytes_free(*mut u8)` then reconstructed `Box<u8>`, which is the wrong allocation layout for a boxed slice and is undefined behavior. Take the original length back from the caller and reconstruct the boxed slice via `Box::from_raw(slice_from_raw_parts_mut(ptr, len))`. Null remains a no-op. Update the safety docs on both builders and on `transaction_bytes_free` itself, refresh `FFI_API.md`, and add focused unit tests covering null free, freeing a `Box<[u8]>` with the matching length, and the empty-slice case. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Fix transaction byte buffer freeing
Summary
transaction_bytes_freeC ABI.function can reconstruct and drop the original boxed slice layout safely.
Validation
cargo test -p key-wallet-ffi transaction_bytes_free --libpython3 contrib/verify_ffi.pygit diff --checknpx markdownlint-cli2 /tmp/pr-body.md