Move timestamp verification into validity window package #1895
Conversation
| ), | ||
| wantErr: ErrInvalidChunk, | ||
| producerNode: nodeID, | ||
| nodeLastAcceptedTimestamp: 1, | ||
| chunkExpiry: 1 + int64(testingDefaultValidityWindowDuration), | ||
| chunkExpiry: 2 + int64(testingDefaultValidityWindowDuration), |
There was a problem hiding this comment.
not opposing to, but - was this change intentional ?
There was a problem hiding this comment.
Yes, this is needed to get the boundary test case passing and align with the behavior of the time validity window. These two were slightly different in how they enforced it.
Previous boundary verification: https://github.com/ava-labs/hypersdk/blob/main/x/dsmr/storage.go#L80
Tx boundary verification: https://github.com/ava-labs/hypersdk/blob/main/chain/base.go#L39
When we call SetMin on the validity window for timestamp t, we will clear out all containers that are strictly less than timestamp t: https://github.com/ava-labs/hypersdk/blob/main/internal/emap/emap.go#L101. This means that allowing a transaction in at the forward boundary ie. blkTimestamp + validityWindow at blkTimestamp, the transaction will be screened out and marked as invalid in any future block with a timestamp up to and including blkTimestamp + validityWindow. After that, it will be marked as invalid because it has expired.
I'll write up a comment and add a test case inside the validitywindow package.
There was a problem hiding this comment.
Added a test here: 3278211
The previous code in chunk verification was overly defensive - it would mark a chunk as invalid if it was at the validity window duration in the future. This would be required if the emap removed any containers <= the current minimum timestamp.
| @@ -184,11 +183,10 @@ func (v *TimeValidityWindow[T]) calculateOldestAllowed(timestamp int64) int64 { | |||
| return max(0, timestamp-v.getTimeValidityWindow(timestamp)) | |||
| } | |||
|
|
|||
| func VerifyTimestamp(containerTimestamp int64, executionTimestamp int64, validityWindow int64) error { | |||
| func VerifyTimestamp(containerTimestamp int64, executionTimestamp int64, divisor int64, validityWindow int64) error { | |||
There was a problem hiding this comment.
the term divisor is mathematically correct, but a bit misleading, imho. I'd opt to use the term gradient or frame here.
There was a problem hiding this comment.
gradient doesn't make sense to me IMO... I also don't like the divisor name but I don't have a good concrete suggestion... I would probably call this something like boundary but I also feel like this name is not amazing
There was a problem hiding this comment.
divisor is the right term, old code had referred to this as modulus which I don't think was right.
Happy to change if there's a better term, otherwise will leave as is since it seems the consensus here is that we don't love the term, but it is the most correct, and we don't have a better one. I do agree with both of you though haha.
There was a problem hiding this comment.
Looks like we're now referring this as boundary anyways in this test, should we rename? 3278211
There was a problem hiding this comment.
I don't see any place in the added test where we are referring to the divisor as boundary. In that test, we are passing in 1 (ie. there's no such thing as a misaligned timestamp) in each instance.
| case containerTimestamp < executionTimestamp: // expiry: 100 block: 110 | ||
| return fmt.Errorf("%w: timestamp (%d) < block timestamp (%d)", ErrTimestampExpired, containerTimestamp, executionTimestamp) | ||
| case containerTimestamp > executionTimestamp+validityWindow: // expiry: 100 block 10 |
There was a problem hiding this comment.
These were in the old code but I feel like we should delete these comments as part of this diff anyways
There was a problem hiding this comment.
I find these helpful, would prefer not to delete. Would you prefer they be moved in some way?
There was a problem hiding this comment.
IMO it's unnecessary to comment to explain how < and > work, but if you find it helpful feel free free to leave it in. IMO these lines are self-documenting
| chunkExpiry: 1 + int64(testingDefaultValidityWindowDuration), | ||
| chunkExpiry: 2 + int64(testingDefaultValidityWindowDuration), |
There was a problem hiding this comment.
Why is this diff needed?
There was a problem hiding this comment.
| // Verify a timestamp at the validity window boundary fails for both a processing | ||
| // and accepted parent. | ||
| // Processing: | ||
| r.ErrorIs(validityWindow.VerifyExpiryReplayProtection(context.Background(), blk2), ErrDuplicateContainer) | ||
|
|
||
| // Accepted: | ||
| validityWindow.Accept(blk1) | ||
| r.ErrorIs(validityWindow.VerifyExpiryReplayProtection(context.Background(), blk2), ErrDuplicateContainer) | ||
|
|
||
| // Verify that after passing the validity window, the timestamp is no longer valid | ||
| r.ErrorIs(VerifyTimestamp(validityWindowDuration, validityWindowDuration+1, 1, validityWindowDuration), ErrTimestampExpired) |
There was a problem hiding this comment.
All of these look like separate test cases to me, can we make them separate tests? I don't think this is going to work well in a table-test which is why I suspect this test was written this way, but the cost of copy-and-paste is not that bad and I would prefer we did that over having a single monolithic test that tests multiple cases.
There was a problem hiding this comment.
I thought of this as the lifespan of a single tx being invalidated, but can break it up / see about making it a table test
There was a problem hiding this comment.
Added a separate table test for VerifyTimestamp and kept this test showing the lifespan of the tx (with an added comment).
The combination of the newly added test and the existing VerifyExpiryReplayProtection tests provides the same coverage (already covers processing/accepted at the boundary). Happy to remove this test if you prefer.
This PR moves timestamp verification checks into the time validity window package. Same code will be shared across tx and chunks.