fix(ffi): free transaction byte buffers correctly by thepastaclaw · Pull Request #749 · dashpay/rust-dashcore

thepastaclaw · 2026-05-09T02:10:38Z

Fix transaction byte buffer freeing

Summary

Preserve the existing one-argument transaction_bytes_free C ABI.
Store the returned transaction byte length in a hidden prefix so the Rust free
function can reconstruct and drop the original boxed slice layout safely.
Route both transaction byte producers through the same allocation helper.
Add unit coverage for null, empty, and non-empty buffers.

Validation

cargo test -p key-wallet-ffi transaction_bytes_free --lib
python3 contrib/verify_ffi.py
git diff --check
npx markdownlint-cli2 /tmp/pr-body.md
Code review gate passed with recommendation: ship.

Summary by CodeRabbit

Release Notes

Documentation
- Updated FFI transaction buffer documentation with explicit deallocation guidance.
Refactor
- Improved transaction buffer memory layout and deallocation handling.
Tests
- Added transaction buffer lifecycle tests for safety verification.

coderabbitai · 2026-05-09T02:10:45Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d3cd6cb0-4031-4187-8fb3-8ea21927411b

📥 Commits

Reviewing files that changed from the base of the PR and between e9296b2 and a7d48fb.

📒 Files selected for processing (3)

key-wallet-ffi/FFI_API.md
key-wallet-ffi/src/transaction.rs
key-wallet-ffi/src/transaction_tests.rs

🚧 Files skipped from review as they are similar to previous changes (3)

key-wallet-ffi/src/transaction_tests.rs
key-wallet-ffi/FFI_API.md
key-wallet-ffi/src/transaction.rs

📝 Walkthrough

Walkthrough

This PR refactors FFI transaction buffer memory management to use a hidden length prefix. Transaction serialization now allocates a buffer prefixed with a usize length field, returns a pointer past the prefix to callers, and deallocates by reading the stored prefix to reconstruct the full allocation. Documentation and tests validate the new contract.

Changes

FFI Transaction Buffer Memory Management

Layer / File(s)	Summary
Buffer Layout & Allocation `key-wallet-ffi/src/transaction.rs`	New `TRANSACTION_BYTES_LEN_PREFIX_SIZE` constant and `transaction_bytes_into_ffi_buffer` helper allocate a buffer with a hidden `usize` length prefix and return a pointer to the transaction bytes region.
Memory Deallocation `key-wallet-ffi/src/transaction.rs`	`transaction_bytes_free` is updated to read the length prefix from memory before the caller-provided pointer, reconstruct the original boxed allocation, and safely deallocate.
Transaction Build Integration `key-wallet-ffi/src/transaction.rs`	Both `wallet_build_and_sign_transaction` and `wallet_build_and_sign_asset_lock_transaction` are updated to use `transaction_bytes_into_ffi_buffer` for serialization and output, with Rust documentation clarifying the `transaction_bytes_free` freeing requirement. Test module wiring is added.
FFI API Documentation `key-wallet-ffi/FFI_API.md`	Documentation for `transaction_bytes_free` is refined to specify pointer validity constraints, and both build functions' documentation is expanded to explicitly state the freeing requirement via `transaction_bytes_free(*tx_bytes_out)`.
Buffer Lifecycle Tests `key-wallet-ffi/src/transaction_tests.rs`	New test module verifies null-pointer safety, successful deallocation of prefixed buffers, and correct length semantics for empty and non-empty payloads.

🎯 3 (Moderate) | ⏱️ ~25 minutes

A rabbit hops through memory,
prefix in hand, pointer unbent,
freed without a double-take—
allocation dreams, fully spent. 🐰✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: fixing memory safety by properly freeing transaction byte buffers in the FFI module. It is concise, specific, and clearly related to the primary objective of the PR.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

thepastaclaw · 2026-05-09T02:12:04Z

@coderabbitai review

coderabbitai · 2026-05-09T02:12:10Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

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

📥 Commits

Reviewing files that changed from the base of the PR and between 34f574d and e9296b2.

📒 Files selected for processing (3)

key-wallet-ffi/FFI_API.md
key-wallet-ffi/src/transaction.rs
key-wallet-ffi/src/transaction_tests.rs

codecov · 2026-05-09T02:19:53Z

Codecov Report

❌ Patch coverage is 54.16667% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.91%. Comparing base (34f574d) to head (a7d48fb).
⚠️ Report is 36 commits behind head on dev.

Files with missing lines	Patch %	Lines
key-wallet-ffi/src/transaction.rs	54.16%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #749      +/-   ##
==========================================
+ Coverage   71.89%   71.91%   +0.02%     
==========================================
  Files         320      320              
  Lines       69473    69465       -8     
==========================================
+ Hits        49947    49957      +10     
+ Misses      19526    19508      -18

Flag	Coverage Δ
core	`76.30% <ø> (ø)`
ffi	`46.57% <54.16%> (+0.07%)`	⬆️
rpc	`20.00% <ø> (ø)`
spv	`87.88% <ø> (+0.03%)`	⬆️
wallet	`71.22% <ø> (+0.03%)`	⬆️

Files with missing lines	Coverage Δ
key-wallet-ffi/src/transaction.rs	`2.92% <54.16%> (+2.92%)`	⬆️

... and 3 files with indirect coverage changes

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>

thepastaclaw · 2026-05-21T12:39:58Z

@coderabbitai review

coderabbitai · 2026-05-21T12:40:06Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

thepastaclaw · 2026-05-21T12:48:12Z

Checked the current head after the CodeRabbit retry: key-wallet-ffi/FFI_API.md and the source docs now use fee_per_kb for wallet_build_and_sign_transaction; I don’t see any remaining fee_rate references for that API. @coderabbitai review

coderabbitai · 2026-05-21T12:48:26Z

@thepastaclaw Thanks for confirming — good to know the fee_rate → fee_per_kb rename is fully propagated across FFI_API.md and the source docs. Triggering a fresh review now.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

thepastaclaw · 2026-06-14T01:13:56Z

Moved this out of draft for human review.

Current status:

CI/checks are green.
The earlier CodeRabbit fee_rate → fee_per_kb doc mismatch was addressed in a7d48fb6; CodeRabbit confirmed the fix in the follow-up inline reply.
No remaining actionable bot feedback found in the current conversation/review comments.

thepastaclaw · 2026-06-14T01:22:11Z

CI follow-up: the new red evaluate run is not a code/test failure. It checked the PR after CodeRabbit approved, saw CI status: all_passed, then failed only when the GitHub Actions integration tried to add the ready-for-review label:

GraphQL: Resource not accessible by integration (addLabelsToLabelable)

I also tried to add ready-for-review directly from this account and GitHub denied it for the same permission reason. So this is a label-permission/automation issue; the actual checks remain green and CodeRabbit approved.

thepastaclaw force-pushed the tracker-1220 branch from e9296b2 to 4b4f9f9 Compare May 9, 2026 02:14

coderabbitai Bot requested changes May 9, 2026

View reviewed changes

Comment thread key-wallet-ffi/FFI_API.md Outdated

thepastaclaw force-pushed the tracker-1220 branch from 4b4f9f9 to a7d48fb Compare May 9, 2026 02:50

thepastaclaw mentioned this pull request May 9, 2026

dash-spv: non-exclusive peer disconnection test times out on Ubuntu ARM #750

Closed

thepastaclaw marked this pull request as ready for review June 14, 2026 01:13

coderabbitai Bot approved these changes Jun 14, 2026

View reviewed changes

thepastaclaw mentioned this pull request Jun 14, 2026

Ready-for-review label workflow lacks label mutation permission #809

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ffi): free transaction byte buffers correctly#749

fix(ffi): free transaction byte buffers correctly#749
thepastaclaw wants to merge 1 commit into
dashpay:devfrom
thepastaclaw:tracker-1220

thepastaclaw commented May 9, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 9, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

thepastaclaw commented May 9, 2026

Uh oh!

coderabbitai Bot commented May 9, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

codecov Bot commented May 9, 2026 •

edited

Loading

Uh oh!

thepastaclaw commented May 21, 2026

Uh oh!

coderabbitai Bot commented May 21, 2026

Uh oh!

thepastaclaw commented May 21, 2026

Uh oh!

coderabbitai Bot commented May 21, 2026

Uh oh!

thepastaclaw commented Jun 14, 2026

Uh oh!

thepastaclaw commented Jun 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

thepastaclaw commented May 9, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix transaction byte buffer freeing

Summary

Validation

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai Bot commented May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

thepastaclaw commented May 9, 2026

Uh oh!

coderabbitai Bot commented May 9, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thepastaclaw commented May 21, 2026

Uh oh!

coderabbitai Bot commented May 21, 2026

Uh oh!

thepastaclaw commented May 21, 2026

Uh oh!

coderabbitai Bot commented May 21, 2026

Uh oh!

thepastaclaw commented Jun 14, 2026

Uh oh!

thepastaclaw commented Jun 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

thepastaclaw commented May 9, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 9, 2026 •

edited

Loading

codecov Bot commented May 9, 2026 •

edited

Loading