util: Make base_uint::GetHex() and base_uint::SetHex() not depend on uint256 #24077

1. 
   TheQuantumPhysicist commented at 4:06 pm on January 15, 2022: contributor

   The current implementations of SetHex() and GetHex() in base_uint use arith_uint256’s implementations. Which means, any attempt to create anything other than arith_uint256 (say arith_uint512) and using any of these functions (which is what I needed in my application) will just not work and will cause compilation errors (besides the immediate linking errors due to templates being in source files instantiated only for 256) because there’s no viable conversion from arith_uint256 and any of the other possible types. Besides that these function will yield wrong results even if the conversion is possible depending on the size. This is fixed in this PR.
2. 
   JeremyRubin commented at 7:50 pm on January 15, 2022: contributor

   is it possible you can add some tests or static asserts that would fail if these APIs were previously being (mis)used with differetn bits lengths?
3. 
   TheQuantumPhysicist commented at 9:13 am on January 16, 2022: contributor

   The problem with having more tests, say for base_uint<512>, is that whenever you call GetHex() or SetHex() on it, you’ll get undefined reference because the template base_uint<512> is not instantiated. I don’t really want to do that in the interest of keeping this PR simple because this can easily turn to a monstrosity, especially that I don’t like the way the instantiations for arith_uint256 are being done… I did instantiations of templates before by class, and not by method. I don’t know why it’s done like this (maybe some compilers don’t like it?). Let’s find out.

   In the interest of being helpful, I’ll add a commit that fixes this… let’s see if I’ll get yelled at by CI.

   If that works, you guys can tell me whether you’ll tolerate the presence of base_uint<512> instantiation just to test this (?).

   On the other hand, a static assert may be appropriate, but I can’t see a place to put it. If anyone can think of something, please recommend it.

4. 
   monstrito commented at 7:13 am on January 18, 2022: none

   @maldito-dot
5. 
   laanwj commented at 2:20 pm on April 5, 2022: member

   Code review ACK 4373362d8e88e3f9752da1c737bf2b0a94982d55 This is a straightforward change that makes the implementation slightly less clumsy.
6. laanwj added the label Validation on Apr 5, 2022
7. Replace uint256 specific implementations of base_uint::GetHex() and base_uint::SetHex() with proper ones that don't depend on uint256 and replace template methods instantiations of base_uint with template class instantiation a4f4f89815
8. in src/arith_uint256.cpp:150 in 4373362d8e outdated

   145@@ -146,13 +146,19 @@ double base_uint<BITS>::getdouble() const
   146 template <unsigned int BITS>
   147 std::string base_uint<BITS>::GetHex() const
   148 {
   149-    return ArithToUint256(*this).GetHex();
   150+    base_blob<BITS> b;
   151+    for(int x = 0; x < this->WIDTH; ++x)
   

   ---

   ---

   

   MarcoFalke commented at 2:56 pm on April 5, 2022:

   nit: Might be good for new code to format properly.

   Also the commit title contains a newline:

   0The subject line of commit hash c612bdf509c6534581d68a25d614545980bceb01 is followed by a non-empty line. Subject lines should always be followed by a blank line.
   

   0    for (int x = 0; x < this->WIDTH; ++x) {
   

   ---

   

   TheQuantumPhysicist commented at 3:12 pm on April 5, 2022:

   I fixed the formatting. About the commit message, thanks to Qt Creator… doing that without my consent. Should I rebase to fix this or do we let it go?

   ---

   

   TheQuantumPhysicist commented at 3:17 pm on April 5, 2022:

   OK. I just noticed the linter isn’t happy about this. Seems like I’ll have to rebase.

   ---

   

   fanquake commented at 3:19 pm on April 5, 2022:

   I fixed the formatting. OK. I just noticed the linter isn’t happy about this. Seems like I’ll have to rebase.

   While you are at it, the fix for the formatting needs to be squashed into the same commit that introduced the code.

   ---

   

   TheQuantumPhysicist commented at 3:28 pm on April 5, 2022:

   Alright. Rebased and squashed. Everything now is one commit.
9. TheQuantumPhysicist force-pushed on Apr 5, 2022
10. 
    laanwj commented at 9:30 am on April 6, 2022: member

    re-ACK a4f4f89815c5aadff51a7a11e0d63caf5212345a
11. in src/arith_uint256.cpp:149 in a4f4f89815

    145@@ -146,13 +146,21 @@ double base_uint<BITS>::getdouble() const
    146 template <unsigned int BITS>
    147 std::string base_uint<BITS>::GetHex() const
    148 {
    149-    return ArithToUint256(*this).GetHex();
    150+    base_blob<BITS> b;
    

    ---

    ---

    

    laanwj commented at 11:29 am on April 6, 2022:

    I think removing it or moving it is out of scope of this PR, however, just FYI this removes the last non-test use of ArithToUint256.

    ---

    

    TheQuantumPhysicist commented at 11:54 am on April 6, 2022:

    Interesting. Didn’t notice. I’m happy to reverse this and make ArithToUint256 use something from here. That would make much more sense.

    ---

    

    laanwj commented at 4:52 am on April 14, 2022:

    I think it’s fine to leave the implementation of ArithToUint256 as-is. I just meant it can be moved to the test utilities, it’s not used in non-test code anymore, but also that that is out of scope for this PR.
12. laanwj added the label Utils/log/libs on Apr 14, 2022
13. laanwj renamed this:
    Replace uint256 specific implementations of base_uint::GetHex() and base_uint::SetHex() with proper ones that don't depend on uint256
    util: Make base_uint::GetHex() and base_uint::SetHex() not depend on uint256
    on Apr 14, 2022
14. laanwj merged this on Apr 14, 2022
15. laanwj closed this on Apr 14, 2022

    ---

16. sidhujag referenced this in commit 3851f1b587 on Apr 14, 2022
17. TheQuantumPhysicist deleted the branch on Apr 16, 2022
18. DrahtBot locked this on Apr 16, 2023

Contributors
TheQuantumPhysicist JeremyRubin monstrito laanwj MarcoFalke fanquake

Labels
Validation Utils/log/libs