Page MenuHomePhabricator

Use the new cashaddr format
AbandonedPublic

Authored by dagurval on Sep 16 2017, 21:05.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

This replaces the existing base58 address format with the new base32 cashaddr format.

Test Plan

Updated unittest.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cashaddr-rebase
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 992
Build 992: arc lint + arc unit

Event Timeline

dagurval created this revision.Sep 16 2017, 21:05
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 16 2017, 21:05
dagurval updated this revision to Diff 1383.Sep 17 2017, 11:55

Differentiate between p2kh and p2sh with version instead of prefix.

Uses version == 0 for p2kh, version == 8 for p2sh. Lower 3 bits are reserved.

dagurval updated this revision to Diff 1384.Sep 17 2017, 11:56

missing commits

deadalnix requested changes to this revision.Sep 17 2017, 13:06
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/base58.cpp
6 ↗(On Diff #1384)

We should rename this as this is clearly not bech32 anymore. cashaddress maybe ?

216 ↗(On Diff #1384)

You can make these uint8_t which would remove the need for cast.

226 ↗(On Diff #1384)

unsigned char => uint8_t

253 ↗(On Diff #1384)

braces around ifs.

327 ↗(On Diff #1384)

Let's not add more reference to global state. Pass the params down.

331 ↗(On Diff #1384)

dito

340 ↗(On Diff #1384)

dito

src/bech32.cpp
34 ↗(On Diff #1384)

Please format like:

/**
 * blablabla
 */

You can put the comment on one line and let clang-format do its magic.

42 ↗(On Diff #1384)

dito

54 ↗(On Diff #1384)

dito

68 ↗(On Diff #1384)

dito

78 ↗(On Diff #1384)

dito

111 ↗(On Diff #1384)
if (c0 & 1) {
  // k(x) = {29}x^5 + {22}x^4 + {20}x^3 + {21}x^2 + {29}x + {18}
  c ^= 0x3b6a57b2;
}
144 ↗(On Diff #1384)

Hash the lower 5 bits of all characters + a zero.

163 ↗(On Diff #1384)

Comment on the previous line.

199 ↗(On Diff #1384)

Use ':' as a separator.

src/chainparams.cpp
198 ↗(On Diff #1384)

bitcoincash.

362 ↗(On Diff #1384)

Not sure, but something more specific.

479 ↗(On Diff #1384)

dito

This revision now requires changes to proceed.Sep 17 2017, 13:06
dagurval updated this revision to Diff 1393.Sep 20 2017, 20:00
dagurval edited edge metadata.
dagurval marked 11 inline comments as done.

formatting, renamed to cashaddr, changed seperator to :, other nits based on feedback

dagurval marked 3 inline comments as done.Sep 20 2017, 20:02
dagurval added inline comments.
src/base58.cpp
327 ↗(On Diff #1384)

This is a huge change all over the code base. Can we put this one on the wish list?

deadalnix requested changes to this revision.Sep 27 2017, 12:44

I think this diff should be splet as right now, it is doing 2 things at once:

  • Implement the format
  • Integrate the format.

Let's focus on the first one and do the second one in another diff. We don't want any problem with one to slow down the other and vice versa.

src/base58.cpp
327 ↗(On Diff #1384)

OK

src/cashaddr.cpp
1 ↗(On Diff #1393)

Add a compyright notice for the bitcoin developers

35 ↗(On Diff #1393)

We will change that one slightly, but we can use it in the meantime.

126 ↗(On Diff #1393)

uint8_t, no unsigned char . There are numerous others.

130 ↗(On Diff #1393)

We should use prefix instead of HRP

141 ↗(On Diff #1393)

We discussed this already, just go throught the lower 5 bits of all char and then a 0. There is no point checksuming the higher bits as they are constants.

156 ↗(On Diff #1393)

+8

158 ↗(On Diff #1393)

uint64_t

172 ↗(On Diff #1393)

CashAddress

184 ↗(On Diff #1393)

CashAddress

190 ↗(On Diff #1393)

braces

194 ↗(On Diff #1393)

braces

src/chainparams.h
89 ↗(On Diff #1393)

Prefix

104 ↗(On Diff #1393)

dito

src/test/cashaddr_tests.cpp
1 ↗(On Diff #1393)

This needs to be changed to test the format. We should probably rewrite it anyway rather than backporting the one from core.

src/utilstrencodings.h
145 ↗(On Diff #1393)

This would deserve a few tests.

This revision now requires changes to proceed.Sep 27 2017, 12:44
dagurval planned changes to this revision.Oct 4 2017, 18:36

I'll update this diff when D575 or D576 have landed

dagurval updated this revision to Diff 1531.Oct 9 2017, 20:08
dagurval edited edge metadata.

Added support for encoding CTxDestination with base58 (the old way). Rebased on top of D576.

dagurval retitled this revision from [WIP] Use bech32 for addresses instead of base58. to [WIP] Use new cashaddress format.Oct 9 2017, 20:10
dagurval edited the summary of this revision. (Show Details)
dagurval updated this revision to Diff 1556.Oct 14 2017, 20:57

rebased

dagurval retitled this revision from [WIP] Use new cashaddress format to Use the new cashaddr format.Oct 14 2017, 20:58
dagurval edited the summary of this revision. (Show Details)
dagurval edited the test plan for this revision. (Show Details)

There are various self contained changes here that could go in now.

The first obvious one is the chain param change.

Another one is the logic for cashaddr key extraction, that can be added and tested as a standalone change without being wired in the existing logic.

dagurval planned changes to this revision.Nov 7 2017, 08:20
dagurval abandoned this revision.Nov 10 2017, 22:55