Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: egress id race condition #4235

Merged
merged 12 commits into from
Nov 16, 2023
Merged

fix: egress id race condition #4235

merged 12 commits into from
Nov 16, 2023

Conversation

kylezs
Copy link
Contributor

@kylezs kylezs commented Nov 13, 2023

Pull Request

Closes: PRO-689

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

See the issue. This fixes it, by unifying the deposit_addresses adapter and the egress_items adapter under the hood. But still provides a wrapper for ease of use for each of these.

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Attention: 282 lines in your changes are missing coverage. Please review.

Comparison is base (f6a06c1) 71% compared to head (8df66b7) 71%.
Report is 4 commits behind head on main.

Files Patch % Lines
...d_chain_source/chunked_by_vault/monitored_items.rs 0% 192 Missing ⚠️
...nked_chain_source/chunked_by_vault/egress_items.rs 0% 43 Missing ⚠️
...chain_source/chunked_by_vault/deposit_addresses.rs 0% 34 Missing ⚠️
engine/src/witness/dot.rs 0% 10 Missing ⚠️
engine/src/witness/btc.rs 86% 3 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #4235    +/-   ##
======================================
- Coverage     71%     71%    -0%     
======================================
  Files        384     385     +1     
  Lines      63007   63151   +144     
  Branches   63007   63151   +144     
======================================
+ Hits       45050   45071    +21     
- Misses     15617   15741   +124     
+ Partials    2340    2339     -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kylezs kylezs force-pushed the fix/egress-id-race-condition branch from 3489695 to d879ea7 Compare November 13, 2023 12:21
Copy link

linear bot commented Nov 13, 2023

PRO-689 Egress Id race condition

It's possible we're not witnessing a particular egress id / TransactionOutId at the time we receive the block that contains the egress transaction we should be watching for.

Copy of the TODO in the code:

// NB: There is a race condition here. If we're not watching for a particular egress id (because our state chain is slow for some reason) at the time
// it arrives on external chain, we won't witness it. This is pretty unlikely since the time between the egress id being set on the SC and the tx
// being confirmed on the external chain is quite large. We should fix this eventually though.

DOT and BTC egress witnessing need to know the set of transactions they are looking for, and therefore it is possible for the witnessers to be ahead (even with lag safety), and miss the block the transaction is included in, because it hasn't seen the SC block that contains the BroadcastRequest event that contains the details required to witness it.

Solution

The solution is to attach a chain tracking block number to the broadcast before the threshold signing request. This is because after a threshold signing is done, it's possible to broadcast the transaction.

We can then "hold up" Ethereum (for example) blocks until we know that we can't possibly have received an address to watch for before that Ethereum block.

First PR, the SC side: #4046

The second part of this is to effectively implement the deposit_addresses adapter for egress ids… and factor out the common code in the process.

@kylezs kylezs changed the title Fix/egress id race condition fix: egress id race condition Nov 13, 2023
@kylezs kylezs marked this pull request as ready for review November 14, 2023 08:20
@kylezs kylezs force-pushed the fix/egress-id-race-condition branch from 7e58ca0 to 8df66b7 Compare November 14, 2023 13:45
@kylezs kylezs merged commit 4e9d18a into main Nov 16, 2023
40 checks passed
@kylezs kylezs deleted the fix/egress-id-race-condition branch November 16, 2023 13:57
dandanlen pushed a commit that referenced this pull request Nov 17, 2023
tomjohnburton pushed a commit that referenced this pull request Nov 20, 2023
Co-authored-by: dandanlen <3168260+dandanlen@users.noreply.github.com>
Co-authored-by: Alastair Holmes <42404303+AlastairHolmes@users.noreply.github.com>
Co-authored-by: Albert Llimos <53186777+albert-llimos@users.noreply.github.com>
Co-authored-by: Martin Rieke <121793148+martin-chainflip@users.noreply.github.com>
Co-authored-by: Maxim Shishmarev <msgmaxim@gmail.com>
Co-authored-by: Marcello <marcello@chainflip.io>
Co-authored-by: Roy Yang <roy@chainflip.io>
Co-authored-by: kylezs <kyle@chainflip.io>
Co-authored-by: Jamie Ford <jamie@chainflip.io>
fix naming of session keys (#4242)
fix: revert restricted balances (#4237)
fix: add missing spans in multisig logs (#4239)
fix cargo features (#4249)
fix: remove bound addresses on account deletion (#4244)
fix: remove existential deposit (#4243)
fix: egress id race condition (#4235)
fix: remove unwrap when getting tx receipt (#4231)
fix: protect against double witnessing after safe mode (#4254)
fix: runtime upgrade utils and migrations (#4258)
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.

None yet

2 participants