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

uses evm tool via server rather than proccess forks #66

Merged
merged 6 commits into from
Mar 1, 2023

Conversation

sudeepdino008
Copy link
Contributor

@sudeepdino008 sudeepdino008 commented Feb 9, 2023

Tested this with the No_such_log fix PR and it was working smooth. Note that this depends on covalenthq/erigon#11 getting merged.
Need to fix testing and any Dockerfile for this.

@sudeepdino008 sudeepdino008 marked this pull request as ready for review February 14, 2023 13:07
@sudeepdino008 sudeepdino008 requested review from noslav and kitti-katy and removed request for noslav February 14, 2023 13:07
@sudeepdino008 sudeepdino008 force-pushed the evm_server branch 2 times, most recently from 32cf891 to 4626657 Compare February 14, 2023 13:11
@sudeepdino008 sudeepdino008 force-pushed the evm_server branch 3 times, most recently from 8ed0ed3 to 1a1a8a2 Compare February 17, 2023 05:26
@noslav noslav changed the base branch from main to develop February 22, 2023 19:15
Copy link
Member

@noslav noslav left a comment

Choose a reason for hiding this comment

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

This is a pretty big PR and it also introduces a fork from the "binary as a pipeline transformer plugin" direction we had before. I would suggest when we merge this after the comments and some corrections are addressed - and that we release the v0.2.0 of rudder, because of the departure from the earlier build direction or support both in the future - running the transformer binaries on bsps through the porcelain process and/or running the http servers as externally dependent services, it would complicate things a bit and so how to do that of course I would leave to you!

.env_example Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
@@ -1,10 +0,0 @@
ERIGON_SUBMODULE=erigon
Copy link
Member

Choose a reason for hiding this comment

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

I think we should still have a makefile for rudder setup where we can consolidate different calls to the iex like loading dependencies etc.

README.md Show resolved Hide resolved
lib/rudder/proof_chain/block_specimen_event_listener.ex Outdated Show resolved Hide resolved
mix.exs Show resolved Hide resolved
test/evm/evm_test.exs Show resolved Hide resolved
test/evm/evm_test.exs Show resolved Hide resolved
test/evm/evm_test.exs Show resolved Hide resolved
@noslav
Copy link
Member

noslav commented Feb 22, 2023

@sudeepdino008 thanks for fixing the CI along with this, btw I noticed some performance drops from using the evm server opposed to the plugin, what do you think we can do about this?

use as plugin

rudder        | 17:23:58.972 [info] Counter for bsp_metrics - [execute: 3]
rudder        | 17:23:58.972 [info] LastValue for bsp_metrics - [execute_last_exec_time: 1.65e-4]
rudder        | 17:23:58.972 [info] Sum for bsp_metrics - [execute_total_exec_time: 9.919999999999998e-4]
rudder        | 17:23:58.972 [info] Summary for bsp_metrics  - ***1.65e-4, 4.4199999999999996e-4***

use as server

rudder        | 13:22:58.696 [info] Counter for bsp_metrics - [execute: 3]
rudder        | 13:22:58.696 [info] LastValue for bsp_metrics - [execute_last_exec_time: 5.909999999999999e-4]
rudder        | 13:22:58.696 [info] Sum for bsp_metrics - [execute_total_exec_time: 0.001382]
rudder        | 13:22:58.696 [info] Summary for bsp_metrics  - ***3.4399999999999996e-4, 5.909999999999999e-4***

anything we can do to make it up?

@sudeepdino008
Copy link
Contributor Author

This is a pretty big PR and it also introduces a fork from the "binary as a pipeline transformer plugin" direction we had before. I would suggest when we merge this after the comments and some corrections are addressed - and that we release the v0.2.0 of rudder, because of the departure from the earlier build direction or support both in the future - running the transformer binaries on bsps through the porcelain process and/or running the http servers as externally dependent services, it would complicate things a bit and so how to do that of course I would leave to you!

running both would be unadvisable - the cost of maintaining and following the documentation and code is too big to justify keeping both approaches in the repo.

@sudeepdino008
Copy link
Contributor Author

good catch on the perf issues. I'm however comfortable with how we have here. We should analyze the pipeline later when there's a need for it; right now in this PR, the only thing that changed was http overhead instead of process forks. I'll nevertheless explore some simple things which should fix evm server perf and post a separate PR for that.

@sudeepdino008
Copy link
Contributor Author

@kitti-katy reverted to defp
I'm fine with the current state of check in listener. The thing tha tmight go wrong is if the "isSessionOpen" check fails and so error is reported, then doesn't that stop the block specimen event listener? Seems like it should continue listening (and maybe have 3 restarts allowed in 2 minutes) and not error out and stop.

@kitti-katy
Copy link
Contributor

@kitti-katy reverted to defp I'm fine with the current state of check in listener. The thing tha tmight go wrong is if the "isSessionOpen" check fails and so error is reported, then doesn't that stop the block specimen event listener? Seems like it should continue listening (and maybe have 3 restarts allowed in 2 minutes) and not error out and stop.

@sudeepdino008 there are other places where the listener makes web3 calls (to fetch logs and block height) which might fail if something is wrong with the node, I don't think isSession function will ever have a failing input (it just accesses storage vars), so it will fail if we made an upgrade on the contract side that does not match the old abi.

In the first case, the http adapter already makes 5 retries and in the second case if the contract is changed, I assume they will have to upgrade the rudder too.

@sudeepdino008
Copy link
Contributor Author

@kitti-katy reverted to defp I'm fine with the current state of check in listener. The thing tha tmight go wrong is if the "isSessionOpen" check fails and so error is reported, then doesn't that stop the block specimen event listener? Seems like it should continue listening (and maybe have 3 restarts allowed in 2 minutes) and not error out and stop.

@sudeepdino008 there are other places where the listener makes web3 calls (to fetch logs and block height) which might fail if something is wrong with the node, I don't think isSession function will ever have a failing input (it just accesses storage vars), so it will fail if we made an upgrade on the contract side that does not match the old abi.

In the first case, the http adapter already makes 5 retries and in the second case if the contract is changed, I assume they will have to upgrade the rudder too.

sounds good @kitti-katy

@sudeepdino008 sudeepdino008 merged commit 283b0e2 into develop Mar 1, 2023
@noslav noslav deleted the evm_server branch May 10, 2023 17:30
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.

None yet

3 participants