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

eth,misc: Use transaction manager with confirmation and optional replacement queue #1923

Merged
merged 5 commits into from Jul 15, 2021

Conversation

kyriediculous
Copy link
Contributor

@kyriediculous kyriediculous commented Jun 17, 2021

What does this pull request do? Explain your changes. (required)
Use a TransactionManager to handle transactions. This service will forward transactions to the ethereum JSON-RPC, when a transaction is broadcasted to the ethereum network it will be added to the queue. Lowest non-confirmed transactions are popped off the queue to wait until they are mined and retrieve their transaction receipt.

Once the transaction receipt is found it will be sent over a subscription of transaction receipts that services that want feedback on tx confirmation can subscribe to. This subscription service now powers LivepeerEthClient.CheckTx() under the hood.

Specific updates (required)

  • 75eed97 eth: transaction manager with confirmation queue and optional replacement transactions

  • 60d33a0 pm: handle ticket revert error

  • a7b7c5e eth: remove replacement transactions from reward service

  • d4886f6 eth,server: move max gas price logic to gas price monitor

  • b12a24d eth: make abiMap and txLog helpers in the eth package rather than Backend methods

  • 1b89b5c cmd: replace transaction flag

  • efa84b4 cmd,eth: use struct for LivepeerEthClient constructor parameters that aren't other services

How did you test each of these updates (required)

  • Unit tests

  • TODO add TransactionManager unit tests

Does this pull request close any open issues?
Fixes #1906

Checklist:

eth/backend.go Outdated Show resolved Hide resolved
@kyriediculous kyriediculous changed the title [WIP] eth,misc: Use transaction manager with confirmation and optional replacement queue eth,misc: Use transaction manager with confirmation and optional replacement queue Jun 18, 2021
@kyriediculous kyriediculous marked this pull request as ready for review June 18, 2021 02:14
@kyriediculous kyriediculous changed the base branch from master to nv/tx-fixes June 18, 2021 02:45
cmd/livepeer/livepeer.go Outdated Show resolved Hide resolved
pm/queue.go Outdated Show resolved Hide resolved
eth/backend.go Outdated Show resolved Hide resolved
eth/client.go Outdated Show resolved Hide resolved
eth/transactionManager.go Outdated Show resolved Hide resolved
eth/transactionManager.go Outdated Show resolved Hide resolved
eth/client.go Outdated Show resolved Hide resolved
eth/client.go Outdated Show resolved Hide resolved
Base automatically changed from nv/tx-fixes to master June 30, 2021 15:46
@yondonfu yondonfu mentioned this pull request Jun 30, 2021
5 tasks
@kyriediculous
Copy link
Contributor Author

@yondonfu Do we want persistent storage for the transaction confirmation queue to avoid unexpected behaviour between node restarts when transactions are still pending ?

Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

How were these changes tested?

Given that the logic of the transaction manager for handling replacing pending transactions is important for funds management and node operation could we add unit tests particularly around the automated transaction replacement flow?

eth/client.go Outdated Show resolved Hide resolved
eth/gaspricemonitor.go Show resolved Hide resolved
eth/client.go Outdated Show resolved Hide resolved
eth/transactionManager.go Outdated Show resolved Hide resolved
eth/transactionManager.go Show resolved Hide resolved
eth/client.go Outdated Show resolved Hide resolved
eth/transactionManager.go Outdated Show resolved Hide resolved
eth/transactionManager.go Outdated Show resolved Hide resolved
pm/sendermonitor.go Outdated Show resolved Hide resolved
cmd/devtool/devtool.go Outdated Show resolved Hide resolved
cmd/devtool/devtool.go Outdated Show resolved Hide resolved
cmd/devtool/devtool.go Outdated Show resolved Hide resolved
cmd/livepeer/livepeer.go Outdated Show resolved Hide resolved
cmd/livepeer/livepeer.go Outdated Show resolved Hide resolved
eth/transactionManager.go Outdated Show resolved Hide resolved
eth/transactionManager.go Outdated Show resolved Hide resolved
eth/transactionManager.go Outdated Show resolved Hide resolved
eth/transactionManager.go Outdated Show resolved Hide resolved
pm/sendermonitor.go Outdated Show resolved Hide resolved
cmd/livepeer/livepeer.go Outdated Show resolved Hide resolved
eth/transactionManager.go Show resolved Hide resolved
eth/transactionManager.go Outdated Show resolved Hide resolved
eth/transactionManager.go Outdated Show resolved Hide resolved
eth/transactionManager.go Outdated Show resolved Hide resolved
eth/transactionManager_test.go Show resolved Hide resolved
eth/transactionManager_test.go Outdated Show resolved Hide resolved
eth/transactionManager_test.go Show resolved Hide resolved
eth/transactionManager.go Show resolved Hide resolved
eth/rewardservice.go Show resolved Hide resolved
Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

Just two comments to address:

Let's also rebase on top of master to resolve the conflicts. Feel free to do this in conjunction with addressing the remaining comments.

@kyriediculous
Copy link
Contributor Author

Rebased, resolved merge conflicts and addresses the remaining changes.

Also moved min and max gas price completely into GasPriceMonitor as discussed previously.

eth/backend.go Show resolved Hide resolved
eth/gaspricemonitor.go Show resolved Hide resolved
eth/gaspricemonitor.go Show resolved Hide resolved
Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

LGTM after squashing 🚢

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

Successfully merging this pull request may close these issues.

Failed gas estimation with revert for txs when another tx affecting the same state is pending
2 participants