fix: c2m-bridge hardening: do not skip failed transfers and retry in the next block instead - #1980
fix: c2m-bridge hardening: do not skip failed transfers and retry in the next block instead#1980LGLO wants to merge 5 commits into
Conversation
![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: 2faa2b2b01
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
2faa2b2 to
21b65b4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21b65b421c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
PR Review Summary
PR: #1980 — fix: c2m-bridge hardening: do not skip failed transfers and retry in the next block instead
Plan: work package README
Activities: Start Work Package · Implementation Analysis · Lean-Coding Audit · Post-Implementation Review · Strategic Review
Reports: prior feedback triage · implementation analysis · code review · test suite review · strategic review
Date: 2026-08-07
Executive Summary
The retry protocol this change introduces is correct end to end, and one route to the silent permanent loss it exists to remove survives through the pallet's success path. The widened checkpoint encoding also ships under an unmoved runtime API version while the body ticks backward compatibility.
Overall Rating: Request Changes
- No build, test, or lint run was attempted, so no finding rests on an executed result — why the suite cannot run here.
- Two comment threads arrived after triage and are not dispositioned here — thread.
- This summary carries the ticket-scope and adjacent findings; sixteen further shape and hygiene findings travel separately — delivery scope.
Prior Feedback Triage
| # | Finding | Author | Disposition |
|---|---|---|---|
| PF-1 | Keep the checkpoint when the SQL row limit is hit | chatgpt-codex-connector[bot] | Confirmed |
| PF-2 | Bounding max_transfers away from 0 and 1 settles it |
LGLO | Refuted |
| PF-3 | Bump the bridge runtime API version | chatgpt-codex-connector[bot] | Confirmed |
| PF-4 | Nodes are updated before the runtime | LGLO | Confirmed |
| PF-5 | Put the change entry under component directories | chatgpt-codex-connector[bot] | Refuted |
| PF-6 | Rebuild metadata for the new checkpoint variant | chatgpt-codex-connector[bot] | Superseded |
| PF-7 | Two pipeline jobs failed | datadog-official[bot] | Superseded |
| PF-8 | Codex review wrapper bodies | chatgpt-codex-connector[bot] | Refuted |
| PF-9 | Empty review bodies carrying the inline replies | LGLO | Refuted |
Code Review Findings
| # | @ | Finding | Severity |
|---|---|---|---|
| CR-1 | > | Truncation is decided in transfers under a limit counted in transactions | High |
| CR-2 | > | The positional drain never checks it is draining the transaction the count came from | High |
| CR-3 | > | A widened SCALE shape is advertised under an unbumped runtime API version | High |
| CR-4 | > | Governance can write a PartialTx with an unvalidated count, and the new documentation sends operators there |
High |
| CR-5 | > | The transfer loop's transaction-grouping assumption is unstated and unasserted | Medium |
| CR-6 | > | Clamps make a violated invariant survivable rather than visible | Medium |
| CR-7 | > | The compatibility guarantee substituting for CR-3 is unrecorded | Medium |
| CR-8 | — | The rejected design alternative and its cost are unrecorded | Medium |
| CR-9 | > | Checkpoint resolution is duplicated across the two data sources, and this change extended both copies by hand | N/A |
| CR-10 | > | Change file outside the component directories it tags | N/A |
| CR-11 | > | TransferHandlerError encodes a policy that does not exist |
N/A |
| CR-12 | > | Weight is charged for the whole batch when the loop breaks early | N/A |
| CR-13 | > | An unexplained lockfile bump widens the authored surface | N/A |
| CR-14 | > | An approval is orphaned when a permanently failing transfer is skipped by governance | N/A |
| CR-15 | > | A count recorded against one transfer derivation is applied to another, and consensus cannot see it | Medium |
Test Review Findings
| # | @ | Finding | Severity |
|---|---|---|---|
| TR-1 | > | No test combines a PartialTx drain with the query row limit |
High |
| TR-2 | > | The harness cannot reach one of the two arms the change added | N/A |
| TR-3 | > | The new wire format has no codec test | N/A |
| TR-4 | > | No scenario varies the transfer derivation between two calls | N/A |
| TR-5 | > | The transaction-grouping assumption is untested | N/A |
| TR-6 | > | One assertion claims a path it cannot distinguish | N/A |
| TR-7 | > | One scenario asserts behaviour for a state no writer produces | N/A |
| TR-8 | > | Two of the three TransferHandler implementors have no test |
N/A |
| TR-9 | > | The governance write path is untested against the widened variant | N/A |
Strategic Review
| # | @ | Finding | Severity |
|---|---|---|---|
| SR-1 | > | an enum introduced for a skip policy the change does not implement | Medium |
| SR-2 | > | a transitive lockfile bump, the one hunk with no relation to the ticket | Medium |
| SR-3 | > | a ticked backward-compatibility claim the change does not support | High |
| SR-4 | — | Fork Strategy answers nothing | High |
| SR-5 | — | Testing Evidence understates the change | N/A |
| SR-6 | — | the Links section is empty | N/A |
| SR-7 | — | the Overview is the change file verbatim, typos included (nit) | N/A |
| SR-8 | > | withdraw the placement half of CR-10; keep the proofreading half | N/A |
| SR-9 | > | drop the spec_version clause from CR-3; keep the version split |
N/A |
What This Change Gets Right
- The write-back gets all three outcomes right, including the no-write arm that makes a first-transfer failure safe — >
- The approval is removed only after the ledger apply returns
Ok, so a retry still finds it — > - A distinct error type keeps every transfer failure inside the
Okpath, so no transfer rejects a block — > - The new variant is appended at index 2, so old encodings still decode under the new type — >
- The inherent check compares the proposed checkpoint, so a divergent drain surfaces as a rejected block — >
Action Items
Must Address (Blocking):
- Decide truncation from the returned transaction count, not the post-drain transfer count (CR-1, PF-1)
- Compare the checkpoint hash against the first transaction and raise an error on a mismatch (CR-2)
- Move the bridge runtime API to version 2 with a v1/v2 method split (CR-3, PF-3)
- Validate or reject a partial-transaction count in the governance extrinsic (CR-4)
- Add the scenario that puts a drain and the row limit together (TR-1)
- Untick or qualify the backward-compatibility claim in the body (SR-3)
Should Address (Recommended):
- State the transaction-grouping invariant and test the non-contiguous case (CR-5)
- Keep the clamp and add the error that makes the violation visible (CR-6)
- Write down the rollout ordering the compatibility claim depends on (CR-7, PF-4)
- Record the rejected rewind alternative and its idempotency cost (CR-8)
- State the derivation-stability assumption at the transfer expansion (CR-15)
- Answer Fork Strategy with the rollout ordering (SR-4)
- Collapse the single-variant error enum to a documented unit type (SR-1)
- Say in one sentence which audit failure the lockfile bump fixes (SR-2)
Posted by an automated review agent on behalf of @m2ux. The recommendation reflects an independent re-verification at head 21b65b42; the maintainers retain full discretion over disposition.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2540e832ee
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Regarding @m2ux automated review. CR-2: if the condition occurs it means we have a bug. I think there is no bug in the code and it will never happen that we have a mismatch. DataSourceError would stop the block production. CR-3: yeah, we widened but "Node supports Runtime" and it has to be rolled out earlier, this is a general rule. I took slack approach because we don't have bridge enabled anywhere in production and no bridge transfer. The state of World is that we won't have problems. CR-4: refuting. Governance can do any harm to the chain in many different ways. Examples: resetting the checkpoint to the past or setting wrong addresses. We need some sane assumptions and governance bearing responsibility is one of them. Please tell your AI that we can't assume governance being incompetent. They have the power to replace runtime with whatever they want. CR-5: this can be added, but PR is big enough and it does not change the assumption that transfers come in the Cardano ordering. Maybe it is not stated clearly enought, but whole data checkpointing lives on this assumption. CR-6: the first place pointed in the review is okay, because the transactions matches there. The second place has been refactored and doesn't apply. |
7ad9c04 to
d6700c4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bedb9e6ef
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -0,0 +1,12 @@ | |||
| #node #runtime #c2m-bridge | |||
There was a problem hiding this comment.
The fresh tree moves the earlier top-level entry into node/runtime directories, but the PR still changes toolkit products under partner-chains/toolkit/bridge and partner-chains/toolkit/data-sources without adding anything under changes/toolkit/changed; toolkit release-note generation can therefore omit the new checkpoint and observability behavior. Add a corresponding toolkit change file rather than covering those changes only with this node entry.
AGENTS.md reference: AGENTS.md:L197-L201
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
#toolkit is not for partner-chains toolkit, but #runtime should be removed from this one
You are on holidays and I'll get others to review it.
…the next block instead Signed-off-by: Lech Głowiak <lech.glowiak@shielded.io>
Signed-off-by: Lech Głowiak <lech.glowiak@shielded.io>
…ansaction is Reserve part has been processed by the ICS part has not Signed-off-by: Lech Głowiak <lech.glowiak@shielded.io>
Signed-off-by: Lech Głowiak <lech.glowiak@shielded.io>
Signed-off-by: Lech Głowiak <lech.glowiak@shielded.io>
1bedb9e to
4a02c06
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a02c06b2b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| match transfers.last() { | ||
| Some(tx) => tx.checkpoint(), | ||
| None => current_checkpoint.clone(), |
There was a problem hiding this comment.
When upgraded nodes still run the old runtime, hitting the transfer limit immediately after the reserve half of a Cardano transaction makes this return the new TxReserveTransfer variant. The old runtime decodes inherent data using the previous two-variant enum, so its create_inherent path rejects the unknown discriminant and cannot build the block; because every retry reaches the same boundary, block production can remain stuck and the runtime upgrade itself may be impossible to enact. Fresh evidence beyond the earlier mixed-binary concern is that the stated node-first rollout order is itself unsafe here: version the API/pallet and retain the legacy no-split behavior while connected to the old runtime.
Useful? React with 👍 / 👎.
3 participants
Overview
C2M Bridge - handling ledger transaction execution errors
The bridge will not move checkpoint when transaction fails execution or serialization.
PC bridge pallet will retry it again in the next block. If it keeps failing, it perhaps
means there is a bug that has to be fixed. Bridge will stop processing at this point.
Future-proof enum (SCALE-friendly) error is introduced in case we would like to skip on some errors in the future.
The exectly-once processing is quite complex because we still don't know if Cardano validator would prevent
transactions that release from unlocked and reserve to ICS at the same time.
A very unlikely corner case when bridge max transfers is configured to 1 and transaction mapped
to two transfers is encountered has been reimplemented to process this Cardano tx in two midnight blocks instead of silently skipping all transfer up to given block bound.
🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCO🧪 Testing Evidence
Please describe any additional testing aside from CI:
🔱 Fork Strategy
Links