Skip to content
Snippets Groups Projects

prop250 - review 1

Closed David Goulet requested to merge ticket16943_029_05 into master

From last review, those two are not fixed and there is a reason why. Just click on them to see what is it about.

d20227c6

f8fff1f9

Merge request reports

Closed by avatar (Mar 13, 2025 8:08pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • David Goulet Added 41 commits:

    Added 41 commits:

    • 5d0656ed...a38dfc7f - 4 commits from branch master
    • 694f1fe8 - write v3-status-votes file earlier in consensus voting
    • 5a831229 - Authorities now sort the "package" lines in their votes
    • 2fa7a3af - Make advisory-warnings on by default.
    • 771ca7c5 - Stop recommending --enable-gcc-warnings in doc/HACKING
    • d875101e - Functionify code that writes votes to disk.
    • 02383ea7 - Forward-port the 0.2.8.3-alpha changelog
    • f2580640 - Bump to 0.2.8.3-alpha-dev
    • a93b6bbf - Merge branch 'maint-0.2.8'
    • 3934e78b - Make format_changelog.py add links to bugs
    • 476714e1 - Merge remote-tracking branch 'arma/bug18840'
    • 437cbb17 - Merge remote-tracking branch 'asn/feature19036'
    • a0dd8360 - Merge remote-tracking branch 'public/ticket19044'
    • e6c15166 - Add tor_htonll/ntohll functions
    • 06edc359 - prop250: Add memory and disk state in new files
    • 5f315079 - prop250: Add commit and SR values generation code
    • 393f52d7 - prop250: Put commits and SRVs in votes/consensus
    • d0cc8ebe - prop250: Parse votes and consensus
    • 6dcf46b9 - prop250: Initialize the SR subsystem and us it!
    • c8c870f2 - prop250: Add unit tests
    • 33dbcf24 - prop250: Add changes file
    • c3d39e1d - prop250: change time_t to uint64_t
    • 08462ac0 - prop250: Use RSA identity digest instead of fingerprint
    • ac5b3012 - prop250: Add a valid flag to sr_commit_t
    • b455a8c0 - prop250: Add version to Commit line in vote and state
    • 447daf9c - prop250: Only trust known authority when computing SRV
    • de7f6886 - prop250: Sort smartlist before you get most frequent SRV.
    • 7425fb0d - prop250: Improve log messages
    • 7260fafc - prop250: Sort commits in lexicographical order in votes
    • 770c15ef - prop250: Fix unit tests about the RSA fingerprint check
    • 4180cacd - prop250: Pass the dst length to sr_srv_encode()
    • fb8feae4 - prop250: Don't reject votes containing commits of unknown dirauths.
    • ef9edd8d - prop250: Silence a logging message.
    • 59783303 - prop250: Don't use {0} to init static struct -- causes warning on clang.
    • fc0fb182 - prop250: Change reveal_num to uint64_t and version to uint32_t
    • 0798646a - prop250: Fix format string encoding in log statement
    • 43cd82ac - Refactor parameter computation and add a helper function
    • f7ceeedc - prop250: Use the new dirvote_get_intermediate_param_value for AuthDirNumSRVAgreements
  • David Goulet Added 36 commits:

    Added 36 commits:

    • d9080f5d - Check linking of hardening options, give better warnings if it fails.
    • 11d52a44 - Disable GET /tor/bytes.txt and GETINFO dir-usage
    • 500c4bf8 - remove an unneeded layer of indentation
    • ae4889ac - remove sentence about tor-ops from manpage: #19185
    • ab8f1a9e - Do not warn on missing headers
    • c4049058 - Fix indentation and quotation of the headers
    • 1ce1214d - get rid of one more piece of --enable-instrument-downloads
    • 3b83da10 - remove a now-unused section of or.h
    • 617b9205 - Merge remote-tracking branch 'public/hardening_flags_must_link'
    • 1e5ad156 - Merge remote-tracking branch 'arma/task19035-fixedup'
    • 87593702 - roger says this url is better
    • fb9975bd - Add tor_htonll/ntohll functions
    • 8c622f62 - prop250: Add memory and disk state in new files
    • ba9305bc - prop250: Add commit and SR values generation code
    • 761c6a20 - prop250: Put commits and SRVs in votes/consensus
    • 2a63c13e - prop250: Parse votes and consensus
    • ec6aa3db - prop250: Initialize the SR subsystem and us it!
    • c2aee71e - prop250: Add unit tests
    • 768ad8b2 - prop250: Add changes file
    • cd7fcf1b - prop250: change time_t to uint64_t
    • 3cca08a0 - prop250: Use RSA identity digest instead of fingerprint
    • 40fe1a33 - prop250: Add a valid flag to sr_commit_t
    • 3d15f446 - prop250: Add version to Commit line in vote and state
    • a511cbdd - prop250: Only trust known authority when computing SRV
    • 7327a7b0 - prop250: Sort smartlist before you get most frequent SRV.
    • baddfe4f - prop250: Improve log messages
    • 59bbc609 - prop250: Sort commits in lexicographical order in votes
    • 568c67c2 - prop250: Fix unit tests about the RSA fingerprint check
    • 1e8f9f5b - prop250: Pass the dst length to sr_srv_encode()
    • d4e63910 - prop250: Don't reject votes containing commits of unknown dirauths.
    • 64727034 - prop250: Silence a logging message.
    • 7521cb5c - prop250: Don't use {0} to init static struct -- causes warning on clang.
    • c00ba6dd - prop250: Change reveal_num to uint64_t and version to uint32_t
    • 58db0e50 - prop250: Fix format string encoding in log statement
    • ed96b568 - Refactor parameter computation and add a helper function
    • 13342477 - prop250: Use the new dirvote_get_intermediate_param_value for AuthDirNumSRVAgreements
  • David Goulet Added 4 commits:

    Added 4 commits:

    • b688be18 - prop250: Change reveal_num to uint64_t and version to uint32_t
    • 4b4db3c4 - prop250: Fix format string encoding in log statement
    • cfd961c2 - Refactor parameter computation and add a helper function
    • 426b5f85 - prop250: Use the new dirvote_get_intermediate_param_value for AuthDirNumSRVAgreements
  • David Goulet Added 24 commits:

    Added 24 commits:

    • 7ea7ff41 - prop250: Add memory and disk state in new files
    • c8d4f7fe - prop250: Add commit and SR values generation code
    • 1637a647 - prop250: Put commits and SRVs in votes/consensus
    • 5effbebc - prop250: Parse votes and consensus
    • 76ddc732 - prop250: Initialize the SR subsystem and us it!
    • 08dd9f53 - prop250: Add unit tests
    • 07347d92 - prop250: Add changes file
    • 89a42dfa - prop250: change time_t to uint64_t
    • c1d5d260 - prop250: Use RSA identity digest instead of fingerprint
    • 042bfd70 - prop250: Add a valid flag to sr_commit_t
    • 4937cdee - prop250: Add version to Commit line in vote and state
    • 383a5d0b - prop250: Only trust known authority when computing SRV
    • 10c2ad91 - prop250: Sort smartlist before you get most frequent SRV.
    • 68c63142 - prop250: Improve log messages
    • 2cb39791 - prop250: Sort commits in lexicographical order in votes
    • 3a409ce1 - prop250: Fix unit tests about the RSA fingerprint check
    • e3c7c0ca - prop250: Pass the dst length to sr_srv_encode()
    • 3ebf8945 - prop250: Don't reject votes containing commits of unknown dirauths.
    • cce0f66e - prop250: Silence a logging message.
    • c73e2689 - prop250: Don't use {0} to init static struct -- causes warning on clang.
    • 09b6be5a - prop250: Change reveal_num to uint64_t and version to uint32_t
    • 59f38813 - prop250: Fix format string encoding in log statement
    • 41d6e481 - Refactor parameter computation and add a helper function
    • bc678f80 - prop250: Use the new dirvote_get_intermediate_param_value for AuthDirNumSRVAgreements
  • David Goulet Added 1 commit:

    Added 1 commit:

    • 14e65c3e - fixup! prop250: Add memory and disk state in new files
  • David Goulet Added 24 commits:

    Added 24 commits:

    • 164a720b - prop250: Add memory and disk state in new files
    • ec7b8611 - prop250: Add commit and SR values generation code
    • d26b0c27 - prop250: Put commits and SRVs in votes/consensus
    • b7bef6bb - prop250: Parse votes and consensus
    • 8708c5c9 - prop250: Initialize the SR subsystem and us it!
    • 5876abb5 - prop250: Add unit tests
    • 99afdc44 - prop250: Add changes file
    • e0cdceea - prop250: change time_t to uint64_t
    • d36f83ce - prop250: Use RSA identity digest instead of fingerprint
    • a01ecd4e - prop250: Add a valid flag to sr_commit_t
    • 9f04560f - prop250: Add version to Commit line in vote and state
    • 820c5bc5 - prop250: Only trust known authority when computing SRV
    • 08831b9f - prop250: Sort smartlist before you get most frequent SRV.
    • 10b9fee6 - prop250: Improve log messages
    • a3d5c9cc - prop250: Sort commits in lexicographical order in votes
    • d6ceb85b - prop250: Fix unit tests about the RSA fingerprint check
    • 239f0eff - prop250: Pass the dst length to sr_srv_encode()
    • c5c430cf - prop250: Don't reject votes containing commits of unknown dirauths.
    • 24e06526 - prop250: Silence a logging message.
    • 976e7c43 - prop250: Don't use {0} to init static struct -- causes warning on clang.
    • 5724059c - prop250: Change reveal_num to uint64_t and version to uint32_t
    • 4e9bfdab - prop250: Fix format string encoding in log statement
    • 8137e429 - Refactor parameter computation and add a helper function
    • d0b20185 - prop250: Use the new dirvote_get_intermediate_param_value for AuthDirNumSRVAgreements
  • David Goulet Added 30 commits:

    Added 30 commits:

    • 57bf8bb2 - remove now-irrelevant XXX020 comments in configure.ac
    • ce31db43 - We no longer generate v0 directories. Remove the code to do so
    • 4f1a04ff - Replace nearly all XXX0vv comments with smarter ones
    • 97f2c1c5 - Wait, we had sprintf() in our unit tests?? FOR SHAME!
    • 3cdc8bfa - Let's not even talk about those errors, ok?
    • a94e9970 - Add tor_htonll/ntohll functions
    • e94db0a7 - prop250: Add memory and disk state in new files
    • 17077155 - prop250: Add commit and SR values generation code
    • 145ad2dc - prop250: Put commits and SRVs in votes/consensus
    • 3ba523c3 - prop250: Parse votes and consensus
    • bd42caae - prop250: Initialize the SR subsystem and us it!
    • 51e57aee - prop250: Add unit tests
    • f375d47e - prop250: Add changes file
    • 93232c15 - prop250: change time_t to uint64_t
    • 2e283ee2 - prop250: Use RSA identity digest instead of fingerprint
    • 15c2310f - prop250: Add a valid flag to sr_commit_t
    • 323fffd4 - prop250: Add version to Commit line in vote and state
    • 6a44818a - prop250: Only trust known authority when computing SRV
    • 7d56587b - prop250: Sort smartlist before you get most frequent SRV.
    • 126542bb - prop250: Improve log messages
    • deb24c0a - prop250: Sort commits in lexicographical order in votes
    • ef79e6e3 - prop250: Fix unit tests about the RSA fingerprint check
    • 8f61ddbe - prop250: Pass the dst length to sr_srv_encode()
    • 4021b3fa - prop250: Don't reject votes containing commits of unknown dirauths.
    • 4e96c5b8 - prop250: Silence a logging message.
    • 736f17cb - prop250: Don't use {0} to init static struct -- causes warning on clang.
    • 1a65bb3d - prop250: Change reveal_num to uint64_t and version to uint32_t
    • 14a0001a - prop250: Fix format string encoding in log statement
    • f604166f - Refactor parameter computation and add a helper function
    • ca8757f5 - prop250: Use the new dirvote_get_intermediate_param_value for AuthDirNumSRVAgreements
  • David Goulet Added 111 commits:

    Added 111 commits:

    • 0279e484 - Add support for temporarily suppressing a warning
    • 0df2c567 - Use ENABLE_GCC_WARNING and DISABLE_GCC_WARNING in tortls.c
    • ce1dbbc4 - Enable the -Waggregate-return warning
    • 55b5e007 - Add another 22 or so GCC warnings. None currently triggers for me.
    • bdc59e33 - Fix a warning on unnamed nodes in node_get_by_nickname().
    • 5854b198 - Use tor_sscanf, not sscanf, in test_util.c.
    • a32ca313 - Merge branch 'maint-0.2.7' into maint-0.2.8
    • b458a81c - Merge branch 'maint-0.2.8'
    • ed0ecd9f - Use tor_sscanf, not sscanf, in test_crypto.c
    • 6eeedc02 - Use directory_must_use_begindir to predict we'll surely use begindir
    • 83513a93 - Check tor_sscanf return value in test_crypto.c
    • c19a3d1b - Merge branch 'maint-0.2.8'
    • 1e330e19 - Repair test_crypto_openssl_version with LibreSSL
    • 36dd9538 - Don't rely on consensus parameter to use a single guard.
    • 9eeaeddb - Reduce make check-spaces noise
    • c274f825 - Merge remote-tracking branch 'asn/bug17688'
    • 4f8086fb - Enable -Wnull-dereference (GCC >=6.1), and fix the easy cases
    • b14c1f40 - Merge remote-tracking branch 'public/bug19203_027' into HEAD
    • 12517c73 - Add -Wduplicated-cond on GCC 6
    • 2ff20c93 - Add -Wunused-const-variable=2 on GCC >=6.1
    • 493499a3 - Add -Wfloat-conversion for GCC >= 4.9
    • 8f2d2933 - Use -Wdouble-promotion in GCC >= 4.6
    • 4caed242 - Enable -Woverlength-strings for GCC>=4.6 on MOST of the code.
    • ad16c552 - Use -Wstrict-overflow=2 on gcc5+.
    • 9bbd6502 - Use autoconf, not gcc version, to decide which warnings we have
    • 15533c88 - Set our autoconf-breaking options last, not before we check for others
    • 26e979b3 - Add all the clang-only warnings that do not trigger now
    • c3adbf75 - Resolve some warnings from OSX clang.
    • 80f1a2cb - Add the -Wextra-semi warning from clang, and fix the cases where it triggers
    • 53a3b39d - Add -Wmissing-variable-declarations, with attendant fixes
    • e80a032b - Add clang's -Wstring-conversion, and fix the one place it hits
    • d6b2af7a - Merge branch 'bug19180_easy_squashed'
    • ada5668c - Merge remote-tracking branch 'public/bug19203_027' into maint-0.2.8
    • 841d1b25 - Merge branch 'maint-0.2.8'
    • cb71c5dd - Whoops -- this got lost in the merge.
    • 47edbd4f - Fix build on 32-bit systems.
    • 80f2c355 - Remove -Wc11-extensions
    • c14c6627 - Update geoip and geoip6 to the June 7 2016 database.
    • 0616fd6f - typo/comment/log fixes i found in my sandbox from montreal
    • 925f76b4 - Keep make check-spaces happy
    • f25f7b75 - Merge branch 'maint-0.2.4' into maint-0.2.5
    • b4bb8860 - Merge branch 'maint-0.2.5' into maint-0.2.6
    • 80089c9e - Merge branch 'maint-0.2.6' into maint-0.2.7
    • 2ee3dbe8 - Merge branch 'maint-0.2.7' into maint-0.2.8
    • 6a7d11f3 - Merge branch 'maint-0.2.8'
    • 4c90cdc0 - Coverity dislikes (double) (int/int).
    • 60b8aaef - lintChanges fixes
    • a427a7c4 - Merge branch 'maint-0.2.8'
    • 86f0b806 - Bug 19406: OpenSSL changed the Thread API in 1.1.0 again.
    • b563a3a0 - Bug 19406: OpenSSL made RSA and DH opaque in 1.1.0.
    • 6ddef1f7 - Bug 19406: OpenSSL removed SSL_R_RECORD_TOO_LARGE in 1.1.0.
    • c5e2f7b9 - Bug 19406: Fix the unit tests to work with OpenSSL 1.1.x
    • b217e4ac - Bug 19406: Add a changes file.
    • 71aacbe4 - Suppress the Wredundant-decls warning in another set of openssl headers
    • df4fa92a - Merge branch 'maint-0.2.8'
    • 3bffdf05 - use new-form macros to disable -Wredundant-decls
    • 99a7ddd6 - Disable -Wc99-c11-compat
    • e54f8e34 - Remove some duplicated warnings from the big list
    • d6b01211 - Resolve the remaining openssl "-Wredundant-decls" warnings.
    • 8486dea8 - Merge branch 'maint-0.2.8'
    • 227d3b3d - Use ENABLE/DISABLE_GCC_WARNING in masater.
    • 05e2750e - whoops; blank line
    • 3a0d42fb - bump version to 0.2.8.4-rc
    • 657067ef - Merge branch 'maint-0.2.8'
    • c7f1b46a - Perform cache lookup when FetchHidServDescriptors is set
    • e718a582 - Bump to 0.2.8.4-rc-dev
    • 2042080b - ondrej is no longer making rpms
    • 4bcec3ba - Merge branch 'maint-0.2.8'
    • 327d4c11 - forward-port changelog
    • 7b54d7eb - Mark src/common tor_assert(0)/tor_fragile_assert() unreached for coverage
    • f986e268 - Reach 100% line coverage on memarea.c
    • d1ab295d - add LCOV_EXCL for unreachable exit() blocks in src/common
    • 6dc2b605 - Improve coverage on esc_for_log, esc_for_log_len
    • f05a213f - Improve coverage on tv_udiff, and tv_mdiff.
    • 41cb26c1 - Correct the rounding behavior on tv_mdiff.
    • 79370914 - tests for size_mul_check__()
    • 9b0bd65f - I believe I found some dead code in our time parsing functions
    • 5c596cdb - Tests for message rate-limiting
    • 6ceb3797 - Try to fix memarea test on 32-bit systems
    • ab35f9de - Correctly close intro circuit when deleting ephemeral HS
    • dd737871 - On Windows, tv_sec is long, not time_t.
    • 128ab31c - Mark code unreachable in unescape_string()
    • c9ea9de8 - Remove parse_config_line_from_str alias
    • a4189049 - Coverage on parse_config_line_from_str_verbose.
    • a8c76622 - Mark an abort() as unreachable.
    • 9a63f059 - Merge remote-tracking branch 'dgoulet/bug18604_029_01'
    • e400e135 - Add tor_htonll/ntohll functions
    • 8eba1137 - prop250: Add memory and disk state in new files
    • a6ff6bc5 - prop250: Add commit and SR values generation code
    • 77c2ca3f - prop250: Put commits and SRVs in votes/consensus
    • c6ddac8b - prop250: Parse votes and consensus
    • 3a8e4674 - prop250: Initialize the SR subsystem and us it!
    • bed851a1 - prop250: Add unit tests
    • fd75e0c6 - prop250: Add changes file
    • a4cb1337 - prop250: change time_t to uint64_t
    • c780dc90 - prop250: Use RSA identity digest instead of fingerprint
    • 04100c60 - prop250: Add a valid flag to sr_commit_t
    • 1f5a0c7f - prop250: Add version to Commit line in vote and state
    • 5a394a93 - prop250: Only trust known authority when computing SRV
    • 95fa859d - prop250: Sort smartlist before you get most frequent SRV.
    • 1ccd5844 - prop250: Improve log messages
    • f0964593 - prop250: Sort commits in lexicographical order in votes
    • d1c04333 - prop250: Fix unit tests about the RSA fingerprint check
    • 564e85dc - prop250: Pass the dst length to sr_srv_encode()
    • f9f2e415 - prop250: Don't reject votes containing commits of unknown dirauths.
    • 9c715ae8 - prop250: Silence a logging message.
    • 9441a834 - prop250: Don't use {0} to init static struct -- causes warning on clang.
    • 67f9c643 - prop250: Change reveal_num to uint64_t and version to uint32_t
    • 8eb1da18 - prop250: Fix format string encoding in log statement
    • 3371dd3b - Refactor parameter computation and add a helper function
    • 81652726 - prop250: Use the new dirvote_get_intermediate_param_value for AuthDirNumSRVAgreements
  • David Goulet Added 54 commits:

    Added 54 commits:

    • 3cc37445 - Add several test scripts wrapping test_workqueue
    • 429d15c5 - Mark the unreachable lines in compat_{,p}threads and workqueue
    • f016213f - Unit tests for our zlib code to test and reject compression bombs.
    • 94762e37 - Use the Autoconf macro AC_USE_SYSTEM_EXTENSIONS
    • 4e4a7d2b - Fix base32 API to take any source length in bytes
    • f4f9a9be - test: Add base32_encode/decode unit tests
    • 48b25e68 - Merge branch 'bug18280_029_03_nm_squashed'
    • 033cf30b - Keep make check-spaces happy
    • 2c96d95c - Fix spelling of --enable-tor2web-mode in manpage
    • b421648d - Merge remote-tracking branch 'public/thread_coverage'
    • 80801531 - Remove support for zlib <= 1.1
    • 358fc026 - Remove a ridiculous realloc call from torgzip.c
    • 5a725dab - Mark some torgzip lines as unreachable/untestable.
    • d937b866 - Unindent block
    • 81cfd5c9 - Merge branch 'zlib_coverage_squashed'
    • 58e6a6aa - Fix #19063: Add check in utility macro
    • 1160ac12 - Changes file for 19063; use the BUG macro
    • 568dc27a - Make base16_decodes return number of decoded bytes
    • 6cedd493 - Merge branch 'bug14013_029_01_squashed'
    • 85edef27 - test: Increase offset to rendcache descriptor time
    • ba88d781 - Fix unit test crash on 32-bit.
    • c1f0ec30 - Merge remote-tracking branch 'dgoulet/bug19465_029_01'
    • 2b74e13a - More coverage in backtrace.c
    • ba28da8d - compat.c coverage: simplify under-tested alloc_getcwd.
    • 603cb712 - Small coverage improvements on compat.c
    • 5fbd1959 - Coverage hack for test_switch_id.sh
    • 2f75b34d - Patch from dgoulet: fix a base16 problem that manifested w stem
    • 400b4250 - mark sanitize_blacklist.txt as obsolete
    • 49e8f475 - util: zero target buffer of base*_encode/decode
    • 9744a40f - Add tor_htonll/ntohll functions
    • b3b4ffce - prop250: Add memory and disk state in new files
    • f0711641 - prop250: Add commit and SR values generation code
    • b1397015 - prop250: Put commits and SRVs in votes/consensus
    • 9cce7fbe - prop250: Parse votes and consensus
    • d6f33429 - prop250: Initialize the SR subsystem and us it!
    • 94d4a086 - prop250: Add unit tests
    • beedf362 - prop250: Add changes file
    • ee5398a1 - prop250: change time_t to uint64_t
    • ac6ad8ba - prop250: Use RSA identity digest instead of fingerprint
    • 55795965 - prop250: Add a valid flag to sr_commit_t
    • c14f77c4 - prop250: Add version to Commit line in vote and state
    • 75b5710d - prop250: Only trust known authority when computing SRV
    • 5f455974 - prop250: Sort smartlist before you get most frequent SRV.
    • 2a7dafe9 - prop250: Improve log messages
    • 1b4d7f63 - prop250: Sort commits in lexicographical order in votes
    • aff5b810 - prop250: Fix unit tests about the RSA fingerprint check
    • 267b065b - prop250: Pass the dst length to sr_srv_encode()
    • 371a62bb - prop250: Don't reject votes containing commits of unknown dirauths.
    • 14fe6808 - prop250: Silence a logging message.
    • e3797451 - prop250: Don't use {0} to init static struct -- causes warning on clang.
    • 93ec0608 - prop250: Change reveal_num to uint64_t and version to uint32_t
    • ad5a9be9 - prop250: Fix format string encoding in log statement
    • bddbbb8b - Refactor parameter computation and add a helper function
    • 483a8126 - prop250: Use the new dirvote_get_intermediate_param_value for AuthDirNumSRVAgreements
  • Nick Mathewson
    Nick Mathewson @nickm_tor started a thread on commit b3b4ffce
892 {
893 state_query(SR_STATE_ACTION_PUT, SR_STATE_OBJ_VALID_AFTER,
894 (void *) &valid_after, NULL);
895 }
896
897 /* Return the phase we are currently in according to our state. */
898 sr_phase_t
899 sr_state_get_phase(void)
900 {
901 void *ptr;
902 state_query(SR_STATE_ACTION_GET, SR_STATE_OBJ_PHASE, NULL, &ptr);
903 return *(sr_phase_t *) ptr;
904 }
905
906 /* Return the previous SRV value from our state. Value CAN be NULL. */
907 sr_srv_t *
  • I wanted this to return const, because if it doesn't, we are totally violating the supposed guarantee that these structures can only change. When you return a mutable view of the interior of a structure, people can change it out from under you.

    I guess we can give this a pass for now (since the code depends on it) but it is totally an abstraction violation and we should fix it some time.

  • Commit 6a29928.

    I'm not entirely sure we had in mind guarantees of mutability for those structures because for instance we fetch a saved commit from our state during the reveal phase and we update it with the reveal value. So we kind of need a "get_commit()" that doesn't return a const unless we had a specific function that only updates a field in the commit object through the state API.

    Anyway, the get previous/current SRV are now const and a delete function has been added.

  • Nick Mathewson
    Nick Mathewson @nickm_tor started a thread on commit 55795965
  • 48 48 #define SR_SRV_VALUE_BASE64_LEN \
    49 49 (((DIGEST256_LEN - 1) / 3) * 4 + 4)
    50 50
    51 /* Assert if commit valid flag is not set. */
    52 #define ASSERT_COMMIT_VALID(c) tor_assert(c->valid)
  • Nick Mathewson
    Nick Mathewson @nickm_tor started a thread on commit c14f77c4
  • 1048 1052 char *value, digest[DIGEST_LEN];
    1049 1053 digest_algorithm_t alg;
    1050 1054 const char *rsa_identity_fpr;
    1051 1055 sr_commit_t *commit = NULL;
    1052 1056
    1053 if (smartlist_len(args) < 3) {
    1057 if (smartlist_len(args) < 4) {
    1054 1058 goto error;
    1055 1059 }
    1056 1060
    1057 /* First argument is the algorithm. */
    1061 /* First is the version number of the SR protocol which indicates at which
    1062 * version that commit was created. */
    1058 1063 value = smartlist_get(args, 0);
    1064 version = (uint32_t) tor_parse_ulong(value, 10, 1, UINT32_MAX, NULL, NULL);
    1065 if (version < SR_PROTO_VERSION) {
  • Nick Mathewson
    Nick Mathewson @nickm_tor started a thread on commit 371a62bb
  • 1108 1118 escaped(rsa_identity_fpr));
    1109 1119 goto error;
    1110 1120 }
    1111 /* Let's make sure, for extra safety, that this fingerprint is known to
    1112 * us. Even though this comes from a vote, doesn't hurt to be
    1113 * extracareful. */
    1114 if (trusteddirserver_get_by_v3_auth_digest(digest) == NULL) {
    1115 log_warn(LD_DIR, "SR: Fingerprint %s is not from a recognized "
    1116 "authority. Discarding commit.",
    1117 escaped(rsa_identity_fpr));
    1118 goto error;
    • Are there any other cases where sr_parse_commit should allow us to accept the rest of the vote, when it currently does "goto error" ?

    • This has been moved to should_keep_commit() for which we now ignore the commit instead of rejecting the vote.

      See commit: 371a62bb

    • David, based on Nick's comment and for the purposes of forwards-compatibility, it might make sense to reconsider whether we want to goto error in cases like the following:

        if (version < SR_PROTO_VERSION) {
          log_info(LD_DIR, "SR: Commit version %" PRIu32 " (%s) is not supported.",
                   version, escaped(value));
          goto error;
        }
        if (alg != SR_DIGEST_ALG) {
          log_warn(LD_BUG, "SR: Commit algorithm %s is not recognized.",
                   escaped(value));
          goto error;
        }
        if (strlen(encoded) > SR_REVEAL_BASE64_LEN) {
          /* This means that if we base64 decode successfully the received reveal
           * value, we'll end up with a bigger decoded value thus unusable. */
          goto error;
        }
    • Indeed, in this case we have to goto error because it's the parsing commit function which also parses the state from disk. Ultimately, when handling the votes, this is called by extract_shared_random_commits(). So if we want forward compat, being unable to parse the commit from the vote would be exactly like ignoring an unknown line thus we shouldn't reject the vote. This below shouldn't goto err then:

            if (extract_shared_random_commits(ns, tokens) < 0) {
              log_warn(LD_DIR, "SR: Unable to parse commits in vote.");
              goto err;
            }
      

      ... and probably this also:

          if (extract_shared_random_srvs(ns, tokens) < 0) {
            log_warn(LD_DIR, "SR: Unable to parse SRV(s)");
            goto err;
          }

      Furthermore, we shouldn't restrict the "extract commit function" to a hardcoded number of arguments in extract_shared_random_commits().

          if (tok->n_args < 4 || tok->n_args > 5) {
            goto err;
          }

      What we want is to extract all tokens, put them in a list and send it to the parse commit function which will decide if it's a good line or not. We've added the version number as the very first arg to the commit so having this hard check on number of args. defies this forward compat purpose with a version in front!

      I'll cook a patch to address this, I think it's going to be much better and cleaner. I'll let you know which fixup commit ID to look at.

    • I like it more like this, see fixup commit: 0ffaa65

  • Nick Mathewson
    Nick Mathewson @nickm_tor started a thread on commit 93ec0608
  • 418 418 /* Add the invariant token. */
    419 419 memcpy(msg, SR_SRV_TOKEN, SR_SRV_TOKEN_LEN);
    420 420 offset += SR_SRV_TOKEN_LEN;
    421 set_uint8(msg + offset, reveal_num);
    421 set_uint64(msg + offset, reveal_num);
    422 422 offset += 1;
    • Shouldn't this be += 8?

    • And don't you need an htonll?

    • Oh crap... GREAT catch with the offset... a value bigger than uint8_t would have be super problematic... Fortunately, reveal_num here is never above 9 (nine dirauth) and the version is 1

      This is for the construction of the SRV which looks like this so no we don't need htonll() since it's not going on the wire:

       *    SRV = SHA3-256("shared-random" | INT_8(reveal_num) |
       *                   INT_4(version) | HASHED_REVEALS | previous_SRV)

      Fixed in b3a4d6e

    • I think you do need htonll, though. Otherwise big-endian systems and little-endian systems will encode this value differently, right?

    • Hrm, yes I think you are right, my mistake. Fixed in caf2c44

      (Note that there is a ticket I'll open once this branch is merged about using trunnel for this! :)

  • Nick Mathewson
    Nick Mathewson @nickm_tor started a thread on commit 93ec0608
  • 418 418 /* Add the invariant token. */
    419 419 memcpy(msg, SR_SRV_TOKEN, SR_SRV_TOKEN_LEN);
    420 420 offset += SR_SRV_TOKEN_LEN;
    421 set_uint8(msg + offset, reveal_num);
    421 set_uint64(msg + offset, reveal_num);
    422 422 offset += 1;
    423 set_uint8(msg + offset, SR_PROTO_VERSION);
    423 set_uint32(msg + offset, SR_PROTO_VERSION);
    424 424 offset += 1;
  • David Goulet Added 6 commits:

    Added 6 commits:

    • 6a29928f - prop250: Add a DEL state action and return const SRVs
    • aaf6bacd - fixup! prop250: Add a valid flag to sr_commit_t
    • 017d21fb - fixup! prop250: Add version to Commit line in vote and state
    • 4bb7d843 - fixup! prop250: Add commit and SR values generation code
    • b3a4d6e8 - fixup! prop250: Change reveal_num to uint64_t and version to uint32_t
    • a1d5378b - fixup! prop250: Add unit tests
  • Nick Mathewson
    Nick Mathewson @nickm_tor started a thread on commit aaf6bacd
  • 49 49 (((DIGEST256_LEN - 1) / 3) * 4 + 4)
    50 50
    51 51 /* Assert if commit valid flag is not set. */
    52 #define ASSERT_COMMIT_VALID(c) tor_assert(c->valid)
    52 #define ASSERT_COMMIT_VALID(c) (tor_assert(c->valid))
  • Nick Mathewson
    Nick Mathewson @nickm_tor started a thread on commit a1d5378b
  • 783 783 const sr_srv_t *current_srv = NULL;
    784 784
    785 785 #define SRV_TEST_VECTOR \
    786 "C3E2E6F48E541F8CB47DAC097050D09ED0AFE0CAC52AF129FCD3D20166B70463"
    786 "F3946A6AED72BA529527A52D65C9E8A2C2E34C2E59974F3BD4F9DC409ACC5905"
    • Does this test vector just assume that whatever the code does now must be correct? Or was it constructed by some other process?

    • Ehm, currently this test indeed assumes that whatever the code does now must be correct... Not good, I agree.

      When we started the project, I implemented the SRV calculation in Python for this test, and that's how the test vector was initially produced. However when we switched from SHA2 to SHA3 I did not update the Python version. It can still be found here: https://gitweb.torproject.org/user/asn/tor.git/tree/src/test/sr_srv_calc_ref.py?h=prop250_final_v4 and it might be worth revising.

      I can try to do this at some point next week if you guys think it's worthwhile.

    • Agree. I'm giving it a shot now.

    • Ok, I manage to adjust it to SHA3 with some tweaks. Unfortunately, only python 3.6 has SHA3 natively in hashlib package so for now you have to install pysha3 >= 1.0. The python script makes a check prior to make sure it uses the right version.

      See fixup commit 2bbd8c3

  • David Goulet Added 4 commits:

    Added 4 commits:

    • fa9de2e0 - fixup! prop250: Add a valid flag to sr_commit_t
    • caf2c447 - fixup! prop250: Add commit and SR values generation code
    • 2bbd8c37 - fixup! prop250: Add unit tests
    • 0ffaa659 - fixup! prop250: Parse votes and consensus
  • David Goulet Added 4 commits:

    Added 4 commits:

    • fa9de2e0 - fixup! prop250: Add a valid flag to sr_commit_t
    • caf2c447 - fixup! prop250: Add commit and SR values generation code
    • 2bbd8c37 - fixup! prop250: Add unit tests
    • 0ffaa659 - fixup! prop250: Parse votes and consensus
  • David Goulet Added 1 commit:

    Added 1 commit:

    • 14b027c8 - fixup! prop250: Parse votes and consensus
  • David Goulet Status changed to closed

    Status changed to closed

  • Please register or sign in to reply
    Loading