Skip to content

Conversation

tomato42
Copy link
Member

@tomato42 tomato42 commented Nov 9, 2020

  • little bit of cleanup after the changes around precomputation code
  • add CI on python 3.9 as it's released now
  • more documentation
  • more test coverage

@tomato42 tomato42 added feature functionality to be implemented maintenance issues related to making the project usable or testable labels Nov 9, 2020
@tomato42 tomato42 self-assigned this Nov 9, 2020
as we now precompute lazily, force the precomputation to happen when
we call the method, now then it's first used; allow also the lazy
option to delay it to the first time point is used (at the price of
first verification latency)
as we don't use the lock now just for scaling the point but for
precomputation too, rename it so it's more apparent
@coveralls
Copy link

coveralls commented Nov 9, 2020

Coverage Status

Coverage increased (+0.03%) to 98.028% when pulling 0dc17f0 on tomato42:precompute-updates into 73f3d3e on warner:master.

@tomato42 tomato42 changed the title Precompute updates Precompute-related updates Nov 9, 2020
the math is the same for NIST P-256 and Brainpool 160r1 curves,
but since the P-256 is larger, it takes longer to process, it in turn
causes random timeouts (tlsfuzzer#206)

decrease the size of numbers to hopefully make it pass CI more
consistently
@tomato42
Copy link
Member Author

@letoams
Copy link

letoams commented Nov 11, 2020

Looks good to me. My only question is what the the precompute has a lazy flag at all. Why not just always have it lazy? Is there a use case where being lazy would be slower?

@tomato42
Copy link
Member Author

yes, the first operation with lazy precompute will be much slower (like few hundred times slower), so if the code is latency sensitive, it may be too late to do it the first time the key is actually used

@tomato42 tomato42 merged commit 23a5b65 into tlsfuzzer:master Nov 11, 2020
@tomato42 tomato42 deleted the precompute-updates branch November 11, 2020 15:20
@letoams
Copy link

letoams commented Nov 11, 2020

yes, the first operation with lazy precompute will be much slower (like few hundred times slower), so if the code is latency sensitive, it may be too late to do it the first time the key is actually used

But is that really a factor ? If performance were that critical, would you use python? Wouldn't this need to use ctypes ?

If you startup a program to do calculations, then you always take this hit anyway before you can "get to work". So the only use case we are talking about is a daemon waiting to do signing operations. Is "a few hundred times slower" meaningful compared to like 20ms of network latency ?

It feels a bit more like feature creep than actual real life use case to me. But having said that, I don't object to any of it.

@tomato42
Copy link
Member Author

yes, the first operation with lazy precompute will be much slower (like few hundred times slower), so if the code is latency sensitive, it may be too late to do it the first time the key is actually used

But is that really a factor ? If performance were that critical, would you use python? Wouldn't this need to use ctypes ?

because on very underperforming platforms (like Raspberry Pi) that few hundred times slower translates for few seconds, see #211 and linked issues

If you startup a program to do calculations, then you always take this hit anyway before you can "get to work". So the only use case we are talking about is a daemon waiting to do signing operations. Is "a few hundred times slower" meaningful compared to like 20ms of network latency ?

yes, on a regular PC it's just 20ms so it doesn't matter, but on a phone it may be 2-3 seconds

It feels a bit more like feature creep than actual real life use case to me. But having said that, I don't object to any of it.

well, yeah, but it's reusing the code we need to library init so... meh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature functionality to be implemented maintenance issues related to making the project usable or testable

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants