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

Orchestrator Async runtime improvements #274

Merged
merged 28 commits into from
Nov 16, 2021
Merged

Conversation

hannydevelop
Copy link
Contributor

No description provided.

@hannydevelop hannydevelop requested review from cbrit and EricBolten and removed request for levicook and zmanian October 27, 2021 18:43
@hannydevelop
Copy link
Contributor Author

async runtime in deploy/erc20.rs has been improved to use tokio::time. Here's what I suggest, the println! in the map for result should be an info! macro since it'll be easy to pick up and users can pick the message Your ERC20 contract was not adopted, double check the metadata and try again easily when there's an error.

@hannydevelop
Copy link
Contributor Author

continue basically terminates the current iteration, returning control to the loop head, typically continuing with the next iteration. However, continue can't be used in async block. I removed continuebecause I am usingtokio::join. This is in orchestrator/src/main_loop.rs`. This is still a work in progress, I'll edit it once I figure things out

@hannydevelop
Copy link
Contributor Author

I changed while loop to loop in happy_path_v2.rs. I also added a match arm for Err and Ok

@hannydevelop
Copy link
Contributor Author

The last commit is for transaction_stress_test.rs, made a mistake in the commit message. I added a loop just like happy_path.rs

@cbrit cbrit linked an issue Nov 5, 2021 that may be closed by this pull request
Copy link
Contributor

@EricBolten EricBolten left a comment

Choose a reason for hiding this comment

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

Left some feedback. In particular I think I might disagree with the auditor's suggestion for the main loops, so perhaps we should discuss that.

orchestrator/orchestrator/src/main_loop.rs Outdated Show resolved Hide resolved
orchestrator/gorc/src/commands/deploy/erc20.rs Outdated Show resolved Hide resolved
(Ok(_latest_eth_block), Ok(ChainStatus::Syncing)) => {
warn!("Cosmos node syncing, Eth oracle paused");
delay_for(DELAY).await;
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

In the old version of this loop, we could fail faster (in this case only waiting DELAY instead of ETH_ORACLE_LOOP_SPEED) and try from the beginning of the loop again. In the tokio join! model we'll always wait the full ETH_ORACLE_LOOP_SPEED before starting again. I know that the task joining model was suggested by the auditor, and it is cleaner, but probably performs worse in cases where we'd rather restart the loop. Based on the comments at the bottom around the elapsed time calculation, we might care more about performance than wall clock consistency.

Copy link
Contributor Author

@hannydevelop hannydevelop Nov 6, 2021

Choose a reason for hiding this comment

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

@cbrit, the problem here isn't sleep, rather it's join. With join, we have to wait the full ETH_ORACLE_LOOP_SPEED before starting again. However, the previous code had continue.

Copy link
Member

@cbrit cbrit Nov 7, 2021

Choose a reason for hiding this comment

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

@hannydevelop I know. I am suggesting that interval might be a better solution than join because it allows us to continue the loop while still making sure the loop only runs every ETH_ORACLE_LOOP_SPEED seconds if the match is a successful case.

The auditor said

using Instant::now() leads to non-exact timeouts and loop_speed because the checks are always if the elapsed time is greater or lesser than a fixed Duration"

but this is solved by interval.

As far as performance, I am not sure how it differs from join, or how much it really matters.

Copy link
Contributor Author

@hannydevelop hannydevelop Nov 7, 2021

Choose a reason for hiding this comment

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

In my opinion, I'll rather use timeout and sleep or return back to using Instant::now. Let's discuss this on Monday, I can try to contact the auditor for the logic behind using join in main_loop

orchestrator/test_runner/src/happy_path_v2.rs Outdated Show resolved Hide resolved
current_eth_valset_nonce = get_valset_nonce(gravity_address, *MINER_ADDRESS, &web30)
.await
.expect("Failed to get current eth valset");
tokio::time::sleep(std::time::Duration::from_secs(4)).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same use statement stuff around delay_for and Duration in happy_path and happy_path_v2 as in the earlier files in the changeset, just noting it here once rather than highlighting each instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This commit: fc227ac fixes this, thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, what I meant was rather that we can include use std::time::Duration and then when we call sleep, just refer to it like tokio::time::sleep(Duration::from_secs(...)), since there is no ambiguity to require fully qualifying it inline.

orchestrator/test_runner/src/transaction_stress_test.rs Outdated Show resolved Hide resolved
orchestrator/test_runner/src/happy_path_v2.rs Outdated Show resolved Hide resolved
orchestrator/test_runner/src/transaction_stress_test.rs Outdated Show resolved Hide resolved
orchestrator/test_runner/src/transaction_stress_test.rs Outdated Show resolved Hide resolved
orchestrator/orchestrator/src/main_loop.rs Outdated Show resolved Hide resolved
orchestrator/orchestrator/src/main_loop.rs Outdated Show resolved Hide resolved
orchestrator/test_runner/src/happy_path_v2.rs Outdated Show resolved Hide resolved
@PeggyJV PeggyJV deleted a comment from cbrit Nov 7, 2021
Copy link
Contributor

@EricBolten EricBolten left a comment

Choose a reason for hiding this comment

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

Left some feedback on the updates.

orchestrator/orchestrator/src/main_loop.rs Outdated Show resolved Hide resolved
orchestrator/orchestrator/src/main_loop.rs Outdated Show resolved Hide resolved
current_eth_valset_nonce = get_valset_nonce(gravity_address, *MINER_ADDRESS, &web30)
.await
.expect("Failed to get current eth valset");
tokio::time::sleep(std::time::Duration::from_secs(4)).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, what I meant was rather that we can include use std::time::Duration and then when we call sleep, just refer to it like tokio::time::sleep(Duration::from_secs(...)), since there is no ambiguity to require fully qualifying it inline.

orchestrator/test_runner/src/happy_path.rs Outdated Show resolved Hide resolved
orchestrator/test_runner/src/happy_path_v2.rs Outdated Show resolved Hide resolved
orchestrator/test_runner/src/transaction_stress_test.rs Outdated Show resolved Hide resolved
@hannydevelop hannydevelop marked this pull request as draft November 9, 2021 11:04
@hannydevelop hannydevelop marked this pull request as ready for review November 10, 2021 10:10
@hannydevelop
Copy link
Contributor Author

oops, last commit message is wrong: fixed conflicting files, ran cargo fmt main again

@zmanian zmanian merged commit b705d74 into main Nov 16, 2021
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.

Orchestrator Async runtime improvements
4 participants