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

Using blockNumber - 1 instead of block.parentHash is a subject to reorg false alarms #513

Open
TheDZhon opened this issue Apr 5, 2024 · 4 comments
Assignees

Comments

@TheDZhon
Copy link
Contributor

TheDZhon commented Apr 5, 2024

tl;dr

When the alert logic requires to query something from the previous block, it's better to use the block.parentHash instead of blockNumber - 1 to avoid issues with the Ethereum chain reorganizations.

How to fix

See the following reference code:
https://github.com/lidofinance/alerting-forta/blob/main/ethereum-steth/src/clients/eth_provider.ts#L181-L196

  public async getBalanceByBlockHash(address: string, blockHash: string): Promise<E.Either<Error, BigNumber>> {
    try {
      const out = await retryAsync<EtherBigNumber>(
        async (): Promise<EtherBigNumber> => {
          return await this.jsonRpcProvider.getBalance(address, {
            blockHash: blockHash,
          } as never)
        },
        { delay: DELAY_IN_500MS, maxTry: ATTEMPTS_5 },
      )

      return E.right(new BigNumber(String(out)))
    } catch (e) {
      return E.left(new NetworkError(e, `Could not fetch balance by address ${address} and blockHash ${blockHash}`))
    }
  }

https://github.com/lidofinance/alerting-forta/blob/main/ethereum-steth/src/services/vault/Vault.srv.ts#L60-L63

const prevBlockWithdrawalVaultBalance = await this.ethProvider.getBalanceByBlockHash(
  this.withdrawalsVaultAddress,
  blockEvent.block.parentHash,
)

Affected scope

List of the files that affected by the issue:

grep --include=\*.ts -rnw '.' -e ".*umber - 1" --exclude-dir=node_modules

./ethereum-validators-set/src/subagents/lido-report/agent-lido-report.ts:841:    blockTag: txEvent.blockNumber - 1,
./ethereum-validators-set/src/subagents/exitbus-oracle/agent-exitbus-oracle.ts:283:      blockEvent.blockNumber - 1,
./ethereum-validators-set/src/subagents/accounting-oracle/agent-accounting-oracle.ts:140:      blockEvent.blockNumber - 1,
./ethereum-validators-set/src/subagents/accounting-oracle/agent-accounting-oracle.ts:148:        blockEvent.blockNumber - 1,
./l2-bridge-linea/src/clients/linea_block_client.ts:72:      const blocks = await this.provider.fetchBlocks(this.cachedBlockDto.number, latestBlock.right.number - 1)
./l2-bridge-mantle/src/clients/mantle_block_client.ts:72:      const blocks = await this.mantleProvider.fetchBlocks(this.cachedBlockDto.number, latestBlock.right.number - 1)
./voting-watcher/src/agent-voting-watcher.ts:156:  const prevBlock = await ethersProvider.getBlock(blockEvent.blockNumber - 1);
./ethereum-steth/src/services/steth_operation/StethOperation.srv.ts:230:      this.ethProvider.getBufferedEther(shiftedBlockNumber - 1),
./l2-bridge-base/src/services/monitor_withdrawals.ts:54:    const withdrawalEvents = await this.withdrawalsClient.getWithdrawalEvents(pastl2Block, l2BlockNumber - 1)
./l2-bridge-base/src/clients/base_block_client.ts:72:      const blocks = await this.provider.fetchL2Blocks(this.cachedBlockDto.number, latestBlock.right.number - 1)
./ethereum-financial/src/services/aave/Aave.srv.ts:63:        this.ethProvider.getStethBalance(this.aaveAstethAddress, blockEvent.number - 1),
./ethereum-financial/src/services/aave/Aave.srv.ts:64:        await this.ethProvider.getTotalSupply(blockEvent.number - 1),
./ethereum-financial/src/services/aave/Aave.srv.ts:101:              `${blockEvent.number - 1}`,
./l2-bridge-zksync/src/clients/zksync_block_client.ts:72:      const blocks = await this.provider.fetchBlocks(this.cachedBlockDto.number, latestBlock.right.number - 1)

Explanations and references

References

Helpful picture

image

@TheDZhon TheDZhon changed the title Using blockNumber - 1 instead of blockEvent.block.parentHash is a subject to reorg false alarms Using blockNumber - 1 instead of block.parentHash is a subject to reorg false alarms Apr 5, 2024
@sergeyWh1te
Copy link
Contributor

@sergeyWh1te
Copy link
Contributor

sergeyWh1te commented Apr 9, 2024

  • ./ethereum-validators-set/src/subagents/lido-report/agent-lido-report.ts:841: blockTag: txEvent.blockNumber - 1,
  • ./ethereum-validators-set/src/subagents/exitbus-oracle/agent-exitbus-oracle.ts:283: blockEvent.blockNumber - 1,
  • ./ethereum-validators-set/src/subagents/accounting-oracle/agent-accounting-oracle.ts:140: blockEvent.blockNumber - 1,
  • ./ethereum-validators-set/src/subagents/accounting-oracle/agent-accounting-oracle.ts:148: blockEvent.blockNumber - 1,
  • ./l2-bridge-linea/src/clients/linea_block_client.ts:72: const blocks = await this.provider.fetchBlocks(this.cachedBlockDto.number, latestBlock.right.number - 1)
  • ./l2-bridge-mantle/src/clients/mantle_block_client.ts:72: const blocks = await this.mantleProvider.fetchBlocks(this.cachedBlockDto.number, latestBlock.right.number - 1)
  • ./voting-watcher/src/agent-voting-watcher.ts:156: const prevBlock = await ethersProvider.getBlock(blockEvent.blockNumber - 1);
  • ./ethereum-steth/src/services/steth_operation/StethOperation.srv.ts:230: this.ethProvider.getBufferedEther(shiftedBlockNumber - 1),
  • ./l2-bridge-base/src/services/monitor_withdrawals.ts:54: const withdrawalEvents = await this.withdrawalsClient.getWithdrawalEvents(pastl2Block, l2BlockNumber - 1)
  • ./l2-bridge-base/src/clients/base_block_client.ts:72: const blocks = await this.provider.fetchL2Blocks(this.cachedBlockDto.number, latestBlock.right.number - 1)
  • ./ethereum-financial/src/services/aave/Aave.srv.ts:63: this.ethProvider.getStethBalance(this.aaveAstethAddress, blockEvent.number - 1),
  • ./ethereum-financial/src/services/aave/Aave.srv.ts:64: await this.ethProvider.getTotalSupply(blockEvent.number - 1),
  • ./ethereum-financial/src/services/aave/Aave.srv.ts:101: ${blockEvent.number - 1},
  • ./l2-bridge-zksync/src/clients/zksync_block_client.ts:72: const blocks = await this.provider.fetchBlocks(this.cachedBlockDto.number, latestBlock.right.number - 1)

sergeyWh1te added a commit that referenced this issue Apr 9, 2024
…parentHash is a subject to reorg false alarms
sergeyWh1te added a commit that referenced this issue Apr 9, 2024
…parentHash is a subject to reorg false alarms
sergeyWh1te added a commit that referenced this issue Apr 9, 2024
…parentHash is a subject to reorg false alarms
sergeyWh1te added a commit that referenced this issue Apr 9, 2024
…parentHash is a subject to reorg false alarms
sergeyWh1te added a commit that referenced this issue Apr 9, 2024
fix: issue #513 in eth-steth: Using blockNumber - 1 instead of block.…
@sergeyWh1te
Copy link
Contributor

sergeyWh1te commented Apr 9, 2024

For ./l2-bridge-zksync/src/clients/zksync_block_client.ts:72: const blocks = await this.provider.fetchBlocks(this.cachedBlockDto.number, latestBlock.right.number - 1)

/l2-bridge-linea/src/clients/linea_block_client.ts:72: const blocks = await this.provider.fetchBlocks(this.cachedBlockDto.number, latestBlock.right.number - 1)

./l2-bridge-mantle/src/clients/mantle_block_client.ts:72: const blocks = await this.mantleProvider.fetchBlocks(this.cachedBlockDto.number, latestBlock.right.number - 1)

For const withdrawalEvents = await this.withdrawalsClient.getWithdrawalEvents(pastl2Block, l2BlockNumber - 1)
if (E.isLeft(withdrawalEvents)) {
return withdrawalEvents
}

./ethereum-validators-set/src/subagents/exitbus-oracle/agent-exitbus-oracle.ts:283: blockEvent.blockNumber - 1,
./ethereum-validators-set/src/subagents/accounting-oracle/agent-accounting-oracle.ts:140: blockEvent.blockNumber - 1,
./ethereum-validators-set/src/subagents/accounting-oracle/agent-accounting-oracle.ts:148: blockEvent.blockNumber - 1,

It's ok - code fetches a range of blocks.

where we really need to fix:

  • ./ethereum-validators-set/src/subagents/lido-report/agent-lido-report.ts:841: blockTag: txEvent.blockNumber - 1,
  • ./voting-watcher/src/agent-voting-watcher.ts:156: const prevBlock = await ethersProvider.getBlock(blockEvent.blockNumber - 1);

@lidofinance/lido-tooling
@lidofinance/lido-dao-ops-team
Pls, look at issue...

@TheDZhon
Copy link
Contributor Author

Turns out it's not possible to use the blockHeader for the API requests at least for some services, the workaround is in #529 (tl;dr: grab the block object by the parent.blockHash and still use block.number)

More context: ethers-io/ethers.js#2808

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

4 participants