Hi Nick. Shared randomness just made it into the consensus and it's making Stem's validator squawk. Trouble is that it's in the wrong position in the header. According to the spec...
The preamble contains the following items. They MUST occur in theorder given here:
However, shared-rand-current-value appears last...
Think I'll mark this as 'high' since it's a new regression in the live consensuses. Gonna hazard to guess we'll need to update the spec rather than tor since this has already made it into a release?
Kinda unfortunate since it means it won't live with the other shared randomness attributes...
To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information
Child items
...
Show closed items
Linked items
0
Link issues together to show that they're related.
Learn more.
Yeah, I think we loosen the spec to let them appear in whatever order, and then we put them in the order we want (I guess in a future consensus method if it's different from what we do now).
Well, the spec sort of has that looseness, I think. Clients are required to accept the header and footer lines in whatever order they occur, and that's fine. The issue is that authorities must generate them in the same order, or else they won't reach the same consensus document.
BTW, what's the right way to fix the spec here? Should we mention in the spec that preamble items can appear in whatever order, but all authorities need to generate them in a consistent way?
We should say that anybody parsing a consensus or a vote MUST accept them in any order, but that authorities building a consensus MUST produce them in a specific order, and then we specify that specific order.
We should say that anybody parsing a consensus or a vote MUST accept them in any order, but that authorities building a consensus MUST produce them in a specific order, and then we specify that specific order.
For what it's worth this doesn't change things for Stem. Without validation Stem is quite happy with the consensus - it's just validation squawking because... well, it's wrong.
Without these validation checks we wouldn't know about this issue at all, so I'm inclined to day 'everything working as intended on the stem and spec front'. We can fix this with a tor change that moves the property to the right spot, and await that release.
Concerning DocTor, I can make a temporary tweak so it ignores this particular issue until the fix is live.
Wait, why don't we simply move the shared-rand-current-value value in the spec to the right place where the tor code actually puts it? That way we have both code and spec aligned.
That would certainly be the quickest resolution (and what I originally thought we were gonna go with). Little nicer if the field moved though. There's a bunch of shared-random fields and this should live with them.
Clients besides Stem's validator don't care about this so there's no big rush. We can await a tor fix.
Wait, why don't we simply move the shared-rand-current-value value in the spec to the right place where the tor code actually puts it? That way we have both code and spec aligned.
If tor indeed places all four fields after params then I'd be happy with this. Might be wise to check a vote to ensure it's the case for shared-rand-participate and shared-rand-commit.
Nope, the proposed spec patch evidently has the wrong position for the shared randomness parameters that appear in votes. We also misorderd the new 'recommended-*-protocols' parameters.
Current vote I'm seeing doesn't have any legacy-dir-key lines so I'm unsure where shared randomness is in reference to those. Can someone confirm shared-random comes after legacy-dir-key? Can we finally drop legacy-dir-key from the spec?
For what it's worth I'll be cutting a stem hotfix release for this so it doesn't have a sad validator. But first I'm waiting for this spec change to be reviewed. Clearly it's error prone enough that it needs a second pair of eyes or two. ;)
Oh for the love of... flag-thresholds is also in the wrong spot. That one was added way back in 2013 but in Stem I coded against tor's actual behavior rather than the spec causing it to go under the radar. Fixed.
Spec ordering with regard to this is clearly error prone enough that I'm gonna change this from a MUST clause to SHOULD. Changes pushed...
As such Stem will no longer check the ordering of these fields. Tor will drift further from the spec here once we drop these checks but I don't think this is a detail any of us are gonna lose sleep over. :)
Feel free to reopen if you'd care to go a different course with this.
Trac: Resolution: N/Ato fixed Status: merge_ready to closed
Hi teor, are you sure you want to go this route? The problem in this ticket isn't that clients are unable to accept misordered fields. Stem can still read the consensus just fine. Rather, the trouble is that its descriptor validator was telling us that tor's actual behavior doesn't match the spec.
I see two options...
Fix the ordering in the spec, and be careful in the future that we don't keep making this mistake.
Loosen the requirement from MUST to SHOULD so it's no big whoop if tor's order matches the spec or not.
Considering that I found three separate instances of these fields being misordered I don't think this is a detail we want to keep contending with.
Hi teor, are you sure you want to go this route? The problem in this ticket isn't that clients are unable to accept misordered fields. Stem can still read the consensus just fine. Rather, the trouble is that its descriptor validator was telling us that tor's actual behavior doesn't match the spec.
I see two options...
Fix the ordering in the spec, and be careful in the future that we don't keep making this mistake.
Loosen the requirement from MUST to SHOULD so it's no big whoop if tor's order matches the spec or not.
Considering that I found three separate instances of these fields being misordered I don't think this is a detail we want to keep contending with.
I think we agree here.
I also think my patch does what you want, with the extra constraint that the arbitrary order of consensus lines may vary between consensus methods, but must be consistent for a particular consensus method:
- The preamble contains the following items. They SHOULD occur in the- order given here:+ The preamble contains the following items.++ Each consensus method specifies an order in which these items MUST occur.+ (A change in order requires a new consensus method.) In a vote, these+ items SHOULD occur in the same order.++ When parsing the consensus and votes, these items MUST be accepted in any+ order.
"Each consensus method specifies an order in which these items MUST occur"
... strikes me as the same as what we previously had...
"The preamble contains the following items. They MUST occur in the order given here:"
When I read 'each consensus method specifies an order' I think 'ok, so this is the order'. My change (simply changing MUST to SHOULD) drops any strong assurance that what follows is the actual ordering.
That said, I don't care too strongly about this. This patch feels like a lateral move to me. If Nick prefers it I don't have a strong objection to merging it.
Hi teor, are you sure you want to go this route? The problem in this ticket isn't that clients are unable to accept misordered fields. Stem can still read the consensus just fine. Rather, the trouble is that its descriptor validator was telling us that tor's actual behavior doesn't match the spec.
I see two options...
Fix the ordering in the spec, and be careful in the future that we don't keep making this mistake.
Loosen the requirement from MUST to SHOULD so it's no big whoop if tor's order matches the spec or not.
Considering that I found three separate instances of these fields being misordered I don't think this is a detail we want to keep contending with.
I think we agree here.
I also think my patch does what you want, with the extra constraint that the arbitrary order of consensus lines may vary between consensus methods, but must be consistent for a particular consensus method:
Hey, why do you think this consensus method consistency rule is important? I feel it makes the text confusing.
Personally, given the inconsistencies that atagar noted, I think the patches in comment:6 and comment:20 look good to me.
Hello, to finally resolve this ticket I merged the branch from comment:6 which specifies that the preamble MUST be accepted in whatever order by clients, but authorities MUST be consistent about it (so that consensus can be reached).
I will close this ticket. Feel free to reopen if you think something is wrong.
Thanks :)
Trac: Resolution: N/Ato fixed Status: needs_review to closed
Hi George, gonna reopen. I argued against your patch a while ago and I still disagree with it. If you have the spec say MUST then this order is something we should validate. See my earlier comments regarding why you probably don't want this headache.
Mind reverting your patch?
Trac: Status: closed to reopened Resolution: fixed toN/A
Hi George, gonna reopen. I argued against your patch a while ago and I still disagree with it. If you have the spec say MUST then this order is something we should validate. See my earlier comments regarding why you probably don't want this headache.
Mind reverting your patch?
Hey atagar, sorry about that. I think I had not understood your concerns.
FWIW I'm also fine with your patch in comment:20. How would you feel if I rebased your comment:20 patch to current torspec master, and pushed that instead?
Hi George. Actually, my change from comment:20 was already pushed (though by changing it back to MUST this reverted it). How in particular are you inclined toward combining these changes?
comment:24 has the reason I'm leaning toward making this SHOULD. However, if you'd care to do an audit that the spec's order is now accurate and ensure we're really careful not to keep making mistakes on this we can go ahead and make it a MUST clause.
Hi George. Actually, my change from comment:20 was already pushed (though by changing it back to MUST this reverted it). How in particular are you inclined toward combining these changes?
comment:24 has the reason I'm leaning toward making this SHOULD. However, if you'd care to do an audit that the spec's order is now accurate and ensure we're really careful not to keep making mistakes on this we can go ahead and make it a MUST clause.
Hey Damian,
sorry about that. I reverted my commit, and now strict ordering is SHOULD.
I'm closing this ticket again, given that we didn't hear from teor. I'm personally OK with the current state of the spec.
Trac: Resolution: N/Ato fixed Status: reopened to closed