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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Assignment of availability-chunk indices to validators #47
Assignment of availability-chunk indices to validators #47
Changes from 3 commits
e00b475
52903f3
a3ccf05
a196ddc
c57a8b6
b3a4868
28511f6
303368b
2395237
90b0a50
4ae7529
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Thinking more about my argument to use
session_index
here, I think it's a very niche edge-case that's not worth worrying about. An alternative to (session_index
,block_number
) would be block hash which would also sidestep that issue.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.
Sounds fair. Another issue I just thought of with using block number is that for disputes hapenning on unimported forks, the ChainAPI call for getting the block number would also fail.
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 didn't get this argument. Are you talking about supply chain attacks? How is it specific to this RFC?
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.
here's an example:
rand
crate (malicious or not) makes a perfectly valid change to the shuffling algorithm, that results in a different output forshuffle()
.IMO this has a high chance of happening in the future.
I view this mapping in a similar way to the per-session validator shuffle we do in the runtime to choose the validator active set.
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.
now, we could implement our own version of
shuffle
, which would mitigate this issue.The thing I'm concerned about is implementing our own version of
ChaCha8Rng
. How feasible is it to assume that it won't change in the future? CC: @burdgesThere 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's not a supply chain attack so much as rand having a history of churn & instability.
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.
on a second thought: is this even possible? If the validator is not aware of the fork, how can it call a runtime API on that fork?
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.
if the validator hasn't seen that fork, it's can't call into runtime API of that
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.
IMHO we shouldn't worry too much about CoreJam compatibility. IIUC there might be a drop-in replacement for
ParaId
(AuthId
), so we should first explore an avenue withParaId
perhaps.The (bigger) problem with
ParaId
is that it's claimable by users, so in the theory one could create a collision attack where multiple paras have the same systematic chunks. In practice, I believe such an attack would be high cost and low benefit. And, perhaps, it could be mitigated by using a cryptographic hash ofParaId
(orAuthId
orcore_index
in the future).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.
We should use the core index here probably, not the para id. I'd thought the core indexes could be spaced somewhat evenly, but with the actual sequence being random.
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 changed my view here above in #47 (comment)
It's probably more future proof if we do not require the core index here, so the map uses num_cores, num_validators, relay parent, and paraid. We've other options but this looks pretty future proof. CoreJam cannot really work without something like paraid.
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.
Using paraid was my first attempt, but it complicates things quite a bit.
As Andronik notes, it's claimable by a user. I don't think it's the biggest problem though, they can't get a different ParaId every day (can they?).
paraid
's biggest problem is that it doesn't form a strictly monontonically increasing sequence, so we'd have to do something else with it. My initial attempt was to seed a ChaCha8 RNG with it and generate one random number. Then this number was the offset into the chunk index vector.But then if we complicate it so much, we may want to embed it into the runtime, so that future updates to the
rand
crate don't break us (which creates all sorts of problems that could be avoided).We've had this paraid -> core_index back and forth for a while now, it'd be great if we could all (the ones interested in this RFC) hop on a call or somehow reach a conclusion. There are pros and cons to both, AFAICT.
I think this remains the only bit that prevents this RFC from moving on (anybody correct me if I'm wrong)