[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #16943 [Tor]: Implement prop250 (Random Number Generation During Tor Voting)
#16943: Implement prop250 (Random Number Generation During Tor Voting)
-------------------------+------------------------------------
Reporter: asn | Owner:
Type: enhancement | Status: needs_review
Priority: High | Milestone: Tor: 0.2.8.x-final
Component: Tor | Version:
Severity: Normal | Resolution:
Keywords: tor-hs | Actual Points:
Parent ID: #8244 | Points: large
Sponsor: SponsorR |
-------------------------+------------------------------------
Comment (by teor):
Replying to [comment:21 dgoulet]:
> After proposal review that happened few days ago on #tor-dev, we made
some adjustments to the specification and code.
>
> Please find the latest code here: `prop250_final_v3`
>
> Proposal 250 has been updated with the latest in torspec.git
Code review:
General comments:
Every other multi-word file name in src/or uses underscores.
If a function moves data from `(dtype* dst, const stype* src)`, can the
arguments be in that order, and can the src argument be const?
There are a lot of missed opportunities for const pointer arguments.
e282227ebc03ae71f1cd6490a2adf2058140c66e:
If you use `#ifdef WORDS_BIGENDIAN` (from orconfig.h), rather than a
runtime check against htonl, some compilers will produce better code.
72baec6805aaab8d62f2deed8fbc658c82a7d086:
shared-random-state.c:
If we use an asymmetric magic number, we can detect when the two halves
are swapped.
{{{
#define SR_DISK_STATE_MAGIC 42424242
}}}
A hard-coded `SHARED_RANDOM_N_ROUNDS` is going to make it really hard to
test hidden services quickly using chutney. (We'll always be testing them
using the default initial shared random value.) Can we make this
configurable in test networks?
{{{
#define SHARED_RANDOM_N_ROUNDS 12
}}}
get_start_time_of_current_round() doesn't take
TestingV3AuthVotingStartOffset into account. Why not use:
{{{
time_t next_start = voting_schedule.interval_starts;
time_t curr_start = dirvote_get_start_of_next_interval(next_start -
interval - 1,interval,
options->TestingV3AuthVotingStartOffset);
}}}
get_next_valid_after_time() duplicates code in
dirvote_recalculate_timing().
Why not use `voting_schedule.interval_starts`?
(You'd need to make sure dirvote_recalculate_timing() had been called
before either of these functions used `voting_schedule.interval_starts`.)
Nitpick: why 2? Can it be a named constant like `SHARED_RANDOM_N_PHASES`?
get_state_valid_until_time, get_sr_protocol_phase
{{{
int total_rounds = SHARED_RANDOM_N_ROUNDS * 2;
}}}
get_state_valid_until_time() also ignores TestingV3AuthVotingStartOffset.
I think this should fix it:
{{{
current_round = (beginning_of_current_round / voting_interval) %
total_rounds;
}}}
get_sr_protocol_phase() also ignores TestingV3AuthVotingStartOffset.
Perhaps passing around the time isn't the best way to do this? It seems
like if we can make this same mistake in multiple places, then we should
refactor the round number calculation so it's in a single function.
Does get_sr_protocol_phase() need to check it's getting an actual
valid_after time, rather than now?
Why do we have disk_state_validate() and disk_state_validate_cb()?
Should one call the other? Should they check _magic?
Should they check ValidAfter < ValidUntil?
disk_state_parse_commits() fails to `SMARTLIST_FOREACH(args, char *, cp,
tor_free(cp));` if commit is NULL.
disk_state_parse_sr_values() contains duplicate code in the if/else block.
The function comment should say it parses both SharedRandPreviousValue and
SharedRandCurrentValue.
I think disk_state_parse_sr_values() might be able to leak memory if
disk_state_parse_srv() fails. At the very least, it leaves an allocated
but zero pointer, which could confuse the rest of the code.
disk_state_parse_sr_values() can assert its arguments are not NULL.
disk_state_put_commit_line() can have commit as a const pointer, and in
the opposite order for consistency.
In disk_state_put_commit_line(), do we need to memwipe reveal_str?
Or are we about to make it public anyway?
disk_state_put_srv_line() can have srv as a const pointer, and in the
opposite order for consistency.
I think disk_state_reset() is the moral equivalent of disk_state_free()
followed by disk_state_new(). Why are they different functions? (Apart
from the memory thing.)
We don't seem to check for errors in disk_state_update(). I think this is
ok, but I'm not sure exactly why.
In disk_state_save_to_disk() and children (and other functions), should we
memwipe any temporary memory that contains the reveal value, or the
encoded reveal value? (before it is revealed)
We do this some places, but not others.
Can data in state_query_get_() be a const pointer?
Some of the sr_state_set_* functions can take const pointers.
shared-random-state.h:
I think this comment is in the wrong place, shouldn't it correspond to
`SHARED_RANDOM_STATE_PRIVATE`?
{{{
/* Private methods (only used by shared-random.c): */
}}}
shared-random.c:
{{{
char b64_decoded[SR_COMMIT_LEN + 2];
}}}
Can we either fix #17868 as part of this branch, or add a #define for the
rounding-up that has to occur when the base64 value isn't padded?
{{{
#define B64_DECODE_PAD(len) ((len) + ((len) % 3))
char b64_decoded[B64_DECODE_PAD(SR_COMMIT_LEN)];
}}}
Is TIMESTAMP first, or H(REVEAL)?
commit_decode says one thing in the comments, and another in the code.
Can we assert that `decoded_len == SR_COMMIT_LEN`? Is that useful?
reveal_decode() duplicates commit_decode(). Can we refactor to avoid that?
In sr_parse_srv(), why do we use base16 for the SRV, but base64 for the
commits?
In sr_parse_commit(), we ignore extra arguments after the fourth. Is this
intentional?
A comment either way would help.
I have 5 more commits to review. (Please add fixups to the end of the
branch so I don't lose my place.)
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/16943#comment:22>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs