[Access] Get protocol snapshot by block id and block height #4957
Conversation
…pshot-from-any-block-endpoint
…o AndriiDiachuk/4869-get-protocol-snapshot-from-any-block-endpoint
…-endpoint' of https://github.com/AndriiDiachuk/flow-go into AndriiDiachuk/4869-get-protocol-snapshot-from-any-block-endpoint
AndriiDiachuk
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4957 +/- ##
==========================================
- Coverage 56.31% 54.20% -2.12%
==========================================
Files 976 786 -190
Lines 91752 73885 -17867
==========================================
- Hits 51673 40047 -11626
+ Misses 36253 31035 -5218
+ Partials 3826 2803 -1023
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
AndriiDiachuk
requested review from
gomisha,
thep2p and
peterargue
as code owners
There was a problem hiding this comment.
thanks @AndriiDiachuk! great work so far! Added a few comments.
@jordanschalm it looks like the original version GetLatestProtocolStateSnapshot uses a method that may return a snapshot from an earlier block. This would be confusing for these new methods. Is that required, or only needed for dynamic bootstrapping?
flow-go/engine/access/rpc/backend/backend_network.go
Lines 135 to 141 in 2a5d0fd
…nted in backend_network.go
The snapshot model doesn't have a way to encode having one part of the sealing segment being in a different epoch or phase than another. So if we return a snapshot that spans two epochs or phases, the client would not be able to correctly interpret the epoch/phase of all the blocks it contains. For now, in this case where a specific height is requested, I suggest we return a sentinel error, rather than walking backward to find a valid snapshot. Blocks with snapshots that span one of these state transitions are very rare (<0.001% on Mainnet). It should be easier to modify the snapshot to accommodate these cases once Dynamic Protocol State is in place. Each block contains a commitment to the corresponding protocol state, including epoch info. Then we can include a list of |
@jordanschalm Thanks for your suggestions. Pushed code with changes. Waiting for a new round of review. |
…o AndriiDiachuk/4869-get-protocol-snapshot-from-any-block-endpoint
…-endpoint' of https://github.com/AndriiDiachuk/flow-go into AndriiDiachuk/4869-get-protocol-snapshot-from-any-block-endpoint
| suite.Require().Equal(status.Errorf(codes.InvalidArgument, "failed to retrieve snapshot for block by height %v", | ||
| fmt.Errorf("%d: %v", newBlock.Header.Height, "block not finalized")).Error(), | ||
| err.Error()) | ||
| suite.T().Run("failed to get a valid snapshot", func(t *testing.T) { |
There was a problem hiding this comment.
What specific failure case are we testing here?
The test code implies that the failure is arising from the mocked error in AtHeight on line 473, however AtHeight function is not invoked in this code path. When I inspect the error, it is actually coming from the fact that we are failing to produce a sealing segment for the queried snapshot, because newBlock has not been finalized as far as the local state variable is concerned. This is inconsistent with the mocked BlockIDByHeight on line 476, which implies that newBlock is finalized.
Please clearly state which failure case is being tested and make sure the test implements that failure case consistently. There are many different cases we can and should test here, for example:
- (unknown query block)
ErrUnknownSnapshotReferenceerror fromsnapshot.AtBlockID-> NotFound code - (different block finalized at height)
BlockIDByHeightreturns different block ID -> InvalidArgument code - unexpected error from
snapshot.AtBlockID-> Internal code - unexpected error from
BlockIDByHeight-> Internal code
Co-authored-by: Jordan Schalm <[email protected]>
…o AndriiDiachuk/4869-get-protocol-snapshot-from-any-block-endpoint
| }) | ||
| } | ||
|
|
||
| // TestGetProtocolStateSnapshotByBlockID_NonQueryBlock tests our GetProtocolStateSnapshotByBlockID RPC endpoint |
There was a problem hiding this comment.
| // TestGetProtocolStateSnapshotByBlockID_NonQueryBlock tests our GetProtocolStateSnapshotByBlockID RPC endpoint | |
| // TestGetProtocolStateSnapshotByBlockID_UnknownQueryBlock tests our GetProtocolStateSnapshotByBlockID RPC endpoint |
| snapshotBytes, err := backend.GetProtocolStateSnapshotByBlockID(ctx, newBlock.ID()) | ||
| suite.Require().Nil(snapshotBytes) | ||
| suite.Require().Error(err) | ||
| suite.Require().ErrorAs(err, &expectedError) |
There was a problem hiding this comment.
expectedError has the interface type error, not a concrete type that implements error, so you could pass in any error-typed value as the first argument here, and the assertion would pass.
status.Errorf does not support unwrapping errors as far as I can tell, so we unfortunately cannot use the built-in error checking functions here. (As a side not, if we could, we would likely want errors.Is, not errors.As, as we want to compare value equality, not type equality.)
| suite.Require().ErrorAs(err, &expectedError) |
There was a problem hiding this comment.
Looks good to me aside from the last couple of comments. @peterargue I ended up reviewing the full PR, not just the finalization logic.
Thank you for the excellent test coverage of the new RPC methods.
There was a problem hiding this comment.
thanks @AndriiDiachuk! Excellent documentation.
Added a couple small comments, but looks good.
There was a problem hiding this comment.
thanks @AndriiDiachuk! Excellent documentation.
Added a couple small comments, but looks good.
…otocol-snapshot-from-any-block-endpoint [Access] Get protocol snapshot by block id and block height
…otocol-snapshot-from-any-block-endpoint [Access] Get protocol snapshot by block id and block height
#4869
In this pull request are Introduced new methods
GetProtocolStateSnapshotByBlockID,GetProtocolStateSnapshotByHeightthat return the snapshot from the specified block if that block is finalized.Related PR:
onflow/flow#1401
onflow/flow-emulator#501