Conversation
rphmeier
added
A0-please_review
roadmap/implementers-guide/src/node/approval/approval-voting.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>
|
We could fix the no-show handling in two different ways: Your new "Take one new non-empty tranche per no-show" vs my old one "Take one new tranche per no-show or the minimal number of tranches which cover the no-show, which ever is more." I think either provides similar security, but I guess your new text yields simpler code. :) |
roadmap/implementers-guide/src/node/approval/approval-voting.md
Outdated
Show resolved
Hide resolved
|
There is a time shifting effect that I implemented and discussed in #1930 that does not appear here. Arguably it's implicit but not really I think. |
|
@burdges wasn't the outcome of our calls that we didn't need to do the time-shifting thing? I'm not totally clear on how that detail works and what time-shifting we're meant to apply. Could you make a PR referring to how we're meant to do it in abstract English, and add that to |
|
We've count no shows from when we received the assignment, which avoids misscount other's late assignments as no show. We do however require some mechanism that prevents us sending announcements too early when a no show happens. If a no show takes 1.5 relay chain blocks, so 36 tranches, then we should avoid everyone from tranche 1-35 announcing when one person from tranche zero no shows. It's straightforward to set a stoping condition in the assignment counter loop to achieve this I think. |
Yeah, I'll add something like this today which should prevent everyone from broadcasting at once. |
| * Count no-shows in tranches `k..l` and for each of those, take another non-empty tranche for each no-show. Repeat so on until either | ||
| * We run out of tranches to take, having not received any assignments past a certain point. In this case we set `n_tranches` to a special value `RequiredTranches::Pending(last_taken_tranche + uncovered_no_shows)` which indicates that new assignments are needed. `uncovered_no_shows` is the number of no-shows we have not yet covered with `last_taken_tranche`. | ||
| * All no-shows are covered by at least one non-empty tranche. Set `n_tranches` to the number of tranches taken and return `RequiredTranches::Exact(n_tranches)`. | ||
| * The amount of assignments in non-empty & taken tranches plus the amount of needed extras equals or exceeds the total number of validators for the approval entry, which can be obtained by measuring the bitfield. In this case we return a special value `RequiredTranches::All` indicating that all validators have effectively been assigned to check. |
There was a problem hiding this comment.
Yes, I think using the bitfield is reasonable in the short term. We'll want something smaller eventually, but asked discussed previously exactly what can be discussed later. In the short term, All could trigger a dispute style 2/3 check, or it could simply trash the candidate and fork the chain, but if we become happy with some lower threshold then maybe trashing the candidate actually becomes more efficient. We'd slash eventually either way.
There was a problem hiding this comment.
I bring up the bitfield here just because it's the easiest way to get n_validators with the data we have available in this scope. A usize would be smaller, but since the ~1000 bits are already referenced by the ApprovalEntry, it's harmless to count the length here.
|
I prefer this version over my own suggestions in #1930 We've many tranche zero assignments so we'll have multipe tranche zero no-shows occasionally. We delay slightly for broadcasting the second obligatory tranche in my version, while we broadcast both tranches immediately in this version. It's also exploits this notion of non-empty tranche to simplify the counting logic |
|
bot merge |
|
Waiting for commit status. |
ghost
merged commit 3 participants
cc @burdges
Supersedes #1930
Change description: