Problem: bridge panics if there's an error retrieving gas price #93
Conversation
Solution: instead, log an error and use previous price
ghost
assigned 
There was a problem hiding this comment.
@yrashk and @DrPeterVanNostrand, what sort of tests did you perform to check these changes? How did you simulate incorrect oracle response to cover all possible error cases? Thanks
bridge/src/bridge/gas_price.rs
Outdated
| @@ -44,6 +45,8 @@ impl GasPriceStream { | |||
| speed: node.gas_price_speed, | |||
| request_timer: timer.clone(), | |||
| interval: timer.interval_at(Instant::now(), CACHE_TIMEOUT_DURATION), | |||
| last_price: node.default_gas_price, | |||
| request_timeout: node.request_timeout, | |||
There was a problem hiding this comment.
@yrashk my expectation is that gas_price_timeout should be used. Since it was introduced for this. I can read the following in README.md:
home/foreign.gas_price_timeout - the number of seconds to wait for an HTTP response from the gas price oracle before using the default gas price. Defaults to 10 seconds.
There was a problem hiding this comment.
Yup, you are correct. That config option was added for this reason. I'm not sure how necessary having this config option it is though. Its plausible that we use a single timeout duration for all http requests (web3 and gas-price).
There was a problem hiding this comment.
It is different systems and logically they could have different requirements to availability. For example, the timeout to handle response from parity node (it is my understanding from @yrashk explanation how it works) could be larger than the timeout to just getting response from gas price oracle. That's why I think they needs to be differentiated.
| @@ -12,7 +12,6 @@ use config::{GasPriceSpeed, Node}; | |||
| use error::Error; | |||
|
|
|||
| const CACHE_TIMEOUT_DURATION: Duration = Duration::from_secs(5 * 60); | |||
There was a problem hiding this comment.
Does it make sense to have CACHE_TIMEOUT_DURATION changeable through the configuration file?
There was a problem hiding this comment.
It would be easy enough to add it to the config file, the only problem that I could see is that if a user accidentally set their cache-duration too low, they could possibly DOS the gas price oracle.
The duration of the cache should reflect Ethereum's gas-price volatility. For example, if the gas-price was rapidly changing, you would want a shorter cache duration, however, looking at the current sate of the Ethereum network, the average gas-price across all confirmed transactions stays within the [5, 25] GWEI range, and has been that way for quite some time. To me, this indicates that cache duration should be a constant.
What are your thoughts?
There was a problem hiding this comment.
thanks for good reasoning! I agree that cache-duration should not be too low. And that's why I do not see any reason it was chosen to be 5 minutes. Could it be greater: 15 minutes for example? Since we could have different view based on different analysis I suggest to leave the hardcoded value at this moment and open a new issue to make this parameter configurable in the next updates. Any objections?
|
@akolotov To test, I just ran When testing, I logged out the retrieved gas prices and verified them manually, by going to the Oracle url in my browser. This is definitely not the ideal way to test, but I found that the test suite was pretty confusing (in code layout, what the tests were printing out, sometimes a parity node would be left running in the background after a test failed). With the changes in this PR, the two error cases for the dynamic gas-price (http request error, unrecognized JSON schema) will just default to the last retrieved price. So i'm not sure if adding any new tests would be beneficial. Do you guys @akolotov @yrashk have any suggestions for better ways to do testing around something like this? |
|
@DrPeterVanNostrand is it enough to run just integration tests to cover all possible error cases addressed in the changes? Just would like to get sure that we confirm that changes are safe before running them on |
|
@akolotov I ran a number of manual tests on getting wrong responses from an oracle. However, I'm working on decomposing the gas price retriever to be able to simulate these scenarios in automated tests. |
Solution: extract price retrieving from GasPriceStream and test it.
However, this is different infrastructure and might have different requirements. Solution: use gas_price_timeout
|
I've added automatic tests and switched to gas price specific timeout |
There was a problem hiding this comment.
@yrashk @DrPeterVanNostrand if you have no more obvious things to add, let's merge the changes and proceed with testing.
|
@akolotov I don't have anything else to add |
Problem: bridge panics if there's an error retrieving gas price
Solution: instead, log an error and use previous price