[bindings] Give Transaction objects a buffer-is-owned flag.

A lot of our container mapping depends on the `is_owned` flag
which we have for in-crate mapped objects to map references and
non-references into the same container type. Transaction was
mapped to two completely different types (a slice and a Vec type),
which led to a number of edge cases in the bindings generation.
Specifically, I spent a few days trying to map
`[(A, &Transaction)]` properly and came up empty - we map slices
into the same types as Vecs (and rely on the `is_owned` flag to
avoid double-free) and the lack of one for `Transaction` would have
required a special-case in numerous functions.

Instead, we just add a flag in `Transaction` to mirror what we do
for in-crate types and check it before free-ing any underlying
memory.

Note that, sadly, because the c_types objects aren't mapped as a
part of our C++ bindings generation, you have to manually call
`Transaction_free()` even in C++.

diff --git a/c-bindings-gen/src/types.rs b/c-bindings-gen/src/types.rs
index 30377c98a9ee7a4f36efe7473ec3d46e90527e12..1ddbe3cb98975c20ff8b95f59aa910617d82bc9f 100644 (file)
--- a/c-bindings-gen/src/types.rs
+++ b/c-bindings-gen/src/types.rs
@@ -340,8 +340,7 @@ impl<'a, 'c: 'a> TypeResolver<'a, 'c> {
                        "bitcoin::blockdata::script::Script" if is_ref => Some("crate::c_types::u8slice"),
                        "bitcoin::blockdata::script::Script" if !is_ref => Some("crate::c_types::derived::CVec_u8Z"),
                        "bitcoin::blockdata::transaction::OutPoint" if is_ref => Some("crate::chain::transaction::OutPoint"),
-                       "bitcoin::blockdata::transaction::Transaction" if is_ref && !ptr_for_ref => Some("crate::c_types::Transaction"),
-                       "bitcoin::blockdata::transaction::Transaction" => Some("crate::c_types::derived::CVec_u8Z"),
+                       "bitcoin::blockdata::transaction::Transaction" => Some("crate::c_types::Transaction"),
                        "bitcoin::blockdata::transaction::TxOut" if !is_ref => Some("crate::c_types::TxOut"),
                        "bitcoin::OutPoint" => Some("crate::chain::transaction::OutPoint"),
                        "bitcoin::network::constants::Network" => Some("crate::bitcoin::network::Network"),
@@ -413,7 +412,7 @@ impl<'a, 'c: 'a> TypeResolver<'a, 'c> {
                        "bitcoin::blockdata::script::Script" if is_ref => Some("&::bitcoin::blockdata::script::Script::from(Vec::from("),
                        "bitcoin::blockdata::script::Script" if !is_ref => Some("::bitcoin::blockdata::script::Script::from("),
                        "bitcoin::blockdata::transaction::Transaction" if is_ref => Some("&"),
-                       "bitcoin::blockdata::transaction::Transaction" => Some("::bitcoin::consensus::encode::deserialize(&"),
+                       "bitcoin::blockdata::transaction::Transaction" => Some(""),
                        "bitcoin::blockdata::transaction::TxOut" if !is_ref => Some(""),
                        "bitcoin::network::constants::Network" => Some(""),
                        "bitcoin::blockdata::block::BlockHeader" => Some("&::bitcoin::consensus::encode::deserialize(unsafe { &*"),
@@ -471,8 +470,7 @@ impl<'a, 'c: 'a> TypeResolver<'a, 'c> {
                        "bitcoin::secp256k1::key::SecretKey" if is_ref => Some("}[..]).unwrap()"),
                        "bitcoin::blockdata::script::Script" if is_ref => Some(".to_slice()))"),
                        "bitcoin::blockdata::script::Script" if !is_ref => Some(".into_rust())"),
-                       "bitcoin::blockdata::transaction::Transaction" if is_ref => Some(".into_bitcoin()"),
-                       "bitcoin::blockdata::transaction::Transaction" => Some(".into_rust()[..]).unwrap()"),
+                       "bitcoin::blockdata::transaction::Transaction" => Some(".into_bitcoin()"),
                        "bitcoin::blockdata::transaction::TxOut" if !is_ref => Some(".into_rust()"),
                        "bitcoin::network::constants::Network" => Some(".into_bitcoin()"),
                        "bitcoin::blockdata::block::BlockHeader" => Some(" }).unwrap()"),
@@ -553,8 +551,7 @@ impl<'a, 'c: 'a> TypeResolver<'a, 'c> {
                        "bitcoin::secp256k1::Error" if !is_ref => Some("crate::c_types::Secp256k1Error::from_rust("),
                        "bitcoin::blockdata::script::Script" if is_ref => Some("crate::c_types::u8slice::from_slice(&"),
                        "bitcoin::blockdata::script::Script" if !is_ref => Some(""),
-                       "bitcoin::blockdata::transaction::Transaction" if is_ref && !ptr_for_ref => Some("crate::c_types::Transaction::from_slice(&local_"),
-                       "bitcoin::blockdata::transaction::Transaction" => Some("local_"),
+                       "bitcoin::blockdata::transaction::Transaction" => Some("crate::c_types::Transaction::from_vec(local_"),
                        "bitcoin::blockdata::transaction::TxOut" if !is_ref => Some("crate::c_types::TxOut::from_rust("),
                        "bitcoin::blockdata::block::BlockHeader" if is_ref => Some("&local_"),
                        "bitcoin::blockdata::block::Block" if is_ref => Some("crate::c_types::u8slice::from_slice(&local_"),
@@ -617,8 +614,7 @@ impl<'a, 'c: 'a> TypeResolver<'a, 'c> {
                        "bitcoin::secp256k1::Error" if !is_ref => Some(")"),
                        "bitcoin::blockdata::script::Script" if is_ref => Some("[..])"),
                        "bitcoin::blockdata::script::Script" if !is_ref => Some(".into_bytes().into()"),
-                       "bitcoin::blockdata::transaction::Transaction" if is_ref && !ptr_for_ref => Some(")"),
-                       "bitcoin::blockdata::transaction::Transaction" => Some(".into()"),
+                       "bitcoin::blockdata::transaction::Transaction" => Some(")"),
                        "bitcoin::blockdata::transaction::TxOut" if !is_ref => Some(")"),
                        "bitcoin::blockdata::block::BlockHeader" if is_ref => Some(""),
                        "bitcoin::blockdata::block::Block" if is_ref => Some(")"),

diff --git a/lightning-c-bindings/demo.c b/lightning-c-bindings/demo.c
index 645c95bc4a889c944e3290451226064cd6690ed3..44442ef3d56ce58c8f906b25107fd48fe98eb9bb 100644 (file)
--- a/lightning-c-bindings/demo.c
+++ b/lightning-c-bindings/demo.c
@@ -19,6 +19,7 @@ uint32_t get_fee(const void *this_arg, LDKConfirmationTarget target) {
 
 void broadcast_tx(const void *this_arg, LDKTransaction tx) {
        //TODO
+       Transaction_free(tx);
 }
 
 LDKCResult_NoneChannelMonitorUpdateErrZ add_channel_monitor(const void *this_arg, LDKOutPoint funding_txo, LDKChannelMonitor monitor) {

diff --git a/lightning-c-bindings/demo.cpp b/lightning-c-bindings/demo.cpp
index 7829f0265f3a2b4fc40950d4d252ae46889a74a9..8fa20209a6412db31e3820743689f25567677641 100644 (file)
--- a/lightning-c-bindings/demo.cpp
+++ b/lightning-c-bindings/demo.cpp
@@ -115,6 +115,7 @@ static int num_txs_broadcasted = 0; // Technically a race, but ints are atomic o
 void broadcast_tx(const void *this_arg, LDKTransaction tx) {
        num_txs_broadcasted += 1;
        //TODO
+       Transaction_free(tx);
 }
 
 struct NodeMonitors {

diff --git a/lightning-c-bindings/src/c_types/mod.rs b/lightning-c-bindings/src/c_types/mod.rs
index be697d49639d2650b1e4dd9ee2ae57ad730f4128..4fc325f08e0fb6fdf61417ddcf0942b965015a83 100644 (file)
--- a/lightning-c-bindings/src/c_types/mod.rs
+++ b/lightning-c-bindings/src/c_types/mod.rs
@@ -86,25 +86,48 @@ impl Secp256k1Error {
 }
 
 #[repr(C)]
-/// A reference to a serialized transaction, in (pointer, length) form.
-/// This type does *not* own its own memory, so access to it after, eg, the call in which it was
-/// provided to you are invalid.
+/// A serialized transaction, in (pointer, length) form.
+///
+/// This type optionally owns its own memory, and thus the semantics around access change based on
+/// the `data_is_owned` flag. If `data_is_owned` is set, you must call `Transaction_free` to free
+/// the underlying buffer before the object goes out of scope. If `data_is_owned` is not set, any
+/// access to the buffer after the scope in which the object was provided to you is invalid. eg,
+/// access after you return from the call in which a `!data_is_owned` `Transaction` is provided to
+/// you would be invalid.
+///
+/// Note that, while it may change in the future, because transactions on the Rust side are stored
+/// in a deserialized form, all `Transaction`s generated on the Rust side will have `data_is_owned`
+/// set. Similarly, while it may change in the future, all `Transaction`s you pass to Rust may have
+/// `data_is_owned` either set or unset at your discretion.
 pub struct Transaction {
        pub data: *const u8,
        pub datalen: usize,
+       pub data_is_owned: bool,
 }
 impl Transaction {
        pub(crate) fn into_bitcoin(&self) -> BitcoinTransaction {
                if self.datalen == 0 { panic!("0-length buffer can never represent a valid Transaction"); }
                ::bitcoin::consensus::encode::deserialize(unsafe { std::slice::from_raw_parts(self.data, self.datalen) }).unwrap()
        }
-       pub(crate) fn from_slice(s: &[u8]) -> Self {
+       pub(crate) fn from_vec(v: Vec<u8>) -> Self {
+               let datalen = v.len();
+               let data = Box::into_raw(v.into_boxed_slice());
                Self {
-                       data: s.as_ptr(),
-                       datalen: s.len(),
+                       data: unsafe { (*data).as_mut_ptr() },
+                       datalen,
+                       data_is_owned: true,
                }
        }
 }
+impl Drop for Transaction {
+       fn drop(&mut self) {
+               if self.data_is_owned && self.datalen != 0 {
+                       let _ = CVecTempl { data: self.data as *mut u8, datalen: self.datalen };
+               }
+       }
+}
+#[no_mangle]
+pub extern "C" fn Transaction_free(_res: Transaction) { }
 
 #[repr(C)]
 #[derive(Clone)]