This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Scheduler Module #1162
Scheduler Module #1162
Changes from 30 commits
af77a9a
d2eb247
631a6ba
cdf114b
b2a8c42
d6d751d
ba8dc9f
be800dd
cad53e6
eb64044
8b4cfd4
ff2e4d7
35a3692
adbaa1d
e5c1651
6a11337
c63a94b
1c6d4ab
83ba678
d9df378
3e7ac7b
1bee255
9096c14
242b129
94cb169
55ad79d
2703687
f707371
40d39eb
19f6d53
f5ef2fa
2d52e05
56e8669
a909ad8
89ad0f8
f85ff4e
50965e8
3918713
3392398
63dfb9c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use doc comments
///
instead of//
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In several places in this document, you remove a
CoreIndex
and insert something named*_offset: u32
. However, down inscheduler.rs
, you define aCoreIndex
type anyway. This leaves me confused: is the type still necessary, just not in these places, or have the documentation and implementation gotten out of sync?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are offsets, not core indices, so it would break an invariant that
CoreIndex
always refers to an actual core index, if that makes sense. I believe the implementation and guide are coherent on this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that we may not wish to pay for runtime checks of these properties, but it feels like a warning flag when we introduce the possibility of UB; those tend to be the difficult bugs to discover, when they occur. Maybe we could introduce a compilation feature like
ub-checks
which does do the appropriate runtime checks?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends what the runtime checks are.
When within the runtime, panicking is pretty much the worst thing you can do. Contrary to typical software, you don't want to fail fast and fail hard - you want to limp along in broken state and get fixed by governance.
Checks that print to console and return as no-op would be welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we'd ever run with
ub-checks
enabled on a "real" node. Instead, they'd be a debugging tool, maybe in CI, just to ensure that the properties we expect haven't been violated.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, although I don't perceive this as a blocker for the PR, better as a follow-on that would make a good first issue for someone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we care that
buf
always has32-len
trailing0
s?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope! In practice hash is always 32 bytes, and 0-padding it is pretty much the best option if the hash is smaller in some case. It doesn't lose us any security in any case.
In the case that the hash type is larger (again, unlikely), 32 random bytes are more than enough for our purposes.