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

transaction: add nonce-based monitor #1465

Merged
merged 6 commits into from Apr 9, 2021
Merged

Conversation

ralph-pichler
Copy link
Member

@ralph-pichler ralph-pichler commented Mar 22, 2021

This is a pretty radical rewrite of how transactions are monitored for confirmations.

The primary aim is to be able to:

  • have a somewhat constant number of backend requests over time independent of the number of pending transactions
  • give up on transactions which are provably cancelled (while avoiding giving up on transactions which might still happen, even if they take a very long time to confirm)
  • provide the base for watching transactions in the background
  • provide the base for advanced transaction functionality such as gas boosting, replacing or rebroadcasting transactions

The way this works is as follows:

Instead of watching transactions individually, the senders nonce is monitored and transactions are checked based on this. The idea is that if the nonce is still lower than that of a pending transaction, there is no point in actually checking the transaction for a receipt. At the same time if the nonce was already used and this was a few blocks ago we can reasonably assume that it will never confirm.

A new component transaction.Monitor was introduced for that purpose (primarily because this could be instantiated even without a signer, and also to avoid the locks interfering with the locks of the transaction sending). However other components still wait using the transaction.Service as using the monitor directly requires knowing the nonce which only the transaction service does.

The transaction.Service is now keeping a record of all transactions it sent. In this PR this is only used to look up the nonce later, but in the future this will allow adding users to query past transaction info, query pending transactions and to enable the advanced functionality mentioned above. Because it now uses the monitor underneath the transaction.Service can now only wait for its own transactions, not arbitrary ones (but it was never used in that way anyway).

In order to test the monitor (which queries many blocks and nonces at different blocks in sequence) a new transaction.Backend mock was added which can be used in a more declarative way.

@ralph-pichler ralph-pichler self-assigned this Mar 22, 2021
@ralph-pichler ralph-pichler force-pushed the tx_watch_by_nonce branch 2 times, most recently from 6f58fce to 2e83af4 Compare March 22, 2021 17:00
@ralph-pichler ralph-pichler added the ready for review The PR is ready to be reviewed label Mar 22, 2021
@ralph-pichler ralph-pichler marked this pull request as ready for review March 22, 2021 17:19
Copy link
Member

@metacertain metacertain left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

Looks good. Requesting some changes regarding test timeouts (consider raising all of them so that the CI doesn't flake), context propagation can also be improved, with the context fields removed from the struct. I think we've done this before in other places and regretted it, it's better to just pass context from caller and create a root context in the forever loop. Also, some things can be improve as a result in terms of shutdown.

pkg/node/chain.go Outdated Show resolved Hide resolved
pkg/settlement/swap/transaction/monitor.go Outdated Show resolved Hide resolved
pkg/settlement/swap/transaction/monitor.go Show resolved Hide resolved
pkg/settlement/swap/transaction/monitor.go Outdated Show resolved Hide resolved
pkg/settlement/swap/transaction/monitor_test.go Outdated Show resolved Hide resolved
pkg/settlement/swap/transaction/monitor_test.go Outdated Show resolved Hide resolved
pkg/settlement/swap/transaction/monitor_test.go Outdated Show resolved Hide resolved
pkg/settlement/swap/transaction/monitor_test.go Outdated Show resolved Hide resolved
pkg/settlement/swap/transaction/monitormock/monitor.go Outdated Show resolved Hide resolved
@ralph-pichler ralph-pichler merged commit c41ae67 into master Apr 9, 2021
@ralph-pichler ralph-pichler deleted the tx_watch_by_nonce branch April 9, 2021 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull-request ready for review The PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants