Track map revision inside mutation#1310
Conversation
This allows callsers to track the revision number of the returned data
Codecov Report
@@ Coverage Diff @@
## master #1310 +/- ##
==========================================
- Coverage 30.45% 30.43% -0.03%
==========================================
Files 45 45
Lines 3766 3769 +3
==========================================
Hits 1147 1147
- Misses 2443 2446 +3
Partials 176 176
Continue to review full report at Codecov.
|
mhutchinson
left a comment
mhutchinson
left a comment
There was a problem hiding this comment.
Is this API still open for breaking changes? Any code using this will be broken by the extra return values on the public methods.
|
|
||
| // BatchVerifiedGetUser returns verified leaf values by userID. | ||
| func (c *Client) BatchVerifiedGetUser(ctx context.Context, userIDs []string) (map[string]*pb.MapLeaf, error) { | ||
| func (c *Client) BatchVerifiedGetUser(ctx context.Context, userIDs []string) ( |
There was a problem hiding this comment.
The doc string should be updated to explain the relationship between the returned values.
It's not immediately clear to me what the contract of this method is. Is it supposed to return the latest value from the server, or is any "recent" (or even historical) revision okay? Passing in an explicit revision for the map would avoid this, but may go against some other design principles here?
There was a problem hiding this comment.
The API contract is indeed unclear and unverified.
I'll add a TODO to fix this since the change affects multiple files.
The intent is that this would fetch values against the latest map root, but as it is, the client does not enforce any constraint on the freshness of the MapRoot.
Your second pair of eyes on this is much appreciated!
core/mutator/entry/mutation.go
Outdated
| // SetPrevious sets the previous hash. | ||
| // If copyPrevious is true, AuthorizedKeys and Commitment are also copied. | ||
| func (m *Mutation) SetPrevious(oldValue []byte, copyPrevious bool) error { | ||
| func (m *Mutation) SetPrevious(rev uint64, oldValue []byte, copyPrevious bool) error { |
There was a problem hiding this comment.
The target of the revision here is potentially ambiguous. My context-free instinct would be that this revision should be set to the revision at which the value was set to oldValue. My understanding from the rest of the code in this CL is that rev is actually intended to be set to the current live/latest revision for the map. This could be a much higher revision than when the value was last modified.
On further reading of waitOnceForUserUpdate I think either revision would be fine. Leaving this comment as I think a little clarity on expectation here would be beneficial.
There was a problem hiding this comment.
The intent here is to give the client a clue about how long to wait before attempting to verify that a mutation was applied correctly. This behavior works best when rev is set to the revision at which oldValue was fetched since the client will wait until the server's reported current revision > rev.
* master: Move verifier to its own package (google#1312)
gdbelvin
changed the title
* master: Track map revision inside mutation (google#1310)
* master: (106 commits) Remove unused logVerifier (google#1324) Verify Revisions in StreamRevisions (google#1323) Pair verifier functions (google#1322) Split VerifyRevision into Verify{LogRoot,MapRevision (google#1318) Make Previous hash check optional (google#1307) Remove VerifySignedMapRoot from VerifierInterface (google#1320) Remove trailing whitespace (google#1321) Encapsulate Client Verifier State in test vectors (google#1316) Pass along err message (google#1314) Remove unnessesary func() (google#1319) New test vector transcript format (google#1315) Track map revision inside mutation (google#1310) Move verifier to its own package (google#1312) go generate ./... (google#1306) Fix proto copying in revisions and paginator tests. (google#1309) Fix proto copying in server_test. (google#1308) go mod tidy (google#1305) Use new TrillianMapWrite API (google#1304) Configurable maximum queue depth for metric reporting. (google#1303) Proposal to refine docker deployment (google#1302) ...
* master: (95 commits) Remove unused logVerifier (google#1324) Verify Revisions in StreamRevisions (google#1323) Pair verifier functions (google#1322) Split VerifyRevision into Verify{LogRoot,MapRevision (google#1318) Make Previous hash check optional (google#1307) Remove VerifySignedMapRoot from VerifierInterface (google#1320) Remove trailing whitespace (google#1321) Encapsulate Client Verifier State in test vectors (google#1316) Pass along err message (google#1314) Remove unnessesary func() (google#1319) New test vector transcript format (google#1315) Track map revision inside mutation (google#1310) Move verifier to its own package (google#1312) go generate ./... (google#1306) Fix proto copying in revisions and paginator tests. (google#1309) Fix proto copying in server_test. (google#1308) go mod tidy (google#1305) Use new TrillianMapWrite API (google#1304) Configurable maximum queue depth for metric reporting. (google#1303) Proposal to refine docker deployment (google#1302) ...
3 participants
After submitting a mutation, the client typically waits for a new
revision to appear before requesting for new data.
This PR fixes a logic bug that assumed that the log root == map revision.
Log roots and map revisions are separate concepts and should not be treated as such.
Breaking this spurious dependency also supports refactoring root tracking #1311