omni / poa-bridge

POA <-> Ethereum bridge for self transfers of POA native token to POA20 (ERC20 representation). Not supported. Use TokenBridge instead
https://github.com/poanetwork/token-bridge
GNU General Public License v3.0
79 stars 38 forks source link

Problem: bridge crashes with insufficient balance #36

Closed yrashk closed 6 years ago

yrashk commented 6 years ago

Currently there are two possible situations related to low balance on the account which is used for bridge operations:

  1. The account which is used to sign transactions to be addressed by ForeignBridge contract has low balance. So, the bridge is not able to do deposit_relay and withdraw_confirm.
  2. The account which is used to sign transactions to be addressed by HomeBridge contract has low balance. So, the bridge is not able to do withdraw_relay.

In both cases bridges hangs silently at the moment of sending transactions and does not proceed with further actions even the operation is intended to be performed in opposite direction (e.g. the bridge hangs at the moment to perform withdraw_relay, so deposit_relay cannot be performed either).

Solution: make bridge track its balance and hande insufficient

Bridge will crash with ERR_INSUFFICIENT_FUNDS (code 4) so that supervisor can decide what should happen next. It will also log the condition.

P.S.Make sure to run the tests with --test-threads=1 to avoid other test conflicting with this one. A better solution to this issue must be devised later, however.

akolotov commented 6 years ago

@yrashk could you elaborate where --test-threads configuration option is from? When you said that it is needed to run tests, does it mean that it is for integration tests only?

akolotov commented 6 years ago

@yrashk I have built bridge binary from your branch and run the bridge. Just after the contract deployment I see the warning message foreign contract balance is unknown:

INFO:bridge: Parsing cli arguments
INFO:bridge: Loading config
INFO:bridge: Starting event loop
INFO:bridge: Establishing ipc connection
INFO:bridge: Deploying contracts (if needed)
INFO:bridge: Deployed new bridge contracts
INFO:bridge:

home_contract_address = "0xa3ccafacef5b5f8765830a4050d5ef32a91e2bf4"
foreign_contract_address = "0xd0bf5d636bc66538e52b840ae9ae7a4a860fd727"
home_deploy = 1
foreign_deploy = 1
checked_deposit_relay = 1
checked_withdraw_relay = 1
checked_withdraw_confirm = 1

INFO:bridge: Starting listening to events
WARN:bridge::bridge::deposit_relay: foreign contract balance is unknown

Is it expectable behavior?

yrashk commented 6 years ago

This may indeed appear in some cases, however, the parts that log this won't progress any further until the balance is retrieved. The only indication that things went wrong there would be if, say, deposit today never commences it's operations. I can also improve logging here and log when balance transitions from unknown to known.

yrashk commented 6 years ago

As for test threads, this is only applicable to running integration tests.

akolotov commented 6 years ago

@yrashk I've got you. It would be great if we mention somehow in the logs that it is not a real issue.

akolotov commented 6 years ago

@yrashk I am getting the warning message every time I run the bridge, even if contracts are already deployed, even just after the bridge interrupt.

INFO:bridge: Parsing cli arguments
INFO:bridge: Loading config
INFO:bridge: Starting event loop
INFO:bridge: Establishing ipc connection
INFO:bridge: Deploying contracts (if needed)
INFO:bridge: Loaded database
INFO:bridge: Starting listening to events
WARN:bridge::bridge::deposit_relay: foreign contract balance is unknown

Think we need fix it in order to avoid incorrect understaning of such message.

My suggestion is to fix it in separate issue - i am going to merge this PR.