light-client: integration test #431
Conversation
|
|
||
| let mut peer_map = HashMap::new(); | ||
| peer_map.insert(primary, node_address.clone()); | ||
| peer_map.insert(witness, node_address); |
There was a problem hiding this comment.
Here the same RPC end-point is used twice. Do you think it makes sense add a comment that in a realistic scenario these would be different and that you'd check if the peerId matches?
light-client/src/contracts.rs
Outdated
| @@ -12,6 +12,7 @@ pub fn trusted_store_contains_block_at_target_height( | |||
| target_height: Height, | |||
| ) -> bool { | |||
| light_store.get(target_height, Status::Verified).is_some() | |||
| || light_store.get(target_height, Status::Trusted).is_some() | |||
There was a problem hiding this comment.
Dope! 👍
Unfortunately, the test fails:
running 1 test
test sync ... FAILED
failures:
---- sync stdout ----
[info ] - iteration 1/20
[info ] synced to block 351
[info ] - iteration 2/20
[error] sync failed: no witness left
thread 'sync' panicked at 'failed to sync to highest: no witness left', light-client/tests/integration.rs:115:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
|
@liamsi Looking into it! |
|
The failure came from a ~2min clock drift between the Docker container and the GitHub CI worker. Increasing the clock drift parameter to 5min (to be safe) has fixed it. @liamsi Does that seem like an acceptable fix to you? |
| /// Correction parameter dealing with only approximately synchronized clocks. | ||
| /// The local clock should always be ahead of timestamps from the blockchain; this | ||
| /// is the maximum amount that the local clock may drift behind a timestamp from the | ||
| /// blockchain. | ||
| pub clock_drift: Duration, | ||
| /// The current time | ||
| pub now: Time, |
There was a problem hiding this comment.
I'm actually using these "Options" in #430 too. I was wondering how now made sense as an option. Much clearer when it isn't 👍
Does sound reasonable. Do you understand why the clock drift is so large? |
I unfortunately don't, would require more investigation. Perhaps someone else knows better? @greg-szabo? |
|
BTW, I also get a lot of |
xla
changed the title
@liamsi Hard to say like that. Might need to add some logging statements to figure out what's the underlying error. Perhaps we could/should even track those somehow. |
cf. #373
Adds an integration test for the light client.
The test spins up a light client against a single Tendermint node, where both the primary and a single witness point to that node.
It tries to verify to the highest block 20 times, at an interval of 800ms to give the chain time to advance.
The test uses a custom evidence reporter which panics if the supervisor tries to report a fork, as no fork should happen with this setup.
This PR also fixes a bug where the postcondition of
LightClient::verify_to_targetwas only looking for verified but not trusted blocks in the light store. This bug occurs when the highest block is still the initial trusted block, which happens in this case because we start syncing to the latest height right after fetching and initializing the trusted block.