test: refactor: remove unneeded bytes<->hex conversions in `byte_to_base58` #24324

1. 
   theStack commented at 1:56 AM on February 12, 2022: member

   It seems like the only reason for using hex strings in this method was to have a convenient way to convert to an integer from the input data interpreted as big-endian. In Python3 we have int.from_bytes(..., 'big') for that purpose, hence there is no need for that anymore and we can simply operate on bytes only.

2. theStack force-pushed on Feb 12, 2022
3. theStack renamed this:
   test: refactor: remove uneeded bytes<->hex conversions in `byte_to_base58`
   test: refactor: remove unneeded bytes<->hex conversions in `byte_to_base58`
   on Feb 12, 2022
4. DrahtBot added the label Tests on Feb 12, 2022
5. shaavan approved
6. 
   shaavan commented at 11:55 AM on February 12, 2022: contributor

   Code Review ACK

   This PR rewrites the byte_to_base58 function, which removes the need for bytes <-> hex conversion. I have manually tested the old and new code, and their results are identical.

   Screenshot:

   Master	PR

   The updated code is clean and concise, and I agree that it can be merged.

7. in test/functional/test_framework/address.py:64 in 69675793e8 outdated

     65 | +    value = int.from_bytes(b, 'big')
     66 |      while value > 0:
     67 |          result = chars[value % 58] + result
     68 |          value //= 58
     69 | -    while (str[:2] == '00'):
     70 | +    while (b[0] == 0):
   

   ---

   ---

   

   laanwj commented at 9:11 AM on February 14, 2022:

   Might as well remove the unnecessary parentheses here while we're at it.

   ---

   

   theStack commented at 11:49 AM on February 14, 2022:

   Thanks, good catch, done.

8. in test/functional/test_framework/address.py:58 in 69675793e8 outdated

     54 | @@ -55,17 +55,15 @@ def create_deterministic_address_bcrt1_p2tr_op_true():
     55 |  
     56 |  def byte_to_base58(b, version):
     57 |      result = ''
     58 | -    str = b.hex()
   

   ---

   ---

   

   laanwj commented at 9:12 AM on February 14, 2022:

   Oof, this was some terrible python code, thanks for improving it :smile:

9. test: refactor: remove unneeded bytes<->hex conversions in `byte_to_base58`

   It seems like the only reason for using hex strings in this
   method was to have a convenient way to convert to an integer
   from the input data interpreted as big-endian.
   In Python3 we have `int.from_bytes(..., 'big')` for that
   purpose, hence there is no need for that anymore and we can
   simply operate on bytes only.

   f11dad22a5
10. theStack force-pushed on Feb 14, 2022
11. 
    laanwj commented at 1:16 PM on February 14, 2022: member

    Code review ACK f11dad22a506e10fbbfbcb6ccf32754bf8e72b72

12. fanquake requested review from MarcoFalke on Feb 14, 2022
13. MarcoFalke merged this on Feb 15, 2022
14. MarcoFalke closed this on Feb 15, 2022

    ---

15. theStack deleted the branch on Feb 15, 2022
16. sidhujag referenced this in commit d189fbf2f0 on Feb 15, 2022
17. Fabcien referenced this in commit c168e61957 on Mar 7, 2022
18. DrahtBot locked this on Feb 15, 2023

Contributors

theStackshaavanlaanwj

Review Requested
MarcoFalke