tests: implement base58

10xcryptodev commented at 2:15 AM on May 13, 2020: contributor

implements TODO: def base58_decode

fanquake added the label Tests on May 13, 2020

10xcryptodev force-pushed on May 13, 2020

in test/functional/test_framework/address.py:45 in 092016ff0e outdated

  41 | @@ -41,7 +42,26 @@ def byte_to_base58(b, version):
  42 |          str = str[2:]
  43 |      return result
  44 |  
  45 | -# TODO: def base58_decode
  46 | +def base58_decode(s):

kallewoof commented at 5:01 AM on May 13, 2020:

Perhaps follow convention of the encoder and call it base58_to_byte?

MarcoFalke commented at 1:06 PM on May 13, 2020:

This is currently unused in the test code, so I don't see the value of adding it unless this is going to be used any time soon

I'd say to just remove the TODO

ryanofsky commented at 2:02 PM on May 13, 2020:

It seems like it could easily be used to test the byte_to_base58 function, helping find & debug problems related to it

kallewoof approved

kallewoof commented at 5:05 AM on May 13, 2020: member

Tested ACK

Verified that

assert_equal(base58_decode('USm3fpXnKG5EUBx2ndxBDMPVciP5hGey2Jh4NDv6gmeo1LkMeiKrLJUUBk6Z'), b'The quick brown fox jumps over the lazy dog.')

(from https://tools.ietf.org/id/draft-msporny-base58-01.html#rfc.section.4)

Btw, there doesn't seem to be a good test file for adding (sanity) tests of this kind.

DrahtBot commented at 7:46 AM on May 13, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17977 ([WIP] Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj commented at 1:08 PM on May 13, 2020: member

It's kind of unfortunate that someone spends work doing this just to be told it's not necessary.

We should really go through the code some time and remove TODOs that aren't really TODOs.

instagibbs commented at 2:03 PM on May 13, 2020: member

concept ACK, it's completely fine to have something that can build usable test off of. It's not as base58 decoding is some strange thing in the repo.

I do however think it needs at least a single set of tests at least showing round-trip works.

Btw, there doesn't seem to be a good test file for adding (sanity) tests of this kind.

Indeed, previously I just shoe-horned it for CScriptNum https://github.com/bitcoin/bitcoin/pull/14816

MarcoFalke commented at 2:16 PM on May 13, 2020: member

concept ACK, it's completely fine to have something that can build usable test off of

<strike>agree, but as I said I don't see how anyone is going to build a useful test off of this any time soon. The TODO has been added 4 years ago and nothing happened. It seems recursive to write functionality in the test framework and then add a round-trip test to test the test. The purpose of tests is to find issues in Bitcoin Core, not in tests that exists only for the reason to find issues in themselves.</strike>

I see the byte_to_base58 is used in tests to test Bitcoin Core. So yes, it could probably make sense to add a round-trip unit test for this. See also #18576

It's kind of unfortunate that someone spends work doing this just to be told it's not necessary.

I also agree here, but I don't see a way to avoid this generally.

Of course, we should look through TODOs in the code and remove them if they are no longer applicable, and we should also avoid adding TODOs in the code that don't make sense in the first place and are not accompanied by a motivation. But this is a best effort basis, and there will always be TODOs that slip through.

When someone asks me for good first issues to work on, I tell them to check back with me before they start working on it, because it might be stale or already otherwise fixed.

MarcoFalke commented at 2:22 PM on May 13, 2020: member

Concept ACK (edited my reply)

10xcryptodev commented at 3:35 PM on May 13, 2020: contributor

@MarcoFalke thanks for reviewing, i will add the round-trip unit tests

Also i will interact with the team before implementing if is not so clear

laanwj commented at 7:29 AM on May 15, 2020: member

Concept ACK anyway.

I also agree here, but I don't see a way to avoid this generally.

I think we that's pretty straightforward: reject adding TODOs in the source code. If there are open TODO items when contributing a PR, it's better to open a github issue for them. At least it allows for discussion, or reconsidering/closing later. Instead of being a single developer's opinion stuck in time.

10xcryptodev force-pushed on May 15, 2020

10xcryptodev commented at 11:13 PM on May 15, 2020: contributor

thanks for the comments and reviews, addressed them

in test/functional/test_framework/address.py:60 in 9942e0211c outdated

  56 | +        digit = chars.index(c)
  57 | +        n += digit
  58 | +    h = '%x' % n
  59 | +    if len(h) % 2:
  60 | +        h = '0' + h
  61 | +    res = binascii.unhexlify(h.encode('utf8'))

ryanofsky commented at 7:43 PM on May 19, 2020:

In commit "tests: implement base58_decode" (9942e0211c7dca24c85a6efcd1de2bd71b95e34d)

I think you can replace this int -> hex -> binary conversion with a direct conversion like

res = n.to_bytes((n.bit_length() + 7) // 8, 'big')

https://stackoverflow.com/questions/21017698/converting-int-to-bytes-in-python-3/30375198#30375198

in test/functional/test_framework/address.py:62 in 9942e0211c outdated

  58 | +    h = '%x' % n
  59 | +    if len(h) % 2:
  60 | +        h = '0' + h
  61 | +    res = binascii.unhexlify(h.encode('utf8'))
  62 | +    pad = 0
  63 | +    for c in s[:-1]:

ryanofsky commented at 7:55 PM on May 19, 2020:

In commit "tests: implement base58_decode" (9942e0211c7dca24c85a6efcd1de2bd71b95e34d)

What is -1 here?

in test/functional/test_framework/address.py:74 in 9942e0211c outdated

  70 |  
  71 |  def keyhash_to_p2pkh(hash, main = False):
  72 |      assert len(hash) == 20
  73 |      version = 0 if main else 111
  74 | +    # round-trip check for base58
  75 | +    assert_equal(base58_to_byte(byte_to_base58(hash, version)), hash)

ryanofsky commented at 8:06 PM on May 19, 2020:

In commit "tests: implement base58_decode" (9942e0211c7dca24c85a6efcd1de2bd71b95e34d)

This is an arbitrary seeming place to put a round trip check. I think it'd be better to add a simple external test, or do something more general like add default check_roundtrip=True parameters to the two functions and checks at the bottom of each:

if check_roundtrip: assert_equals(base58_to_byte(result, check_roundtrip=False) == b

MarcoFalke commented at 9:37 PM on May 19, 2020:

We have a python unit test framework now: #18576

in test/functional/test_framework/address.py:67 in 9942e0211c outdated

  63 | +    for c in s[:-1]:
  64 | +        if c == chars[0]:
  65 | +            pad += 1
  66 | +        else:
  67 | +            break
  68 | +    return b'\x00' * pad + res[1:21]

ryanofsky commented at 8:07 PM on May 19, 2020:

In commit "tests: implement base58_decode" (9942e0211c7dca24c85a6efcd1de2bd71b95e34d)

I think 21 should be replaced with -4 here to strip off the checksum bytes without assuming a fixed length string. Also it would be good to verify the checksum, maybe optionally with a verify_checksum=True default parameter.

ryanofsky commented at 8:18 PM on May 19, 2020: member

Code review almost-ACK 9942e0211c7dca24c85a6efcd1de2bd71b95e34d. This has some rough edges still. I left some comments below that I think would be better to resolve before merging this (in my opinion)

10xcryptodev force-pushed on May 20, 2020

10xcryptodev commented at 1:03 AM on May 20, 2020: contributor

thanks @ryanofsky and @MarcoFalke for the infos and directions, added your suggestions

MarcoFalke commented at 12:51 PM on May 20, 2020: member

Approach ACK 31cfd81e1f374a722ab0190cd89814f22b8f91f8 (haven't reviewed the code)

in test/functional/test_framework/address.py:67 in 31cfd81e1f outdated

  63 | +    for c in s:
  64 | +        if c == chars[0]:
  65 | +            pad += 1
  66 | +        else:
  67 | +            break
  68 | +    if (verify_checksum):

ryanofsky commented at 1:05 PM on May 20, 2020:

In commit "tests: implement base58_decode" (31cfd81e1f374a722ab0190cd89814f22b8f91f8)

Normally these parentheses are not written in python

in test/functional/test_framework/address.py:48 in 31cfd81e1f outdated

  43 | @@ -41,7 +44,32 @@ def byte_to_base58(b, version):
  44 |          str = str[2:]
  45 |      return result
  46 |  
  47 | -# TODO: def base58_decode
  48 | +
  49 | +def base58_to_byte(s, version, verify_checksum=True):

ryanofsky commented at 1:13 PM on May 20, 2020:

In commit "tests: implement base58_decode" (31cfd81e1f374a722ab0190cd89814f22b8f91f8)

I think version argument is confusing and should be dropped. There's no need to require it since version number is in res[0]

in test/functional/test_framework/address.py:69 in 31cfd81e1f outdated

  65 | +            pad += 1
  66 | +        else:
  67 | +            break
  68 | +    if (verify_checksum):
  69 | +        hash = byte_to_base58(res[1:-4], version)
  70 | +        assert_equal(s, hash)

ryanofsky commented at 1:17 PM on May 20, 2020:

In commit "tests: implement base58_decode" (31cfd81e1f374a722ab0190cd89814f22b8f91f8)

I think it's less complicated to just assert_equal(hash256(res[:-4], res[-4:]) instead of calling byte_to_base58 to re-encode what was just decoded

in test/functional/test_framework/address.py:71 in 31cfd81e1f outdated

  67 | +            break
  68 | +    if (verify_checksum):
  69 | +        hash = byte_to_base58(res[1:-4], version)
  70 | +        assert_equal(s, hash)
  71 | +
  72 | +    return b'\x00' * pad + res[1:-4]

ryanofsky commented at 1:27 PM on May 20, 2020:

In commit "tests: implement base58_decode" (31cfd81e1f374a722ab0190cd89814f22b8f91f8)

Might be nice return the version number as well as the data. Maybe:

res = b'\x00' * pad + res
if verify_checksum:
  assert_equal(hash256(res[:-4], res[-4:])
return res[1:-4], int(res[0])

I think this also fixes a padding bug. It looks like in byte_to_base58 the padding precedes the version number and data, but the current return statement effectively adds the padding between the version number and the data.

in test/functional/test_framework/address.py:137 in 31cfd81e1f outdated

 132 | +
 133 | +class TestFrameworkScript(unittest.TestCase):
 134 | +    def test_base58encodedecode(self):
 135 | +        self.assertEqual(base58_to_byte(byte_to_base58(b'\x1f\x8e\xa1p*{\xd4\x94\x1b\xca\tA\xb8R\xc4\xbb\xfe\xdb.\x05', 111), 111), b'\x1f\x8e\xa1p*{\xd4\x94\x1b\xca\tA\xb8R\xc4\xbb\xfe\xdb.\x05')
 136 | +        self.assertEqual(base58_to_byte(byte_to_base58(b':\x0b\x05\xf4\xd7\xf6l;\xa7\x00\x9fE50)l\x84\\\xc9\xcf', 111), 111), b':\x0b\x05\xf4\xd7\xf6l;\xa7\x00\x9fE50)l\x84\\\xc9\xcf')
 137 | +        self.assertEqual(base58_to_byte(byte_to_base58(b'A\xc1\xea\xf1\x11\x80%Y\xba\xd6\x1b`\xd6+\x1f\x89|c\x92\x8a', 111), 111), b'A\xc1\xea\xf1\x11\x80%Y\xba\xd6\x1b`\xd6+\x1f\x89|c\x92\x8a')

ryanofsky commented at 1:38 PM on May 20, 2020:

In commit "tests: implement base58_decode" (31cfd81e1f374a722ab0190cd89814f22b8f91f8)

Could avoid needing to repeat the byte strings (they are hard to compare visually) with a helper lambda:

check = lambda data, ver: self.assertEqual(base58_to_byte(byte_to_base58(data, ver)), (data, ver))
check(b'\x1f\x8e\xa1p*{\xd4\x94\x1b\xca\tA\xb8R\xc4\xbb\xfe\xdb.\x05', 111)
check(b':\x0b\x05\xf4\xd7\xf6l;\xa7\x00\x9fE50)l\x84\\\xc9\xcf', 111)
check(b'A\xc1\xea\xf1\x11\x80%Y\xba\xd6\x1b`\xd6+\x1f\x89|c\x92\x8a', 111)

Also it might be good to add a check with 0 version number and some leading \0 bytes to make sure roundtrips with padding are working

ryanofsky commented at 1:43 PM on May 20, 2020: member

Code review almost-ACK 31cfd81e1f374a722ab0190cd89814f22b8f91f8. This seems pretty close, but I think there is a padding bug, and I also added some more suggestions for cleanup, see comments below.

tests: implement base58_decode 60ed33904c

10xcryptodev force-pushed on May 20, 2020

10xcryptodev commented at 10:00 PM on May 20, 2020: contributor

thanks @ryanofsky about lambda flake8 warned about lambda assignment E731, changed to def also assert_equal(hash256(res[:-4], res[-4:]) this line was not right but i got the idea

ryanofsky approved

ryanofsky commented at 10:15 PM on May 26, 2020: member

Code review ACK 60ed33904cf974e8f3c1b95392a23db1fe2d4a98. Just suggested changes since last review. Thank you for taking suggestions!

MarcoFalke merged this on May 30, 2020

MarcoFalke closed this on May 30, 2020

sidhujag referenced this in commit 6404ef42c2 on May 31, 2020

in test/functional/test_framework/address.py:48 in 60ed33904c

  43 | @@ -41,7 +44,32 @@ def byte_to_base58(b, version):
  44 |          str = str[2:]
  45 |      return result
  46 |  
  47 | -# TODO: def base58_decode
  48 | +
  49 | +def base58_to_byte(s, verify_checksum=True):

jnewbery commented at 3:37 PM on June 10, 2020:

This shouldn't add an optional verify_checksum parameter which is never used (this function is only called once without specifying this parameter).

This function should also have a docstring to say what it's doing (and more importantly to document that it asserts if the checksum is bad)

in test/functional/test_framework/address.py:15 in 60ed33904c

  10 |  from .script import hash256, hash160, sha256, CScript, OP_0
  11 |  from .util import hex_str_to_bytes
  12 |  
  13 |  from . import segwit_addr
  14 |  
  15 | +from test_framework.util import assert_equal

jnewbery commented at 3:38 PM on June 10, 2020:

This module already imports from util (3 lines up)

jnewbery commented at 3:39 PM on June 10, 2020: member

Concept ACK. A couple of things that could be fixed in a follow-up.

ftrader referenced this in commit 430a3113b3 on Dec 1, 2020

Fabcien referenced this in commit 06eae6f27d on Feb 24, 2021

DrahtBot locked this on Feb 15, 2022

tests: implement base58_decode #18965

Conflicts