boyuan-chen commented 3 years ago

Currently, the YNATM package is responsible for gas price logic. It starts with a low value, and gradually increments the gas price until the messages get through. Unfortunately, this can go wrong, resulting in stuck transactions that do not get relayed promptly.

In this scenario, the gas price (1) increases to above maximum_value, and (2) stays above that value for longer than timeout, resulting in a reject error, which we presently don't handle automatically, but rather, manually.

The goal of these changes is to automatically deal with the rejected messages, by keeping track of them in a buffer, and relaying them until the buffer is empty.

codecov[bot] commented 3 years ago

Codecov Report

Merging #558 (b8e6191) into develop (9c4d5c4) will decrease coverage by 0.02%. The diff coverage is 50.00%.

@@             Coverage Diff             @@
##           develop     #558      +/-   ##
===========================================
- Coverage    76.10%   76.07%   -0.03%     
===========================================
  Files           81       81              
  Lines         3038     3039       +1     
  Branches       469      469              
===========================================
  Hits          2312     2312              
- Misses         726      727       +1

Flag	Coverage Δ
batch-submitter	`59.59% <100.00%> (ø)`
contracts	`86.00% <0.00%> (-0.05%)`	:arrow_down:
core-utils	`68.16% <ø> (ø)`
data-transport-layer	`37.68% <ø> (ø)`
message-relayer	`72.22% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...VM/bridge/messaging/OVM_L1CrossDomainMessenger.sol	`95.65% <0.00%> (-1.41%)`	:arrow_down:
...ubmitter/src/batch-submitter/tx-batch-submitter.ts	`42.48% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9c4d5c4...b8e6191. Read the comment docs.

souradeep-das commented 3 years ago

for this batch overwrite issue, would making use of the waitForTransaction(), numConfirmations( L732) work instead?

boyuan-chen commented 3 years ago

Not that easy. The gas is difficult to guess due to the volatility. The ynatm packages has the maximum gas price. If the gas price suddenly increases to more than the maximum value we set, we want to wait until the gas price goes down. Therefore, the ynatm package has the maximum waiting time and it returns the reject error. The message relayer will just skip those messages.

    const finalTimeoutId = setTimeout(() => {
      reject(new Error("Transaction taking too long!"));
    }, sanitizeTimeout((gasPrices.length + 1) * delay));
    timeoutIds.push(finalTimeoutId);

omgnetwork / optimism

Fix message relayers and check gas price #558

Codecov Report