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

Problem: potential loss of database updates #84

Closed
yrashk opened this issue May 22, 2018 · 0 comments
Closed

Problem: potential loss of database updates #84

yrashk opened this issue May 22, 2018 · 0 comments
Assignees

Comments

@yrashk
Copy link
Contributor

yrashk commented May 22, 2018

The way the core bridge module is structured there's a non-zero chance of loss of database updates that would cause it to redo transactions it already did.

Let's say we've got some database updates through deposit relaying:
https://github.com/poanetwork/poa-bridge/blob/master/bridge/src/bridge/mod.rs#L164-L165

Then, during relaying withdrawals, an error happened:
https://github.com/poanetwork/poa-bridge/blob/master/bridge/src/bridge/mod.rs#L171-L172

This means that we won't reach https://github.com/poanetwork/poa-bridge/blob/master/bridge/src/bridge/mod.rs#L185-L193 to save the result.

Also, in a similar vein, if one of these streams was to end (Ready(None)) we'd experience a similar loss of updates: https://github.com/poanetwork/poa-bridge/blob/master/bridge/src/macros.rs#L5

yrashk added a commit to yrashk/parity-bridge that referenced this issue May 26, 2018
chance of loss of database updates that would cause it to redo
transactions it already did.

Let's say we've got some database updates through deposit relaying:
https://github.com/poanetwork/poa-bridge/blob/master/bridge/src/bridge/mod.rs#L164-L165

Then, during relaying withdrawals, an error happened:
https://github.com/poanetwork/poa-bridge/blob/master/bridge/src/bridge/mod.rs#L171-L172

This means that we won't reach
https://github.com/poanetwork/poa-bridge/blob/master/bridge/src/bridge/mod.rs#L185-L193
to save the result.

Also, in a similar vein, if one of these streams was to end
(`Ready(None)`) we'd experience a similar loss of updates:
https://github.com/poanetwork/poa-bridge/blob/master/bridge/src/macros.rs#L5

Solution: refactor bridge into two streams, splitting responsibilities

One stream (`BridgeEventStream`) returns `BridgeChecked` and the other
(`Bridge`) writes those checks down to the database.

This way we're not accumulating chcecks before we serialize them,
risking not serializing them at all in the event of an unrelated error.

Fixes omni#84
@ghost ghost assigned yrashk May 26, 2018
@ghost ghost added the in progress label May 26, 2018
@yrashk yrashk closed this as completed in #95 Jun 4, 2018
@ghost ghost removed the in progress label Jun 4, 2018
noot pushed a commit to noot/poa-bridge that referenced this issue Jul 18, 2018
explain current functionality, next steps and relay flows in readme
noot pushed a commit to noot/poa-bridge that referenced this issue Jul 18, 2018
chance of loss of database updates that would cause it to redo
transactions it already did.

Let's say we've got some database updates through deposit relaying:
https://github.com/poanetwork/poa-bridge/blob/master/bridge/src/bridge/mod.rs#L164-L165

Then, during relaying withdrawals, an error happened:
https://github.com/poanetwork/poa-bridge/blob/master/bridge/src/bridge/mod.rs#L171-L172

This means that we won't reach
https://github.com/poanetwork/poa-bridge/blob/master/bridge/src/bridge/mod.rs#L185-L193
to save the result.

Also, in a similar vein, if one of these streams was to end
(`Ready(None)`) we'd experience a similar loss of updates:
https://github.com/poanetwork/poa-bridge/blob/master/bridge/src/macros.rs#L5

Solution: refactor bridge into two streams, splitting responsibilities

One stream (`BridgeEventStream`) returns `BridgeChecked` and the other
(`Bridge`) writes those checks down to the database.

This way we're not accumulating chcecks before we serialize them,
risking not serializing them at all in the event of an unrelated error.

Fixes omni#84
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

No branches or pull requests

1 participant