Cjl/litt integration#3435
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3435 +/- ##
==========================================
- Coverage 59.30% 58.94% -0.36%
==========================================
Files 2118 2176 +58
Lines 175556 181527 +5971
==========================================
+ Hits 104108 107006 +2898
- Misses 62383 64938 +2555
- Partials 9065 9583 +518
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| var err error | ||
|
|
||
| hostKeyCallback := ssh.InsecureIgnoreHostKey() | ||
| hostKeyCallback := ssh.InsecureIgnoreHostKey() //nolint:gosec // overridden when knownHosts provided |
There was a problem hiding this comment.
[SUGGESTION] nolint:gosec for InsecureIgnoreHostKey() understates the risk
The comment says "overridden when knownHosts provided", but if a caller passes knownHosts == "" the insecure callback is what's actually used. This is pre-existing behavior, but if you're going to suppress the lint here, the nolint comment should reflect that fact (e.g. "intentional fallback; callers must supply knownHosts in production"), or — better — refuse to construct the session without knownHosts. Not introduced by this PR, but it's now being silenced rather than fixed.
There was a problem hiding this comment.
Created a follow up task for this. The ssh feature is not something we will be using. It was useful at Eigen since people wanted to use SSH to perform periodic DB backups, but we don't have that need here. If not actively disabled, this code won't even execute, so there is no risk in the short term.
https://linear.app/seilabs/issue/STO-545/safer-ssh-configuration
| TargetSegmentFileSize: math.MaxUint32, | ||
| MaxSegmentKeyCount: 50_000, | ||
| TargetSegmentKeyFileSize: 2 * units.MiB, | ||
| TargetSegmentKeyFileSize: 2 * unit.MB, |
There was a problem hiding this comment.
unit.MB is defined as 1024 * 1024 in sei-db/common/unit/data_units.go, which matches docker/go-units.MiB, so all sizes/throughputs are preserved. However, naming a 2^20 constant MB is the opposite of the SI convention and will eventually trip someone up. Not blocking — just worth a doc comment on the constant if you can't rename.
There was a problem hiding this comment.
I'm willing to rename to unit.MiB. Ten second refactor in the IDE, but will yield hundreds of diffs scattered across the codebase. Would you be ok if this happens in a separate PR?
There was a problem hiding this comment.
| keyRequest := &types.ScopedKey{ | ||
| Key: data.Key, | ||
| Address: types.NewAddress(s.index, firstByteIndex, uint8(shard), uint32(len(data.Value))), | ||
| Address: types.NewAddress(s.index, firstByteIndex, uint8(shard), uint32(len(data.Value))), //nolint:gosec // shard <= 255, value len fits uint32 |
There was a problem hiding this comment.
uint8(shard) is justified only because shard < ShardingFactor ≤ MaxShardingFactor = 256. The comment says "shard <= 255" which is right, but if a corrupted metadata file ever deserializes a shardingFactor > 256, this will silently truncate rather than fail loudly. Consider asserting shardingFactor <= MaxShardingFactor in loadMetadataFile/deserialize.
There was a problem hiding this comment.
The shard for each key is serialized/deserialized to/from a single byte, so it's impossible for data corruption to cause the value to exceed the max.
shardID: bytes[8], // shardID is type uint8
You are correct that the metadata file could be corrupted in a way that leads to this violation getting broken. In order to avoid that, I've modified serialization/deserialization for the metadata to also store data in a single byte, making this sort of bounds overflow impossible.
| // Write the TTL | ||
| ttlNanoseconds := t.GetTTL().Nanoseconds() | ||
| binary.BigEndian.PutUint64(data[4:12], uint64(ttlNanoseconds)) | ||
| binary.BigEndian.PutUint64(data[4:12], uint64(ttlNanoseconds)) //nolint:gosec // serialized as time.Duration |
There was a problem hiding this comment.
uint64(ttlNanoseconds) and time.Duration(uint64) round-trip is fine, but TTL is signed; a negative TTL would deserialize to a huge positive duration. Probably impossible in practice, but worth validating TTL > 0 on load
There was a problem hiding this comment.
Added a check for negative values:
intTTL := int64(binary.BigEndian.Uint64(data[4:12])) //nolint:gosec // serialized as time.Duration
if intTTL < 0 {
return nil, fmt.Errorf("TTL is negative: %d", intTTL)
}
ttl := time.Duration(intTTL)
PR SummaryMedium Risk Overview Sharding factor is standardized on Dependency/lint hardening and test adjustments. Replaces Reviewed by Cursor Bugbot for commit ac9765b. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 476462e. Configure here.
| // - 4 bytes for keyCount | ||
| // - 1 byte for sealed | ||
| V3MetadataSize = 21 | ||
| V3MetadataSize = 18 |
There was a problem hiding this comment.
Segment metadata format changed without version bump
Low Severity
The segment metadata binary layout changed (shardingFactor shrunk from 4 bytes to 1 byte, shifting all subsequent fields, total size from 21 to 18) while LatestSegmentVersion remains at ShardedAddressSegmentVersion = 3. Any existing V3 metadata files from development will fail with a confusing size-mismatch error instead of a clear version error. The version constant in segment_version.go was not incremented to reflect this incompatible on-disk format change.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 476462e. Configure here.
There was a problem hiding this comment.
Not a problem. The current version 3 is already a bump compared the prior deployed version (i.e. in the EigenDA repo). Since this isn't deployed to production yet, it's safe to make as many serialization breaks as we need without a version bump. That will change after we have our first production deployment, though.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
3 participants
Describe your changes and provide context
Testing performed to validate your change
unit test coverage, not yet used in prod