feat: range reduction breaks paired claims / unclaims#345
Conversation
|
|
joanestebanr
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a safety validation to detect when a proposer-reduced end_block would cause an import/unclaim pair to be split (and therefore incorrectly treated as a valid claim), plus a helper script to generate a locally-runnable config from a Kurtosis enclave.
Changes:
- Add
validate_no_broken_pairsto detect broken (imported_bridge_exit, unclaim) pairs under range reduction, with unit tests. - Wire the validation into
AggchainProofServiceand introduce a dedicated error variant. - Add a script to download the Kurtosis config artifact and rewrite internal endpoints/paths for local execution.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/generate_config_based_kurtosis_to_run_locally.sh | Downloads config/genesis from Kurtosis and rewrites endpoints/paths for local runs. |
| crates/aggchain-proof-service/src/validation.rs | Implements broken-pair detection logic with a comprehensive test suite. |
| crates/aggchain-proof-service/src/service.rs | Calls the new validation when the proposer returns a different end_block. |
| crates/aggchain-proof-service/src/lib.rs | Registers the new validation module. |
| crates/aggchain-proof-service/src/error.rs | Adds an error variant for broken import/unclaim pairs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
joanestebanr
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0545b620d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// The k-th unclaim for a given `global_index` (sorted by block_number) | ||
| /// is matched to the k-th import with the same `global_index` (also sorted by | ||
| /// block_number). A pair is broken when the import falls within the new block | ||
| /// range but its matching unclaim does not, meaning the import would appear as | ||
| /// valid in the proof even though it was intended to be cancelled. | ||
| pub(crate) fn validate_no_broken_pairs( | ||
| imported_bridge_exits: &[ImportedBridgeExitWithBlockNumber], | ||
| unclaims: &[UnclaimWithBlockNumber], | ||
| new_end_block: u64, | ||
| ) -> Result<(), Error> { | ||
| // Collect import block_numbers per global_index. | ||
| let mut imports_by_global_index: HashMap<U256, Vec<u64>> = HashMap::new(); | ||
| for import in imported_bridge_exits { | ||
| let global_index: U256 = import.global_index.into(); | ||
| imports_by_global_index | ||
| .entry(global_index) | ||
| .or_default() | ||
| .push(import.block_number); | ||
| } | ||
| for blocks in imports_by_global_index.values_mut() { | ||
| blocks.sort_unstable(); | ||
| } | ||
|
|
||
| // Collect unclaim block_numbers per global_index. | ||
| let mut unclaims_by_global_index: HashMap<U256, Vec<u64>> = HashMap::new(); | ||
| for unclaim in unclaims { | ||
| unclaims_by_global_index | ||
| .entry(unclaim.global_index) | ||
| .or_default() | ||
| .push(unclaim.block_number); | ||
| } | ||
| for blocks in unclaims_by_global_index.values_mut() { | ||
| blocks.sort_unstable(); | ||
| } | ||
|
|
||
| // Check each (import_k, unclaim_k) pair for the same global_index. | ||
| for (global_index, unclaim_blocks) in &unclaims_by_global_index { | ||
| let import_blocks = imports_by_global_index | ||
| .get(global_index) | ||
| .map(Vec::as_slice) | ||
| .unwrap_or(&[]); | ||
|
|
||
| for (k, &unclaim_block) in unclaim_blocks.iter().enumerate() { | ||
| let Some(&import_block) = import_blocks.get(k) else { | ||
| // No matching import for this unclaim position; not a split. | ||
| continue; | ||
| }; | ||
|
|
There was a problem hiding this comment.
validate_no_broken_pairs collects only block_number (dropping log_index) and then sorts, which can mis-order events that occur in the same block and therefore mis-pair imports/unclaims. Since ImportedBridgeExitWithBlockNumber and UnclaimWithBlockNumber define Ord including log_index, store and sort full events (or at least (block_number, log_index)) when building the per-global_index vectors so pairing is deterministic and chronologically correct.
| /// The k-th unclaim for a given `global_index` (sorted by block_number) | |
| /// is matched to the k-th import with the same `global_index` (also sorted by | |
| /// block_number). A pair is broken when the import falls within the new block | |
| /// range but its matching unclaim does not, meaning the import would appear as | |
| /// valid in the proof even though it was intended to be cancelled. | |
| pub(crate) fn validate_no_broken_pairs( | |
| imported_bridge_exits: &[ImportedBridgeExitWithBlockNumber], | |
| unclaims: &[UnclaimWithBlockNumber], | |
| new_end_block: u64, | |
| ) -> Result<(), Error> { | |
| // Collect import block_numbers per global_index. | |
| let mut imports_by_global_index: HashMap<U256, Vec<u64>> = HashMap::new(); | |
| for import in imported_bridge_exits { | |
| let global_index: U256 = import.global_index.into(); | |
| imports_by_global_index | |
| .entry(global_index) | |
| .or_default() | |
| .push(import.block_number); | |
| } | |
| for blocks in imports_by_global_index.values_mut() { | |
| blocks.sort_unstable(); | |
| } | |
| // Collect unclaim block_numbers per global_index. | |
| let mut unclaims_by_global_index: HashMap<U256, Vec<u64>> = HashMap::new(); | |
| for unclaim in unclaims { | |
| unclaims_by_global_index | |
| .entry(unclaim.global_index) | |
| .or_default() | |
| .push(unclaim.block_number); | |
| } | |
| for blocks in unclaims_by_global_index.values_mut() { | |
| blocks.sort_unstable(); | |
| } | |
| // Check each (import_k, unclaim_k) pair for the same global_index. | |
| for (global_index, unclaim_blocks) in &unclaims_by_global_index { | |
| let import_blocks = imports_by_global_index | |
| .get(global_index) | |
| .map(Vec::as_slice) | |
| .unwrap_or(&[]); | |
| for (k, &unclaim_block) in unclaim_blocks.iter().enumerate() { | |
| let Some(&import_block) = import_blocks.get(k) else { | |
| // No matching import for this unclaim position; not a split. | |
| continue; | |
| }; | |
| /// The k-th unclaim for a given `global_index` (sorted chronologically using | |
| /// the event type's `Ord`, including `block_number` and `log_index`) is | |
| /// matched to the k-th import with the same `global_index` (sorted the same | |
| /// way). A pair is broken when the import falls within the new block range | |
| /// but its matching unclaim does not, meaning the import would appear as | |
| /// valid in the proof even though it was intended to be cancelled. | |
| pub(crate) fn validate_no_broken_pairs( | |
| imported_bridge_exits: &[ImportedBridgeExitWithBlockNumber], | |
| unclaims: &[UnclaimWithBlockNumber], | |
| new_end_block: u64, | |
| ) -> Result<(), Error> { | |
| // Collect imports per global_index and preserve full event ordering. | |
| let mut imports_by_global_index: HashMap<U256, Vec<&ImportedBridgeExitWithBlockNumber>> = | |
| HashMap::new(); | |
| for import in imported_bridge_exits { | |
| let global_index: U256 = import.global_index.into(); | |
| imports_by_global_index | |
| .entry(global_index) | |
| .or_default() | |
| .push(import); | |
| } | |
| for imports in imports_by_global_index.values_mut() { | |
| imports.sort_unstable(); | |
| } | |
| // Collect unclaims per global_index and preserve full event ordering. | |
| let mut unclaims_by_global_index: HashMap<U256, Vec<&UnclaimWithBlockNumber>> = | |
| HashMap::new(); | |
| for unclaim in unclaims { | |
| unclaims_by_global_index | |
| .entry(unclaim.global_index) | |
| .or_default() | |
| .push(unclaim); | |
| } | |
| for unclaims_for_global_index in unclaims_by_global_index.values_mut() { | |
| unclaims_for_global_index.sort_unstable(); | |
| } | |
| // Check each (import_k, unclaim_k) pair for the same global_index. | |
| for (global_index, unclaim_events) in &unclaims_by_global_index { | |
| let import_events: &[&ImportedBridgeExitWithBlockNumber] = imports_by_global_index | |
| .get(global_index) | |
| .map(Vec::as_slice) | |
| .unwrap_or(&[]); | |
| for (k, unclaim) in unclaim_events.iter().enumerate() { | |
| let Some(import) = import_events.get(k) else { | |
| // No matching import for this unclaim position; not a split. | |
| continue; | |
| }; | |
| let import_block = import.block_number; | |
| let unclaim_block = unclaim.block_number; |
| // Collect unclaim block_numbers per global_index. | ||
| let mut unclaims_by_global_index: HashMap<U256, Vec<u64>> = HashMap::new(); | ||
| for unclaim in unclaims { | ||
| unclaims_by_global_index | ||
| .entry(unclaim.global_index) | ||
| .or_default() | ||
| .push(unclaim.block_number); | ||
| } | ||
| for blocks in unclaims_by_global_index.values_mut() { | ||
| blocks.sort_unstable(); | ||
| } |
There was a problem hiding this comment.
Same issue for unclaims: only block_number is stored and sorted, ignoring log_index. This can cause incorrect pairing and false negatives/positives when multiple unclaims for the same global_index are emitted in the same block. Consider sorting UnclaimWithBlockNumber (or (block_number, log_index)) instead of plain block numbers.
Add Bash 4.2+ version check (macOS ships 3.2), a portable sedi() function to handle sed -i differences between Linux and macOS, and a portable_realpath() fallback for systems without GNU coreutils. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
tonic::Status is 176 bytes and is part of the Proofs trait signature generated by tonic — we have no control over its size in test closures or in the mockall::mock! expansion. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…order Events with the same global_index in the same block were only sorted by block_number, causing the k-th import to be paired with the wrong k-th unclaim and either missing a real split or raising a false positive. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
joanestebanr
force-pushed
the
feat/unclaim_unpaired-343
branch
from
545e56b to
9f96677
Compare
hadjiszs
left a comment
hadjiszs
left a comment
There was a problem hiding this comment.
Thanks! Here is a first pass of review
| let Some(&(import_block, _)) = import_events.get(k) else { | ||
| // No matching import for this unclaim position; not a split. | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
can we end up in this else? Having one unclaim without a corresponding import in the block range that we are processing leads to an error normally no?
| /// import falls within the new block range but its matching unclaim does not, | ||
| /// meaning the import would appear as valid in the proof even though it was | ||
| /// intended to be cancelled. | ||
| pub(crate) fn validate_no_broken_pairs( |
There was a problem hiding this comment.
I asked claude to try to make this function more readable
it suggested something like below (not tested nor properly reviewed), wdyt? Happy to review another version if you find better 👌
but basically trying to have dedicated structs if possible, and splitting this function into "build pairs into well-suited data structures" followed by "check the pairs"
#[derive(Debug)]
struct ImportUnclaimPair {
global_index: U256,
import_block: u64,
unclaim_block: u64,
}
pub(crate) fn validate_no_broken_pairs(
imports: &[ImportedBridgeExitWithBlockNumber],
unclaims: &[UnclaimWithBlockNumber],
new_end_block: u64,
) -> Result<(), Error> {
for ImportUnclaimPair {
global_index,
import_block,
unclaim_block,
} in pair_imports_with_unclaims(imports, unclaims)
{
if import_block <= new_end_block && unclaim_block > new_end_block {
return Err(Error::BrokenImportUnclaimPair {
global_index,
import_block,
unclaim_block,
new_end_block,
});
}
}
Ok(())
}
/// Pairs the k-th import event for each `global_index` with the k-th unclaim
/// for the same `global_index`, both in (block_number, log_index) order.
/// Excess events on either side are dropped — they correspond to imports
/// not yet cancelled, or unclaims targeting imports outside this witness.
fn pair_imports_with_unclaims(
imports: &[ImportedBridgeExitWithBlockNumber],
unclaims: &[UnclaimWithBlockNumber],
) -> Vec<ImportUnclaimPair> {
#[derive(Default)]
struct EventLists {
imports: Vec<(u64, u64)>,
unclaims: Vec<(u64, u64)>,
}
let mut by_gi: HashMap<U256, EventLists> = HashMap::new();
for ev in imports {
by_gi
.entry(ev.global_index.into())
.or_default()
.imports
.push((ev.block_number, ev.log_index));
}
for ev in unclaims {
by_gi
.entry(ev.global_index)
.or_default()
.unclaims
.push((ev.block_number, ev.log_index));
}
by_gi
.into_iter()
.flat_map(|(global_index, mut events)| {
events.imports.sort_unstable();
events.unclaims.sort_unstable();
events
.imports
.into_iter()
.zip(events.unclaims)
.map(move |((import_block, _), (unclaim_block, _))| ImportUnclaimPair {
global_index,
import_block,
unclaim_block,
})
})
.collect()
}| expected_by_contract: Box::new(retrieved_from_contracts), | ||
| expected_by_verifier: Box::new(aggregation_proof_public_values.clone()), | ||
| }); | ||
| if std::env::var("IGNORE_VKEY_MISMATCH").is_ok() { |
There was a problem hiding this comment.
the check isn't only about vkeys here but on all the public values of the proof received from the op-succinct proposer. To check that they are the same as the ones expected by the L1 verifier (e.g., prev / new blocks, l1 head etc)
just to be sure I understand the motivation, this check is a problem with the aggkit-prover run in which way and with which setup?
There was a problem hiding this comment.
I introduce that to be able to run aggkit-prover even that is going to fail to return a proof becuase is not going to pass the verify of the proof locally. This allow to start the server and attend gRPC requests...
If you think that can be dangerous in some way I'll remove it. WDTY?
Summary
This PR adds two independent safety features:
Fix: #343 Detect broken import/unclaim pairs after range reduction
When the proposer reduces the
end_blockof a certificate (range reduction), it is possible that animported_bridge_exitfalls within the new range while its matchingunclaimdoes not. This would cause the import to appear as valid in the proof even though it was intentionally cancelled.A new
validate_no_broken_pairscheck is added inaggchain-proof-servicethat, whenever the proposer returns a differentend_blockthan the one originally requested, verifies that no(imported_bridge_exit, unclaim)pair is split by the new boundary. Pairs are matched byglobal_indexin chronological order (k-th import ↔ k-th unclaim). If a broken pair is found, proof generation is aborted with a descriptive error.Fix: #346 full error chain returned to gRPC clients
Previously, gRPC error responses only included the top-level error message (
error.to_string()), silently dropping all nested cause messages. A newformat_error_chainhelper walks the fullsource()chain and concatenates all messages, so clients now receive the complete error description.Chore: IGNORE_VKEY_MISMATCH environment variable to be able to develop/debug locally
Adds an opt-in escape hatch for development/testing environments: when
IGNORE_VKEY_MISMATCHis set, vkey hash and range vkey commitment mismatches between the on-chain op-succinct config and the local ELF are logged as warnings instead of returning an error, allowing the server to continue running. The mismatch is still clearly surfaced in the logs. This flag should never be set in production.Chore: scrtipt automates the setup needed to run the aggkit-prover locally against a live Kurtosis enclave (a local multi-service test environment).
Changes
aggchain-proof-service/src/validation.rs— new module withvalidate_no_broken_pairsand unit testsaggchain-proof-service/src/service.rs— calls the validation after receiving a proposer response with a reduced end blockaggchain-proof-service/src/error.rs— newBrokenImportUnclaimPairerror variantaggchain-proof-builder/src/lib.rs—IGNORE_VKEY_MISMATCHsupport for aggregation vkey hash and range vkey commitment checksaggkit-prover/src/rpc.rs—format_error_chainhelper; full error chain now returned to gRPC clientsproposer-client/src/rpc/mod.rs— minor error wrapping fixscripts/generate_config_based_kurtosis_to_run_locally.sh— new helper script for local Kurtosis-based testingFixes #343, #346