light-client: treat `Trusted` status as a special case of `Verified` as per the spec #419

romac · 2020-07-08T10:23:32Z

Close #427

This PR also addresses a concern raised in #418. While has been brought to light that there is no actual case where a trusted block would be downgraded to verified, I believe it is a bit cleaner to assume it could happen and handle it properly.

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGES.md

Only looking for blocks with `Status::Verified` might miss a block that has previously gone through fork detection and marked as `Trusted`. We would then fetch it again from the peer, go through verification again, and then override its status to `Verified`, thus downgrading it from `Trusted` to `Verified`. This commit fixes this by asking the light store for blocks which are either trusted or verified.

light-client/src/light_client.rs

light-client/src/store.rs

light-client/src/types.rs

light-client/src/store.rs

liamsi

Overall approach makes sense to me. Maybe remove least_trusted?

Co-authored-by: Ismail Khoffi <[email protected]>

romac · 2020-07-09T11:09:38Z

@josef-widder This PR also addresses a concern raised in #418. While has been brought to light that there is no actual case where a trusted block would be downgraded to verified, I believe it is a bit cleaner to assume it could happen and handle it properly. We can revisit this when we add support for verifying blocks below the latest trusted state. What do you think?

liamsi

Although it seems that this code solve a non-existing issue, the changes make sense to me. Would like to hear other's thoughts too.

codecov-commenter · 2020-07-09T20:56:54Z

Codecov Report

Merging #419 into master will increase coverage by 0.0%.
The diff coverage is 46.3%.

@@          Coverage Diff           @@
##           master    #419   +/-   ##
======================================
  Coverage    30.7%   30.8%           
======================================
  Files         131     132    +1     
  Lines        5459    5516   +57     
  Branches     1697    1712   +15     
======================================
+ Hits         1678    1699   +21     
- Misses       2598    2619   +21     
- Partials     1183    1198   +15

Impacted Files	Coverage Δ
light-client/src/contracts.rs	`50.0% <0.0%> (-13.7%)`	⬇️
light-client/src/errors.rs	`2.1% <ø> (+<0.1%)`	⬆️
light-client/src/fork_detector.rs	`29.7% <0.0%> (ø)`
light-client/src/light_client.rs	`28.5% <35.7%> (-2.6%)`	⬇️
light-client/src/utils.rs	`12.9% <36.3%> (ø)`
light-client/src/types.rs	`51.1% <52.3%> (+5.3%)`	⬆️
light-client/src/store.rs	`60.0% <57.1%> (+18.3%)`	⬆️
light-client/src/macros.rs	`25.0% <0.0%> (+1.9%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 142796b...9631ead. Read the comment docs.

xla

Lots of good stuff in here, but there is also quite some surprises in the code. In general the changes don't really reflect the premise, as Trusted and Verified are still distinct states. More inline.

light-client/src/lib.rs

xla · 2020-07-10T08:00:03Z

light-client/src/light_client.rs

+            .fetch_light_block(self.peer, AtHeight::At(height))
+            .map_err(ErrorKind::Io)?;
+
+        state.light_store.insert(block.clone(), Status::Unverified);


It's strange to see mutations being done in a method that is called get_or_fetch_block. I know it has been here before but it indicates something being of about where we manage this concern. As now there is an implicit behavioural assumption that the fetched block will be in the light store going forward.

Agreed it's not ideal. Allhough we don't really rely on this assumption, it's more of a performance optimization (ie. caching of fetched blocks). Perhaps the method should be renamed, or even refactored into a helper which caches blocks in the store and the original get_or_fetch_block. This would make the light store mutation more explicit at call site. Will think about it.

Ideally behaviour like this (sounds like write-through caching) happens on a layer wrapping this component. Often referred to as decorators.This way each layer can be looked at in isolation and concerns are separated.

I can give examples and/or go into more detail if it is of interest.

Ideally behaviour like this (sounds like write-through caching) happens on a layer wrapping this component. Often referred to as decorators.This way each layer can be looked at in isolation and concerns are separated.

Agreed, that's actually what I had in mind when I wrote "refactored into a helper which caches blocks in the store" but the name decorator didn't spring to my mind at the time :)

Can we capture this somewhere so it doesn't get lost?

Captured in #434.

light-client/src/light_client.rs

light-client/src/utils.rs

light-client/src/types.rs

romac · 2020-07-10T09:53:15Z

Thanks @xla for the valuable comments!

In general the changes don't really reflect the premise, as Trusted and Verified are still distinct states. More inline.

Agreed, going further would require a better API for the Store, something I have somewhat expressed in #428.

xla

Besides one follow-up we should capture, all dope!

🐦 ↗️ 🉐 💟

romac added 2 commits July 7, 2020 15:09

Refactor LightStore::latest_trusted_or_verified

7f1f813

romac added the light-client Issues/features which involve the light client label Jul 8, 2020

romac requested review from brapse, xla and liamsi July 8, 2020 10:23

romac marked this pull request as draft July 8, 2020 10:25

romac added 2 commits July 8, 2020 12:28

Small readability improvement in get_or_fetch_block

5e5ee5e

Ensure blocks are not downgraded from trusted to verified

2c69947

OStevan reviewed Jul 8, 2020

View reviewed changes

light-client/src/light_client.rs Outdated Show resolved Hide resolved

Preserve Trusted status when updating a block that was just verified

cd729b9

romac changed the title ~~Look for trusted or verified blocks in bisection loop~~ Avoid downgrading blocks from trusted to verified during bisection Jul 8, 2020

liamsi reviewed Jul 8, 2020

View reviewed changes

light-client/src/store.rs Outdated Show resolved Hide resolved

liamsi reviewed Jul 8, 2020

View reviewed changes

light-client/src/types.rs Outdated Show resolved Hide resolved

liamsi reviewed Jul 8, 2020

View reviewed changes

light-client/src/store.rs Outdated Show resolved Hide resolved

liamsi reviewed Jul 8, 2020

View reviewed changes

romac mentioned this pull request Jul 9, 2020

Light client might downgrade already Trusted blocks to Verified #418

Closed

Fix typo in comment

9516493

Co-authored-by: Ismail Khoffi <[email protected]>

romac changed the title ~~Avoid downgrading blocks from trusted to verified during bisection~~ Treat Trusted status as a special case of Verified as per the spec Jul 9, 2020

romac added 2 commits July 9, 2020 13:11

Remove Status::least_verified method

87c7f2a

Update changelog

f6064a7

romac requested a review from liamsi July 9, 2020 11:12

Rename LightStore::get_any to get_non_failed

9974c04

romac marked this pull request as ready for review July 9, 2020 11:16

liamsi mentioned this pull request Jul 9, 2020

light-client: integration test #431

Merged

5 tasks

liamsi reviewed Jul 9, 2020

View reviewed changes

Fix contracts to take into account the Trusted status

9631ead

xla changed the title ~~Treat Trusted status as a special case of Verified as per the spec~~ light-client: treat Trusted status as a special case of Verified as per the spec Jul 10, 2020

xla suggested changes Jul 10, 2020

View reviewed changes

romac added 2 commits July 10, 2020 11:55

Rename utils module to std_ext with inner modules

95448d2

Improve doc of get_or_fetch_block

60d33e3

romac mentioned this pull request Jul 10, 2020

light-client: improve LightStore API #428

Open

xla approved these changes Jul 10, 2020

View reviewed changes

brapse merged commit 3531624 into master Jul 10, 2020

brapse deleted the romain/trusted-or-verified branch July 10, 2020 10:49

romac mentioned this pull request Jul 10, 2020

light-client: improve separation of concerns in get_or_fetch_block #434

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

light-client: treat `Trusted` status as a special case of `Verified` as per the spec #419

light-client: treat `Trusted` status as a special case of `Verified` as per the spec #419

romac commented Jul 8, 2020 •

edited

Loading

liamsi left a comment •

edited

Loading

romac commented Jul 9, 2020

liamsi left a comment

codecov-commenter commented Jul 9, 2020

xla left a comment

xla Jul 10, 2020

romac Jul 10, 2020

xla Jul 10, 2020

xla Jul 10, 2020

romac Jul 10, 2020

xla Jul 10, 2020

romac Jul 10, 2020

romac commented Jul 10, 2020

xla left a comment

light-client: treat Trusted status as a special case of Verified as per the spec #419

light-client: treat Trusted status as a special case of Verified as per the spec #419

Conversation

romac commented Jul 8, 2020 • edited Loading

liamsi left a comment • edited Loading

Choose a reason for hiding this comment

romac commented Jul 9, 2020

liamsi left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 9, 2020

Codecov Report

xla left a comment

Choose a reason for hiding this comment

xla Jul 10, 2020

Choose a reason for hiding this comment

romac Jul 10, 2020

Choose a reason for hiding this comment

xla Jul 10, 2020

Choose a reason for hiding this comment

xla Jul 10, 2020

Choose a reason for hiding this comment

romac Jul 10, 2020

Choose a reason for hiding this comment

xla Jul 10, 2020

Choose a reason for hiding this comment

romac Jul 10, 2020

Choose a reason for hiding this comment

romac commented Jul 10, 2020

xla left a comment

Choose a reason for hiding this comment

light-client: treat `Trusted` status as a special case of `Verified` as per the spec #419

light-client: treat `Trusted` status as a special case of `Verified` as per the spec #419

romac commented Jul 8, 2020 •

edited

Loading

liamsi left a comment •

edited

Loading