[KOL-91] Implement Ethereum reorg mitigation by side2k · Pull Request #512 · fpco/kolme

side2k · 2026-03-25T19:06:11Z

Task: https://fpcomplete.atlassian.net/browse/KOL-91

add confirmation_depth(Option<u64>) field to ChainConfig
in Ethereum listener, poller now is the single source of data
Ethereum subscriber now forms a FIFO-queue of hints - these hints are used by poller to skip chunks of blocks
in existing integration tests for Ethereum, explicitly configure confirmation_depth to None
add one more integration test with confirmation_depth set to 1, and inside this test ensure that event is not processed until extra block is generated
update docs accordingly

…t confirmation_depth

… fast-track path in poller

cloudflare-workers-and-pages · 2026-03-25T19:07:25Z

Deploying kolme with Cloudflare Pages

Latest commit:	`7a04403`
Status:	✅ Deploy successful!
Preview URL:	https://119341b8.kolme.pages.dev
Branch Preview URL:	https://ethereum-bridge-4-reorgs.kolme.pages.dev

View logs

…tting

snoyberg · 2026-03-26T09:22:10Z

+    /// This setting is generic for future chain listeners, but currently enforced
+    /// only by the Ethereum listener.
+    #[serde(default = "ConfirmationDepth::use_default")]
+    pub confirmation_depth: ConfirmationDepth,


I originally hadn't considered this as part of the chain config itself, but rather a config set in the listeners. I can see the advantage of having it as part of the chain config, but it's also somewhat unenforceable. Putting it here does force a migration of data, but it's not a big deal. And even though the existing chains don't use a confirmation depth, they probably should, so I'm OK with this being present for all chain types.

Any thoughts on chain config vs listener-specific config?

Chain config seems to be the single source of truth for all nodes - so storing confirmation depth there seems more robust.
Could you please explain what did you mean by unenforceable?

Unenforceable in the sense that it's the listeners themselves who will be deciding whether sufficient confirmations are present, and they can choose to ignore this setting if they so choose. With other verifications, external entities can confirm whether a validator cheated (e.g., issuing a withdrawal to a chain without a valid Kolme-chain action, or a processor producing invalid blocks). For this case, there would still be a valid transaction on the external chain, it's just a question of whether the listener added it earlier than they should have.

they can choose to ignore this setting

But this depends on listener's code, and not on where setting is stored, if I understand you correctly?

Correct. That's my point: "enforcing" this at the chain level doesn't add any extra protection.

snoyberg · 2026-03-26T09:26:16Z

+        EthereumChain::Mainnet | EthereumChain::Sepolia => POLL_INTERVAL_DEFAULT,
+    };
    let mut next_ws_retry = tokio::time::Instant::now();
+    let mut subscriber_task: Option<tokio::task::JoinHandle<()>> = None;


This approach risks the spawned task outliving its parent, which I don't think is intended. There are a few ways to approach this, but I'm not really sure what the goal of the overall code is, so I'm not sure what to propose.

The goal is to launch a subscriber, which gets events and stores their ids and block ids as hints for the poller, so it can skip blocks and jump straight to a next event blocks, when it becomes eligible.

I mean why does this need to be a separate task instead of in the current task?

Because subscriber runs concurrently with a poller. When subscriber receives events, they're beyond the reach of the poller, but if it has issues running (e.g. RPC provider does not support WS) - we don't want it to affect poller's work at all.

I will fix the "exit when parent is done" thing.

The approach doesn't work in general: panics will not lead to the subtask being aborted. The part of goal here that I was unclear on is why do we need both an external loop and a separate polling task, plus why we didn't need it previously.

why we didn't need it previously

We did not need it, because subscriber was the default source of data and polling was a fallback. So, subscriber, technically, had its own loop, just not in parallel with poller.

For the design, that has been implemented in current PR, I did not want stream.next().await to block poller. Now, that I've looked more thoroughly and found out that tokio::select!() can be used to orchestrate poller and subscriber and, potentially, make whole thing more clean. I'll implement it and update you.

Addressed this.

I don't see any new commits on the PR, can you link to the change?

Sorry, forgot to push. Fixed.

… is done

…h setting This reverts commit de6b6db.

snoyberg · 2026-03-27T07:08:32Z

    /// only by the Ethereum listener.
-    #[serde(default = "ConfirmationDepth::use_default")]
-    pub confirmation_depth: ConfirmationDepth,
+    pub confirmation_depth: Option<u64>,


This still leaves two questions:

Do we want to allow for a "default" value here, which may change implicitly with code base updates? I lean towards no.

Do we want this as part of chain config as opposed to runtime listener config.

snoyberg · 2026-03-27T07:09:56Z

+        EthereumChain::Mainnet | EthereumChain::Sepolia => POLL_INTERVAL_DEFAULT,
+    };
    let mut next_ws_retry = tokio::time::Instant::now();
+    let mut subscriber_task: Option<tokio::task::JoinHandle<()>> = None;


The approach doesn't work in general: panics will not lead to the subtask being aborted. The part of goal here that I was unclear on is why do we need both an external loop and a separate polling task, plus why we didn't need it previously.

Also: - do some refactoring and cleanup - separate Startup and Runtime errors from the subscriber - the former lead to subscriber restart for a limited amount of times, the latter do not

snoyberg

I'm not comfortable with the approach here, and it still comes back to my original confusion about why all this code is needed. If we're legitimately just running two tasks simultaneously, why not use select! or JoinSet? The code structure looks like it's allowing for optionality in the subscriber_supervisor which is never actually needed.

snoyberg · 2026-04-05T07:44:05Z

+
+impl SubscriberTaskGuard {
+    fn set(&mut self, handle: tokio::task::JoinHandle<()>) {
+        self.handle = Some(handle);


This won't abort an existing handle.

Elaborate please?

Leonid Amirov added 4 commits March 24, 2026 21:51

feat(types): ChainConfig: add confirmation_depth and default wiring

1e5d732

feat(ethereum): listener: make poller main source of truth and respec…

cca0044

…t confirmation_depth

feat(ethereum): listener: store hints from subscriber and use them to…

36fb136

… fast-track path in poller

chore(tests): remove subscription-only mode Ethereum test

03284c9

Leonid Amirov added 4 commits March 25, 2026 22:33

feat(types): ChainConfig: use 3-member enum for confirmation depth se…

de6b6db

…tting

feat(tests): ethereum e2e: implement test with confirmation_depth=1

4f54740

feat(tests): ethereum: cover subscriber hints

64e0467

feat(doc): updates supporting the recent changes

bc6bc97

side2k force-pushed the ethereum-bridge-4-reorgs branch from feaa983 to bc6bc97 Compare March 25, 2026 19:33

side2k requested a review from snoyberg March 25, 2026 19:47

side2k marked this pull request as ready for review March 26, 2026 09:15

snoyberg reviewed Mar 26, 2026

View reviewed changes

Leonid Amirov added 2 commits March 26, 2026 22:25

fix(ethereum): listener: ensure subscriber task is killed when poller…

acede36

… is done

feat(types): ChainConfig: revert to Option<u64> for confirmation_dept…

653fc63

…h setting This reverts commit de6b6db.

snoyberg reviewed Mar 27, 2026

View reviewed changes

feat(ethereum): listener: more robust guard for subscriber task

7a04403

Also: - do some refactoring and cleanup - separate Startup and Runtime errors from the subscriber - the former lead to subscriber restart for a limited amount of times, the latter do not

snoyberg reviewed Apr 5, 2026

View reviewed changes

Conversation

side2k commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloudflare-workers-and-pages Bot commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying kolme with Cloudflare Pages

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snoyberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

side2k commented Mar 25, 2026 •

edited

Loading

cloudflare-workers-and-pages Bot commented Mar 25, 2026 •

edited

Loading