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 1285
Build 1285: 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

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

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

Fix calculations

Harbormaster completed remote builds in B1275: Diff 1985.Dec 7 2017, 22:37

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

Fix comments from @deadalnix

deadalnix accepted this revision.Dec 8 2017, 00:33

This revision is now accepted and ready to land.Dec 8 2017, 00:33

Add a test

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.

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

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.

Fix more nits

Harbormaster completed remote builds in B1282: Diff 1992.Dec 8 2017, 02:22

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?

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);

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.

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]

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.

Encode cashaddr size properly in version bit.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 1998

src/cashaddrenc.h

src/cashaddrenc.cpp

src/test/cashaddrenc_tests.cpp

Encode cashaddr size properly in version bit.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 1998

src/cashaddrenc.h

src/cashaddrenc.cpp

src/test/cashaddrenc_tests.cpp

Encode cashaddr size properly in version bit.
ClosedPublic
Actions

Revision Contents
Changeset List