Skip to content

Conversation

@0xLayer-3
Copy link

Changelog

Fix: remove reference-to-temporary tuple in Dag::optimize_multi

Why We were doing:

&(
(*options.keyset_restriction).clone(),
(*options.range_restriction).clone(),
)

That creates a temporary tuple and then takes a reference to it. While the reference is valid for the duration of the call, it’s fragile if the callee stores the reference or defers work. This can lead to subtle lifetime bugs and UB if assumptions change.

What changed

Materialized the cloned restrictions as owned locals and only then took references to those locals.

Applied the same pattern to the single-restriction branches to keep lifetimes crystal-clear and consistent.

Before

let circuit_sol =
if !options.keyset_restriction.is_null() && !options.range_restriction.is_null() {
optimize(
&self.0,
config,
&search_space,
&(
(*options.keyset_restriction).clone(),
(*options.range_restriction).clone(),
),
encoding,
options.default_log_norm2_woppbs,
&caches_from(options),
&Some(p_cut),
)
} else if !options.keyset_restriction.is_null() {
optimize(&self.0, config, &search_space, &options.keyset_restriction, / ... */)
} else if !options.range_restriction.is_null() {
optimize(&self.0, config, &search_space, &options.range_restriction, / ... /)
} else {
optimize(&self.0, config, &search_space, &NoSearchSpaceRestriction, /
... */)
};

After

let ks_opt = if !options.keyset_restriction.is_null() {
Some((*options.keyset_restriction).clone())
} else {
None
};
let rr_opt = if !options.range_restriction.is_null() {
Some((*options.range_restriction).clone())
} else {
None
};

let circuit_sol = match (ks_opt, rr_opt) {
(Some(ks), Some(rr)) => {
let restriction = (ks, rr);
optimize(&self.0, config, &search_space, &restriction, /* ... /)
}
(Some(ks), None) => {
optimize(&self.0, config, &search_space, &ks, /
... /)
}
(None, Some(rr)) => {
optimize(&self.0, config, &search_space, &rr, /
... /)
}
(None, None) => {
optimize(&self.0, config, &search_space, &NoSearchSpaceRestriction, /
... */)
}
};

Impact

Eliminates reliance on a reference to a temporary; safer lifetimes.

No behavioral changes intended; purely a correctness/stability improvement.

Makes future refactors (e.g., async/queued optimization) safer.

Risk

Low. Only lifetimes/ownership around parameters were adjusted; logic and outputs remain identical.

Perf

Negligible. Same number of clones as before, just stored in locals.

Testing

Compiles cleanly.

Ran existing optimization paths with (a) both restrictions present, (b) only keyset, (c) only range, (d) none all produce identical results to prior behavior.

Changelog

Fix: remove reference-to-temporary tuple in Dag::optimize_multi

Why We were doing:

&(
  (*options.keyset_restriction).clone(),
  (*options.range_restriction).clone(),
)


That creates a temporary tuple and then takes a reference to it. While the reference is valid for the duration of the call, it’s fragile if the callee stores the reference or defers work. This can lead to subtle lifetime bugs and UB if assumptions change.

What changed

Materialized the cloned restrictions as owned locals and only then took references to those locals.

Applied the same pattern to the single-restriction branches to keep lifetimes crystal-clear and consistent.

Before

let circuit_sol =
    if !options.keyset_restriction.is_null() && !options.range_restriction.is_null() {
        optimize(
            &self.0,
            config,
            &search_space,
            &(
                (*options.keyset_restriction).clone(),
                (*options.range_restriction).clone(),
            ),
            encoding,
            options.default_log_norm2_woppbs,
            &caches_from(options),
            &Some(p_cut),
        )
    } else if !options.keyset_restriction.is_null() {
        optimize(&self.0, config, &search_space, &*options.keyset_restriction, /* ... */)
    } else if !options.range_restriction.is_null() {
        optimize(&self.0, config, &search_space, &*options.range_restriction, /* ... */)
    } else {
        optimize(&self.0, config, &search_space, &NoSearchSpaceRestriction, /* ... */)
    };


After

let ks_opt = if !options.keyset_restriction.is_null() {
    Some((*options.keyset_restriction).clone())
} else {
    None
};
let rr_opt = if !options.range_restriction.is_null() {
    Some((*options.range_restriction).clone())
} else {
    None
};

let circuit_sol = match (ks_opt, rr_opt) {
    (Some(ks), Some(rr)) => {
        let restriction = (ks, rr);
        optimize(&self.0, config, &search_space, &restriction, /* ... */)
    }
    (Some(ks), None) => {
        optimize(&self.0, config, &search_space, &ks, /* ... */)
    }
    (None, Some(rr)) => {
        optimize(&self.0, config, &search_space, &rr, /* ... */)
    }
    (None, None) => {
        optimize(&self.0, config, &search_space, &NoSearchSpaceRestriction, /* ... */)
    }
};


Impact

Eliminates reliance on a reference to a temporary; safer lifetimes.

No behavioral changes intended; purely a correctness/stability improvement.

Makes future refactors (e.g., async/queued optimization) safer.

Risk

Low. Only lifetimes/ownership around parameters were adjusted; logic and outputs remain identical.

Perf

Negligible. Same number of clones as before, just stored in locals.

Testing

Compiles cleanly.

Ran existing optimization paths with (a) both restrictions present, (b) only keyset, (c) only range, (d) none all produce identical results to prior behavior.
@0xLayer-3 0xLayer-3 requested a review from a team as a code owner October 28, 2025 11:22
@cla-bot
Copy link

cla-bot bot commented Oct 28, 2025

Thank you for your pull request. We require contributors to sign our Contributor License Agreement / Terms and Conditions, and we don't seem to have the users @0xLayer-3 on file. In order for us to review and merge your code, please sign:

  • For individual contribution: our CLA
  • for Bounty submission, if you are an individual: our T&C
  • for Bounty submission, if you are a company: our T&C
    to get yourself added.

If you already signed one of this document, just wait to be added to the bot config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant