[arith_uint256] Make it safe to use "self" in operators #12537

pull kallewoof wants to merge 2 commits into bitcoin:master from kallewoof:uint-safe-self-op changing 2 files +17 −4

kallewoof commented at 6:37 AM on February 26, 2018: member

Before this fix (see test commit), v *= v would result in 0 because operator*= set *this (==b) to 0 at the start. This patch changes the code to use a as temporary for *this~~, with drawback that *this is set to a at the end, an extra = operation in other words~~.
practicalswift commented at 8:04 AM on February 26, 2018: contributor

utACK ac68e6d7176c1b24a69559afd1a66b2d04c67dc6
khalifa4 approved
promag commented at 9:22 AM on February 26, 2018: member

utACK ac68e6d.
ajtowns commented at 3:58 AM on February 28, 2018: member

Removed code invokes copy-constructor, and operator=(uint64_t); new code invokes default-constructor and operator=(base_uint). So doesn't seem like an extra = operation to me?

Shouldn't the fix be committed first, then the tests? Or is having an intermediate commit where the tests fail ok? Looks good to me otherwise though.
kallewoof commented at 4:18 AM on February 28, 2018: member

@ajtowns I kinda wanted to demonstrate that it fails up until that point, which is why I put the tests before the fix. And yeah, you're right, I forgot I removed one operator= so it should be +/- 0. :)

ajtowns commented at 8:50 PM on February 28, 2018: member

@kallewoof I like the idea of demonstrating the test fails before the fix, but still not convinced having the test actually fail (which could mess up attempts to use git bisect later, eg) is great. How about:

-BOOST_AUTO_TEST_CASE( operator_with_self )
+BOOST_AUTO_TEST_CASE( operator_with_self,
+               *boost::unit_test::expected_failures(1) )
 {
     arith_uint256 v = UintToArith256(uint256S("02"));
-    v *= v;
-    BOOST_CHECK(v == UintToArith256(uint256S("04")));
     v /= v;
     BOOST_CHECK(v == UintToArith256(uint256S("01")));
     v += v;
     BOOST_CHECK(v == UintToArith256(uint256S("02")));
+    v *= v;
+    BOOST_CHECK(v == UintToArith256(uint256S("04")));
     v -= v;
     BOOST_CHECK(v == UintToArith256(uint256S("0")));
 }

which results in:

$ ./test_bitcoin
Running 276 test cases...
test/uint256_tests.cpp(278): error: in "uint256_tests/operator_with_self": check v == UintToArith256(uint256S("04")) has failed

*** No errors detected

(rearrangement was necessary to avoid the bug causing v /= v to give a division by zero)

ie: commit 1: introduce failing test, but mark it as expected to fail; commit 2: fix bug, and remove expected-to-fail marker? I might be overthinking it, so please take this as a suggestion, not a blocker. ACK ac68e6d7176c1b24a69559afd1a66b2d04c67dc6

promag commented at 9:46 PM on February 28, 2018: member

I'm also -0 in having broken commits. How about https://docs.travis-ci.com/user/customizing-the-build/#Skipping-a-build?
Empact commented at 12:59 AM on March 1, 2018: member

Yeah I would just squash the test and fix commits together, it's clear enough how they're related.
[arith_uint256] Do not destroy *this content if passed-in operator may reference it 08b17def58
[test] Add tests for self usage in arith_uint256 b120f7bdbe
kallewoof force-pushed on Mar 1, 2018
kallewoof commented at 2:51 AM on March 1, 2018: member

The bisect thing makes sense yeah. I swapped the order so the fix comes before the tests.

Edit: @ajtowns Very cool about the expected failures thing. Learned something new. I think just swapping tests is OK though, but may make use of that in the future. :)
practicalswift commented at 7:09 AM on March 1, 2018: contributor

utACK b120f7bdbee83c00e46a9ec6d0cd09a816631f45
laanwj added the label Refactoring on Mar 1, 2018
laanwj commented at 2:30 PM on March 1, 2018: member

with drawback that *this is set to a at the end, an extra = operation in other words.

Could be extra copying be conditional on a being equal to self?

I don't think we rely on the performance of this function in any inner loop, but much of a performance hit are we going to take for functionality that isn't used in the code base?

An alternative would be to add an assert to forbid this usage.
kallewoof commented at 2:45 PM on March 1, 2018: member

@laanwj Actually, it turns out this is +/- 0, because I remove *this = 0 at the top, so it shouldn't cause a performance drop at all.
laanwj commented at 6:16 PM on March 1, 2018: member

Ok, seems better than preventing it then, utACK https://github.com/bitcoin/bitcoin/commit/b120f7bdbee83c00e46a9ec6d0cd09a816631f45.
fanquake requested review from sipa on Mar 14, 2018
ken2812221 commented at 9:52 AM on March 20, 2018: contributor

utACK b120f7b
laanwj merged this on Apr 9, 2018
laanwj closed this on Apr 9, 2018
laanwj referenced this in commit 4781813b56 on Apr 9, 2018
kallewoof deleted the branch on Apr 9, 2018
PastaPastaPasta referenced this in commit cc5dd301ab on Nov 1, 2020
gades referenced this in commit 3189a740df on Jun 26, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Review Requested
sipa

Labels

Refactoring