Feat/mul div - #54
Feat/mul div#54
Conversation
| /// Panics if the result overflows a u256 | ||
| pub fn mul_div_256(a: u256, b: u256, denominator: u256) -> u256 { | ||
| match is_zero_256(denominator) { | ||
| true => 0, |
There was a problem hiding this comment.
Shouldn't we panic if we divide by 0?
There was a problem hiding this comment.
I agree that panicking on division by zero would be less error-prone, but the jet's division functions don't panic on division by zero, so here it feels more like the default behavior
There was a problem hiding this comment.
We had a discussion on a call with @roconnor-blockstream about this behavior, which other people found unusual or surprising. I think it has to do with some functional programming conventions related to wanting functions to be total when possible. I don't know that he necessarily felt that all library functions written using the jet needed to replicate the behavior, but I don't remember his exact position about this.
I think @apoelstra is also familiar with this discussion and might be able to weigh in.
There was a problem hiding this comment.
For now, mul_div functions will return 0 on division by zero instead of panicking. A more general approach to this topic will be discussed later
There was a problem hiding this comment.
Arguably we should return an error here so that the user can choose what to do (unwrap to get panicking behavior, unwrap_or(0) to get "just use 0" behavior).
I understand there is some mathematical reason to support division by 0 being defined to be 0, which simplifies formal reasoning in some cases.
I nonetheless think that returning surprising or arbitrary values from a contract is a very bad idea. This reminds me very much of the "x mod 0 = 0" bug in Ethereum from 2014 or so that allowed signature validation to be entirely bypassed.
So if we are not going to return an error, we should panic.
There was a problem hiding this comment.
Yeah, I wholeheartedly agree. This behaviour will be fixed in the upcoming PR, I'll send you a link.
moved arithmetic section for u128 and u256 to be consistent with other sections
LesterEvSe
left a comment
LesterEvSe
left a comment
There was a problem hiding this comment.
Also let's add more tests to cover new u512 division functionality.
- Force entry into
algorithm_d_512_256:
a = generate_u256(2^128, U256::MAX)
b = generate_u256(2^128, U256::MAX)
result_high = high 256 bits of a.full_mul(b)
c = generate_u256(max(2^128, result_high + 1), U256::MAX)Guarantees result_high != 0 and denom_high != 0.
- Add two
normalize_to_threshold_512_127tests.
2.1.norm == 1. Forces the divisor's top bit to already be set:
a = generate_u256(2^128, U256::MAX)
b = generate_u256(2^128, U256::MAX)
result_high = high 256 bits of a.full_mul(b)
c = generate_u256(max(2^255, result_high + 1), U256::MAX)2.2. norm > 1. Forces the divisor's top bit to be clear:
a = generate_u256(2^128, U256::MAX)
b = generate_u256(2^128, 2^192)
result_high = high 256 bits of a.full_mul(b) // < 2^192, well under the 2^255 ceiling
c = generate_u256(max(2^128, result_high + 1), 2^255 - 1)-
Same
a, bas (1), butc = result_high + 1. -
Exact division with
remainder == 0. Seta = cfor somecin range[2^128, U256::MAX], pickbasa*b >= 2^256. -
Minimal
denom_high:
a = generate_u256(2^128, 2^129)
b = generate_u256(2^128, 2^129)
result_high = high 256 bits of a.full_mul(b) // in {1, 2, 3}
c = generate_u256(max(2^128, result_high + 1), 2^129 - 1)
LesterEvSe
left a comment
LesterEvSe
left a comment
There was a problem hiding this comment.
LGTM!
But worth mentioning, from time to time I get this error on the test u256_test_mul_div_256_algorithm_d_512_256_c_is_res_high:
Error: Broadcast failed with HTTP 400 for http://127.0.0.1:41877/tx: sendrawtransaction RPC error -26:
non-mandatory-script-verify-flag (Program's execution cost could exceed budget)
Probably worth researching on the simplex side to see how we can solve it
Yeah, me too, and only when I run tests with |
* added mul_div functions with tests * added zero checks for div_128 and div_256 to be consistent with jet's `divide` * added tests for new helper functions * typo in a test name * utilized convert and split functions * added mul_div docs; moved arithmetic section for u128 and u256 to be consistent with other sections * linting * added docs for new functions * fixed typo * fixed bug in normalize_to_threshold funcs * added more tests to cover new u512 division functionality * bumped simplex version * optimization for calculate_normalizer_base_128 * typo fixes * a couple of new tests for calculate_normalizer_base_128
5 participants
For now,
mul_divfunctions return 0 on division by zero instead of panicking. A more general approach to this topic will be discussed later.