Page MenuHomePhabricator

Encode cashaddr size properly in version bit.
ClosedPublic

Authored by schancel on Dec 6 2017, 03:33.

Details

Summary

Encode the hash size in the version bit

Test Plan

make -j8 src/test/test_bitcoin && src/test/test_bitcoin --run_test='cashaddrenc_tests/*'

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cashaddr-size
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 1277
Build 1277: arc lint + arc unit

Event Timeline

If this patch is correct, should also include an additional test for larger sizes.

deadalnix requested changes to this revision.Dec 6 2017, 13:00

If you compute the size in there, then you don't need the expected size and you can't use asserts in there. You also want to add tests.

This revision now requires changes to proceed.Dec 6 2017, 13:00
deadalnix requested changes to this revision.Dec 7 2017, 23:31

You need to add a unit test to check that different sizes work/fail as expected.

src/cashaddrenc.cpp
39 ↗(On Diff #1985)

It seems that a switch would be the best option here.

46 ↗(On Diff #1985)

This misses one byte.

This revision now requires changes to proceed.Dec 7 2017, 23:31
This revision is now accepted and ready to land.Dec 8 2017, 00:33

Add encode_decode test back

deadalnix requested changes to this revision.Dec 8 2017, 01:40
deadalnix added inline comments.
src/cashaddrenc.cpp
17 ↗(On Diff #1988)

Remove

25 ↗(On Diff #1988)

size_t

144 ↗(On Diff #1988)

This should be a CashAddrType then

src/cashaddrenc.h
14 ↗(On Diff #1988)

CashAddrType

src/test/cashaddrenc_tests.cpp
15 ↗(On Diff #1988)

Declare it in the header file, not here.

38 ↗(On Diff #1988)

Why generating a rand32 to get only 8 bits ? Also, just use the uint8_t constructor.

77 ↗(On Diff #1988)

Use explicit types. In C++, a lot of types can be expensive to copy, so it's useful to know.

85 ↗(On Diff #1988)

This do not check the size bits, which is the very thing this test is supposed to test. it also doesn't check that invalid size fail.

181 ↗(On Diff #1988)

Why do you stick UL all over the place ?

This revision now requires changes to proceed.Dec 8 2017, 01:40
src/test/cashaddrenc_tests.cpp
85 ↗(On Diff #1988)

It checks that the decoded size is the encoded size. That won't be true unless the size was encoded properly.

Fix nits

src/test/cashaddrenc_tests.cpp
181 ↗(On Diff #1988)

Because they generate annoying warnings on the MacOS toolchain,

Invalid sizes failing are checked in the test check_size

src/cashaddrenc.cpp
130 ↗(On Diff #1990)

You wanted CASHADDR_BYTES removed, but it's used here. This seems bad. Is it what you wanted?

src/test/cashaddrenc_tests.cpp
85 ↗(On Diff #1988)

No, it may be true for all kind of other reasons. For instance because you encode/decode whatever data there is without checking the size at all.

181 ↗(On Diff #1988)

I can't help you if you don't give me the reason.

deadalnix requested changes to this revision.Dec 8 2017, 02:07
deadalnix added inline comments.
src/cashaddrenc.cpp
130 ↗(On Diff #1990)

If you want to support different sizes, you need to reserve the right amount here. It's actually fairy easy from the length of the data to decode.

This revision now requires changes to proceed.Dec 8 2017, 02:07
schancel added inline comments.
src/test/cashaddrenc_tests.cpp
181 ↗(On Diff #1988)
In file included from ../../src/test/cashaddrenc_tests.cpp:12:
In file included from /usr/local/include/boost/test/unit_test.hpp:18:
In file included from /usr/local/include/boost/test/test_tools.hpp:46:
/usr/local/include/boost/test/tools/old/impl.hpp:107:17: warning: comparison of integers of different signs: 'const unsigned long' and 'const int' [-Wsign-compare]
    return left == right;
           ~~~~ ^  ~~~~~
/usr/local/include/boost/test/tools/old/impl.hpp:130:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl<unsigned long, int>' requested here
        return equal_impl( left, right );
               ^
/usr/local/include/boost/test/tools/old/impl.hpp:145:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::call_impl<unsigned long, int>' requested here
        return call_impl( left, right, left_is_array() );
               ^
/usr/local/include/boost/test/tools/old/impl.hpp:92:50: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::operator()<unsigned long, int>' requested here
BOOST_PP_REPEAT( BOOST_TEST_MAX_PREDICATE_ARITY, IMPL_FRWD, _ )
                                                 ^
/usr/local/include/boost/preprocessor/repetition/repeat.hpp:29:26: note: expanded from macro 'BOOST_PP_REPEAT'
# define BOOST_PP_REPEAT BOOST_PP_CAT(BOOST_PP_REPEAT_, BOOST_PP_AUTO_REC(BOOST_PP_REPEAT_P, 4))
                         ^
/usr/local/include/boost/preprocessor/cat.hpp:22:32: note: expanded from macro 'BOOST_PP_CAT'
#    define BOOST_PP_CAT(a, b) BOOST_PP_CAT_I(a, b)
                               ^
/usr/local/include/boost/preprocessor/cat.hpp:29:34: note: expanded from macro 'BOOST_PP_CAT_I'
#    define BOOST_PP_CAT_I(a, b) a ## b
                                 ^
<scratch space>:219:1: note: expanded from here
BOOST_PP_REPEAT_1
^
../../src/test/cashaddrenc_tests.cpp:200:9: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::check_frwd<boost::test_tools::tt_detail::equal_impl_frwd, unsigned long, int>' requested here
        BOOST_CHECK_EQUAL(content.hash.size(), 0);
        ^
/usr/local/include/boost/test/tools/old/interface.hpp:152:45: note: expanded from macro 'BOOST_CHECK_EQUAL'
#define BOOST_CHECK_EQUAL( L, R )           BOOST_TEST_TOOL_IMPL( 0, \
                                            ^
/usr/local/include/boost/test/tools/old/interface.hpp:65:47: note: expanded from macro 'BOOST_TEST_TOOL_IMPL'
    BOOST_PP_IF( frwd_type, report_assertion, check_frwd ) (                    \
                                              ^
1 warning generated.

Handle valid sizes better

Looks good. You need a test to ensure PackCashAddrContent throws when it should.

src/cashaddrenc.cpp
20 ↗(On Diff #1993)

nit: can be shortened to uint8_t version_byte(type << 3);

49 ↗(On Diff #1993)

nit: would std::invalid_argument be more fitting?

129 ↗(On Diff #1993)

"Reserve maximum" implies there is a different minimum. Is there?

src/test/cashaddrenc_tests.cpp
66 ↗(On Diff #1993)

const

Also maybe this should be a std::array .

src/cashaddrenc.cpp
129 ↗(On Diff #1993)

No, the comment refers to an outdated version of the code.

141 ↗(On Diff #1993)
CashAddrType type((version >> 3) & 0x1f);

Fix some nits, and add a test.

src/cashaddrenc.cpp
141 ↗(On Diff #1993)

Would need to be:

CashAddrType type(CashAddrType((version >> 3) & 0x1f));

I think the current form is more legible.

src/cashaddrenc.cpp
143 ↗(On Diff #1999)
CashAddrType type((version >> 3) & 0x1f);
src/test/cashaddrenc_tests.cpp
84 ↗(On Diff #1999)
BOOST_CHECK_EQUAL(packed_data[1] >> 3, ps.first)
src/cashaddrenc.cpp
49 ↗(On Diff #1999)

I don't think this exception is appropriate given this:

http://www.cplusplus.com/reference/stdexcept/logic_error/

Which invalid_argument is a subclass of.

src/test/cashaddrenc_tests.cpp
84 ↗(On Diff #1999)

This is not correct.

../../src/test/cashaddrenc_tests.cpp:84: error: in "cashaddrenc_tests/encode_decode_all_sizes": check packed_data[1] >> 3 == ps.first has failed [0 != 0x1]
../../src/test/cashaddrenc_tests.cpp:84: error: in "cashaddrenc_tests/encode_decode_all_sizes": check packed_data[1] >> 3 == ps.first has failed [1 != 0x2]
../../src/test/cashaddrenc_tests.cpp:84: error: in "cashaddrenc_tests/encode_decode_all_sizes": check packed_data[1] >> 3 == ps.first has failed [1 != 0x3]
../../src/test/cashaddrenc_tests.cpp:84: error: in "cashaddrenc_tests/encode_decode_all_sizes": check packed_data[1] >> 3 == ps.first has failed [2 != 0x4]
../../src/test/cashaddrenc_tests.cpp:84: error: in "cashaddrenc_tests/encode_decode_all_sizes": check packed_data[1] >> 3 == ps.first has failed [2 != 0x5]
../../src/test/cashaddrenc_tests.cpp:84: error: in "cashaddrenc_tests/encode_decode_all_sizes": check packed_data[1] >> 3 == ps.first has failed [3 != 0x6]
../../src/test/cashaddrenc_tests.cpp:84: error: in "cashaddrenc_tests/encode_decode_all_sizes": check packed_data[1] >> 3 == ps.first has failed [3 != 0x7]

Fix nits, change exception back to runtime_error

deadalnix requested changes to this revision.Dec 8 2017, 20:20
deadalnix added inline comments.
src/test/cashaddrenc_tests.cpp
84 ↗(On Diff #1999)

You got to shit by 2, alright, please fix anyway.

This revision now requires changes to proceed.Dec 8 2017, 20:20
schancel marked 2 inline comments as done.
schancel marked 4 inline comments as done.
schancel marked 3 inline comments as done.
This revision is now accepted and ready to land.Dec 8 2017, 20:34
This revision was automatically updated to reflect the committed changes.
schancel marked an inline comment as done and 2 inline comments as not done.