Page MenuHomePhabricator
Differential D761

Encode cashaddr size properly in version bit.
ClosedPublic
Actions

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

Details

Reviewers
deadalnix
freetrader
dagurval
Group Reviewers
Restricted Project
Commits
rSTAGING43d3835e02aa: Encode cashaddr size properly in version bit.
rABC43d3835e02aa: Encode cashaddr size properly in version bit.
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

schancel created this revision.Dec 6 2017, 03:33
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 6 2017, 03:33
schancel added a comment.Dec 6 2017, 04:28

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
schancel updated this revision to Diff 1985.Dec 7 2017, 22:37
  • Fix calculations
Harbormaster completed remote builds in B1275: Diff 1985.Dec 7 2017, 22:37
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
schancel updated this revision to Diff 1987.Dec 7 2017, 23:54
deadalnix accepted this revision.Dec 8 2017, 00:33
This revision is now accepted and ready to land.Dec 8 2017, 00:33
schancel updated this revision to Diff 1988.Dec 8 2017, 01:30

Add a test

schancel updated this revision to Diff 1989.Dec 8 2017, 01: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
schancel added inline comments.Dec 8 2017, 01:52
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.

schancel updated this revision to Diff 1990.Dec 8 2017, 01:53

Fix nits

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

Because they generate annoying warnings on the MacOS toolchain,

schancel added a comment.Dec 8 2017, 01:56

Invalid sizes failing are checked in the test check_size

schancel added inline comments.Dec 8 2017, 01:57
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?

deadalnix added inline comments.Dec 8 2017, 02:04
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 marked 3 inline comments as done.Dec 8 2017, 02:11
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.
schancel updated this revision to Diff 1992.Dec 8 2017, 02:22

Fix more nits

Harbormaster completed remote builds in B1282: Diff 1992.Dec 8 2017, 02:22
schancel updated this revision to Diff 1993.Dec 8 2017, 02:25

Handle valid sizes better

dagurval added a comment.Dec 8 2017, 10:18

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?

deadalnix added inline comments.Dec 8 2017, 11:16
src/test/cashaddrenc_tests.cpp
66 ↗(On Diff #1993)

const

Also maybe this should be a std::array .

deadalnix added inline comments.Dec 8 2017, 16:56
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);
schancel updated this revision to Diff 1998.Dec 8 2017, 18:08

Fix some nits, and add a test.

Harbormaster completed remote builds in B1285: Diff 1998.Dec 8 2017, 18:08
schancel added inline comments.Dec 8 2017, 18:09
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.

schancel updated this revision to Diff 1999.Dec 8 2017, 18:10

Fix a typo

Harbormaster completed remote builds in B1286: Diff 1999.Dec 8 2017, 18:10
deadalnix added inline comments.Dec 8 2017, 18:59
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)
schancel added inline comments.Dec 8 2017, 19:02
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.

schancel added inline comments.Dec 8 2017, 19:05
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]
schancel updated this revision to Diff 2000.Dec 8 2017, 19:08

Fix nits, change exception back to runtime_error

Harbormaster completed remote builds in B1287: Diff 2000.Dec 8 2017, 19:08
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 3 inline comments as done.Dec 8 2017, 20:33
schancel marked 2 inline comments as done.
schancel marked 4 inline comments as done.
schancel marked 3 inline comments as done.
deadalnix accepted this revision.Dec 8 2017, 20:34
This revision is now accepted and ready to land.Dec 8 2017, 20:34
Closed by commit rABC43d3835e02aa: Encode cashaddr size properly in version bit. (authored by schancel). · Explain WhyDec 8 2017, 20:35
This revision was automatically updated to reflect the committed changes.
schancel marked an inline comment as done and 2 inline comments as not done.

Revision Contents
Changeset List

PathSize
src/
53 lines

Diff 1987

View Options

src/cashaddrenc.cpp

Loading...