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

adds evm_client wait for transaction confirmation to QGB #354

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Apr 26, 2022

Adds a simple waiting mechanism to wait for evm client transaction confirmation before sending the next transaction.
Closes #319

@rach-id rach-id self-assigned this Apr 26, 2022
@rach-id rach-id added the C: QGB label Apr 26, 2022
@rach-id rach-id changed the title adds evm_client wait for transaction confirmation adds evm_client wait for transaction confirmation to QGB Apr 26, 2022
ec.logger.Info("ValSetUpdate", tx.Hash().String())

// TODO put this in a separate function and listen for new EVM blocks instead of just sleeping
for i := 0; i < 60; i++ {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the magic 60?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was 6 at the beginning to wait for a minute. However, I am testing against rinkeby/ropsten, and they take a lot of time to include transactions in blocks. So, I changed it to 10 minutes of wait time.
In the future, when we have the worker pool design, we will change this by listening to new blocks and waiting for a number of confirmations before moving to the next attestation

Copy link
Member Author

@rach-id rach-id Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a QGB contract in action:
https://ropsten.etherscan.io/address/0x820a20eb3c8f7789557595874b89dd71fea11a0e
I am generating a new valset every 15 Celestia blocks.
Don't mind the failing transactions, as those are duplicates (will be fixed when we implement the new design)

PS: We still don't use the latest nonce => height change, and we're deploying an older version

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense for a temporary solution

👍

Comment on lines +93 to +105
// TODO put this in a separate function and listen for new EVM blocks instead of just sleeping
for i := 0; i < 60; i++ {
ec.logger.Debug(fmt.Sprintf("waiting for valset %d to be confirmed: %s", nonce, tx.Hash().String()))
lastNonce, err := ec.StateLastValsetNonce(&bind.CallOpts{Context: ctx})
if err != nil {
return err
}
if lastNonce == nonce {
ec.logger.Info(fmt.Sprintf("relayed valset %d: %s", nonce, tx.Hash().String()))
return nil
}
time.Sleep(10 * time.Second)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as stated in the todo, I like the idea of a separate evm client method that waits for a specific update or something similar

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's write that down in an issue if we haven't already tho

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can create a generic method that subscribes to new blocks and checks if a transaction hash is included or not.
Could be something like:

func (ec *EvmClient) waitForConfirmations(txHash string, numberOfConfirmations int) error

I wrote some snippets, but it would take much time debugging and making it work. Especially that subscribing to events requires using a web socket connection and that needs to be added to the config which itself needs to be updated to reflect the different commands.
Thus, I decided to leave it for later and do this instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: #356

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if we can merge.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@rach-id rach-id merged commit 1534631 into celestiaorg:qgb-integration Apr 26, 2022
@rach-id rach-id deleted the evm_client_wait_for_tx_confirmation branch April 26, 2022 17:02
rach-id added a commit to rach-id/celestia-app that referenced this pull request May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

The QGB relayer should be able to wait for the transactions to be confirmed before moving to the next one
3 participants