uint256: 1 is a constant

ajtowns commented at 3:11 pm on September 25, 2020: member

UINT256_ONE() returns a reference to a global; mark it as const to be sure someone doesn’t accidently modify it.

ajtowns commented at 3:16 pm on September 25, 2020: member

This was added in #17261 in which it was remarked “static initialization order result[ed] in a potential uninitialized object access.” I have an alternative change at https://github.com/ajtowns/bitcoin/commits/202009-uint256_1_v2 which does a static init and hence avoids the functional call, and seems to pass travis fine, so might be worth considering that? cc @achow101 @kallewoof

achow101 commented at 3:45 pm on September 25, 2020: member

I have an alternative change at https://github.com/ajtowns/bitcoin/commits/202009-uint256_1_v2 which does a static init and hence avoids the functional call, and seems to pass travis fine, so might be worth considering that?

Concept ACK

kallewoof commented at 7:07 am on September 26, 2020: member

Clear improvement.

ACK 832109af0d213239b1b803a1de7f78f193d28ad2

ajtowns force-pushed on Sep 26, 2020

ajtowns commented at 8:41 am on September 26, 2020: member

Updated to simplfy constructors and replace UINT256_ONE() call with uint256::ONE constant

DrahtBot added the label Consensus on Sep 26, 2020

DrahtBot added the label Wallet on Sep 26, 2020

MarcoFalke removed the label Wallet on Sep 26, 2020

practicalswift commented at 10:06 am on September 26, 2020: contributor

Concept ACK

kristapsk approved

kristapsk commented at 11:41 am on September 26, 2020: contributor

ACK 915c1c25fd6d6bdc249b71be12db0744312729da

sipa commented at 6:44 pm on September 26, 2020: member

utACK 915c1c25fd6d6bdc249b71be12db0744312729da

1 is a constant

It is in tautologies that the ordering of society is centered.

in src/uint256.h:27 in fe9fbb6886 outdated

26-    }
27+    /* construct 0 value by default */
28+    base_blob() : m_data() {}
29+
30+    /* constructor for constants between 1 and 255 */
31+    explicit constexpr base_blob(uint8_t v) : m_data{v} {}

MarcoFalke commented at 8:22 pm on September 26, 2020:

0    constexpr explicit base_blob(uint8_t v) : m_data{v} {}

style nit: Would be nice to use the same ordering of keywords for different lines in the same patch.

ajtowns commented at 9:56 am on September 27, 2020:

Makes sense; https://github.com/cplusplus/draft/wiki/Specification-Style-Guidelines#formatting-declarations-and-definitions also suggests constexpr explicit

MarcoFalke approved

MarcoFalke commented at 8:25 pm on September 26, 2020: member

Not sure how this could have led to memory access violations. Though, if the wallet or the script interpreter is running after the end of the main function, something else has gone wrong and should probably be fixed.

cr ACK 915c1c25fd6d6bdc249b71be12db0744312729da 📺

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3cr ACK 915c1c25fd6d6bdc249b71be12db0744312729da 📺
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhQlQwAqTJdOujLC6CD+i1XI8ISvOTG76CLhzgK5wg+63FYUVILodO1T0ig7NJH
 8D+RHnv0fdyO3OIFGH3C4ooSkHNqIiKec7Q0V2uemtU5TZX04jM6ocsP/frsCfSmU
 9p13UkuNxx5QSnrSlFLxrGrmuX/BkLxdubGLTXNQHNltx5rHz2p0H03ueep2/XrXM
10BeMAdrBcBvjsQ3SW5XuUND8hza6YajWpAMOUSnS7rbT/hpxi3SakjYyA8E/fMQUo
11tr7F/XVLX9KTYzRFvbQLRdUs2Uy3jIP4AhuJJqa7rt/1wv3Au7N0nL96wPJlBiKO
12radqu65JIp2sNDl4XtXcq1gol+O184rASiaeDZlI1kAxUKoBvDKd9XtBnDWHywyl
13vvulJWWkZLHN6q0z3JJ2GON8Uyr+q6ErUrOuOyLsuRvnrmawqkc6tBaUpkCefPuQ
146kKwWybrM5xSq0/OYiOQ0vFRJY34GLy7qNmyUOIQrGpbfn9AL7c9B5p9FueqMukP
15/HlO1n43
16=ZuQH
17-----END PGP SIGNATURE-----

Timestamp of file with hash ed0494b57cec88e0ff8da75d55c543250f502605792f0e336b3c5d14e6a470b4 -

MarcoFalke commented at 8:26 pm on September 26, 2020: member

(1 is not a constant in python, but we don’t use that here https://kate.io/blog/2017/08/22/weird-python-integers/)

promag commented at 9:56 pm on September 26, 2020: member

ACK 915c1c25fd6d6bdc249b71be12db0744312729da.

in src/test/uint256_tests.cpp:281 in fe9fbb6886 outdated

277@@ -278,4 +278,10 @@ BOOST_AUTO_TEST_CASE( operator_with_self )
278     BOOST_CHECK(v == UintToArith256(uint256S("0")));
279 }
280 
281+BOOST_AUTO_TEST_CASE( check_ONE )

hebasto commented at 7:31 am on September 27, 2020:

fe9fbb6886a96de38dc3fdd65f28dfc01f707f81

style nit:

0BOOST_AUTO_TEST_CASE(check_ONE)

ajtowns commented at 6:00 am on September 28, 2020:

Added the constexpr annotations. Left the formatting consistent with the rest of uint256_tests.cpp (and arith_uint256_tests.cpp).

hebasto approved

hebasto commented at 8:00 am on September 27, 2020: member

ACK 915c1c25fd6d6bdc249b71be12db0744312729da, I have reviewed the code and it looks OK, I agree it can be merged.

May I suggest a further step:

 0--- a/src/uint256.h
 1+++ b/src/uint256.h
 2@@ -21,7 +21,7 @@ protected:
 3     uint8_t m_data[WIDTH];
 4 public:
 5     /* construct 0 value by default */
 6-    base_blob() : m_data() {}
 7+    constexpr base_blob() : m_data() {}
 8 
 9     /* constructor for constants between 1 and 255 */
10     explicit constexpr base_blob(uint8_t v) : m_data{v} {}
11@@ -112,7 +112,7 @@ public:
12  */
13 class uint160 : public base_blob<160> {
14 public:
15-    uint160() {}
16+    constexpr uint160() {}
17     explicit uint160(const std::vector<unsigned char>& vch) : base_blob<160>(vch) {}
18 };
19 
20@@ -123,7 +123,7 @@ public:
21  */
22 class uint256 : public base_blob<256> {
23 public:
24-    uint256() {}
25+    constexpr uint256() {}
26     constexpr explicit uint256(uint8_t v) : base_blob<256>(v) {}
27     explicit uint256(const std::vector<unsigned char>& vch) : base_blob<256>(vch) {}
28     static const uint256 ONE;

?

uint256: Update constructors to c++11, make ONE static

Replace the memset with C++11 value/aggregate initialisation of
the m_data array, which still ensures the unspecified values end
up as zero-initialised.

This then allows changing UINT256_ONE() from dynamically allocating an
object, to a simpler referencing a static allocation.

183f308fff

scripted-diff: Replace UINT256_ONE() with uint256::ONE

-BEGIN VERIFY SCRIPT-
sed -i '/inline.* UINT256_ONE() {/,+1d' src/uint256.h
sed -i 's/UINT256_ONE()/uint256::ONE/' $(git grep -l UINT256_ONE)
-END VERIFY SCRIPT-

82cf4641f4

wallet: no need for duplicate storage for ABANDON_HASH constant 4cc7171c98

ajtowns force-pushed on Sep 28, 2020

kallewoof commented at 7:10 am on September 28, 2020: member

ACK 4cc7171c9887e88027642927c979e507d7b78dda

promag commented at 7:13 am on September 28, 2020: member

ACK 4cc7171c9887e88027642927c979e507d7b78dda

MarcoFalke commented at 7:15 am on September 28, 2020: member

re ACK 4cc7171c98, only change is some constexpr shuffling 🛁

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re ACK 4cc7171c98, only change is some constexpr shuffling 🛁
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhxqwwAxiSleGr6i0+dxWiXOWdI69VYKTpCJZx1eqemwQOuNA+YguYEgdIvm+78
 82uFRqv3WgLDF7RvJuGVoX8x3BYMaNML0SsADXDNi0A/1FCHMdH1Mh9R+kJLFrYyK
 9+EsRdPGYnt4RNstY/lKkMiN6zKwEA889hcUFJbGHdnjO9+LsnfO2oCV2HSB5n6c+
10uj//8F3NwvyIFwVVYdn0XnDML777HOY6qwsIQFtjNV8MCzPyEdO9qGAyrORWKLIs
11rDNUFByTi+i8Eg0iOUeDHarOq9JRRXk9iw09Mo207PeplnjoIvFUbdDLBkpiaIf2
126xh2tIsnZRBcVWyLmwtNPA+4Hg59a6mujwV0HEgoq1gZ55fS6qxB9gHyVElWmqZX
13beGvhIM6Oa4j6449UFbGhAMn5hl7m3xXZyGr+HuV8u1RYEpMEpkiNV3CMrMSPQbA
145LY4cIsOLWmuuI/RQJmecftKiKu+YkoknRNjqF7TIqdX5qVMP6V4buB6MeFvB6R/
15vgDgxnQp
16=JHzT
17-----END PGP SIGNATURE-----

Timestamp of file with hash 7017836e4e8e4ce0003284c993c86597e80a695b740243eca5ab7c60803d2d5e -

MarcoFalke merged this on Sep 28, 2020

MarcoFalke closed this on Sep 28, 2020

hebasto commented at 8:38 am on September 28, 2020: member

re-ACK 4cc7171c9887e88027642927c979e507d7b78dda, only suggested changes since my previous review.

deadalnix referenced this in commit 1377a4f39f on May 5, 2021

DrahtBot locked this on Feb 15, 2022

uint256: 1 is a constant #20016