Opened 2 years ago

Closed 2 years ago

#23368 closed enhancement (implemented)

Add design and coding guidelines for using floating point

Reported by: teor Owned by: catalyst
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: doc, tor-safety, review-group-23
Cc: catalyst, mcs Actual Points:
Parent ID: Points: 0.5
Reviewer: Sponsor:

Description

We should add these to a document in doc/HACKING:

  1. Don't use floats.
  2. If you must use floats, document how the limits of floating point precision and calculation accuracy affect function outputs.
  3. Remember that different environments can get different results from the same floating point calculations. So you can't use floats in anything that needs to be deterministic, like consensus generation.

Child Tickets

Change History (13)

comment:1 Changed 2 years ago by teor

Status: newneeds_review

comment:2 Changed 2 years ago by teor

  1. Try to do as many of your calculations in integer precision as possible, and convert to floating point for display or transfer.

It might also help to link to a general list of do's and don't's, like:

  • don't compare floating-point numbers using equals (comparing with 0.0 is an exception)
  • always check for division by zero, even if the inputs to a function are non-zero, the result can round to zero

comment:3 in reply to:  2 Changed 2 years ago by yawning

Replying to teor:

  1. Try to do as many of your calculations in integer precision as possible, and convert to floating point for display or transfer.

"No non-serialized floating point values on the wire."

comment:4 Changed 2 years ago by catalyst

Cc: catalyst added

comment:5 Changed 2 years ago by mcs

Cc: mcs added

comment:7 Changed 2 years ago by nickm

Keywords: review-group-23 added

Put 0.3.2 needs_review and merge_ready tickets into review-group-23.

comment:8 Changed 2 years ago by catalyst

Owner: set to catalyst
Status: needs_reviewaccepted

There's no patch here so I'll work on one.

comment:9 Changed 2 years ago by catalyst

Status: acceptedneeds_review

comment:10 in reply to:  9 Changed 2 years ago by teor

Status: needs_reviewneeds_revision

Replying to catalyst:

Proposed patch in https://oniongit.eu/catalyst/tor/merge_requests/1

Looks great!

T1. It might help to be more specific here:

   - If you must send floating point numbers on the wire, serialize
     them in a platform-independent way. Tor avoids exchanging
     floating-point values. But when it does, it uses ASCII numerals,
     with a decimal point ("."). This is the "C" Locale Format.

For reference, I could only find one floating-point value in all the directory documents.
In extra-info documents, Tor uses the following C code:

printf("epsilon=%.2f", EPSILON_CONSTANT)

Which produces:

epsilon=0.30

T2. I'd also like to add some more notes like this:

   - Changing the order of operations changes the results of many
     floating-point calculations. Be careful when you simplify
     calculations! If the order is significant, document it using
     a code comment.
   - Comparing most floating point values for equality is unreliable.
     Avoid using `==`, instead, use `>=` or `<=`.
   - Testing floating-point outputs in unit tests is hard. Tests
     that work on your machine can fail in other environments.

Once you've made these changes, feel free to flip this to "ready to merge".

comment:11 Changed 2 years ago by catalyst

These are all good suggestions! I'm trying to figure out how to include them without making the section too long.

comment:12 Changed 2 years ago by catalyst

Status: needs_revisionneeds_review

Pushed an update to oniongit that incorporates these suggestions.

comment:13 Changed 2 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Looks like a good, comprehensive start to me. I've merged it to master. We can add more here as we go along. Thanks!

Note: See TracTickets for help on using tickets.