Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet: Change default address type to bech32 #16884

Merged
merged 3 commits into from
Oct 2, 2019

Conversation

instagibbs
Copy link
Member

@instagibbs instagibbs commented Sep 16, 2019

No description provided.

@cvengler
Copy link
Contributor

Concept ACK, same is already in bitcoin-qt so this makes it just more coherent.

@instagibbs instagibbs changed the title Change default address type to bech32 RPC: Change default address type to bech32 Sep 16, 2019
@maflcko
Copy link
Member

maflcko commented Sep 16, 2019

I think you can also revert fa5a4cd

@maflcko maflcko changed the title RPC: Change default address type to bech32 wallet: Change default address type to bech32 Sep 16, 2019
@fanquake fanquake added this to the 0.20.0 milestone Sep 16, 2019
@nopara73
Copy link

Concept ACK

@Sjors
Copy link
Member

Sjors commented Sep 17, 2019

Concept ACK. Needs a release note.

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 18, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15606 ([experimental] UTXO snapshots by jamesob)

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.

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
@laanwj
Copy link
Member

laanwj commented Sep 18, 2019

Concept ACK

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

test/functional/wallet_import_with_label.py Outdated Show resolved Hide resolved
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK.

@@ -0,0 +1,4 @@
The wallet now by default uses bech32 addresses when using RPC, and creates native segwit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop "when using RPC"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QT already by default does bech32, so I believe this is more accurate?

@fanquake
Copy link
Member

Concept ACK

1 similar comment
@darosior
Copy link
Member

Concept ACK

@maflcko
Copy link
Member

maflcko commented Sep 25, 2019

in commit 9721429:

Does not compile because you forgot to remove the

// Always fall back to bech32 in the gui

case

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b631464 looks good, did not compile

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK b631464f54e4b826a52c04161492fec89a711866 looks good, did not compile
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiFOQv6A3gQDyHxmmYJuYzWi4MYaCerjQ+6wUjnTgFvkDGsGo70m2yRf9zfb1Tx
vw24BOYmqq8C1eHHDl3JixPTxpYfemdu2tdyNdzM+QLJD4pzcZFGYsGlqpqu6GLO
TUV7MOIbzlBmJ1G4kIQFFeF3cJhcTeYPhQaNLqPbsGhvdpy4CDb8Mj2co7KSFVs3
kieSDVKK4VMbs52srHJel3WLNTW5HORfsTM7CrwAb0TMUn/HwjDmgPcZdZYhYZxU
0K7St/Dtf0NAYdDCxInzyXfMUkhbcE9w4hye2jbtnOIGbZH+DCfsjuaW/ITdmfxW
4HzP7dbYg5IRrY304ohKTgC6P3m+F7Qw57R7hrpo90sgdIy/m0k4E89Ifv08pP8O
bHWzPYOv560vX6zNRoVjztY6/yNwWceJ9/SX6L6otx4D28fsTJleDfEWTHIebqKk
gp71AUYwNDrSvcyvu/uayp93U1enrmS97pq1kz3eR6irRL1G6hLsCVDcgfW9Y9U2
3qR5na16
=QpM0
-----END PGP SIGNATURE-----

Timestamp of file with hash 0979dba1d91ee3700a115a74e85034597fa657a8e983564f537c26f83c78585e -

src/wallet/init.cpp Outdated Show resolved Hide resolved
src/wallet/wallet.cpp Outdated Show resolved Hide resolved
@instagibbs
Copy link
Member Author

I'm going to revert my change-mimicking revert due to unpopularity on IRC.

@instagibbs
Copy link
Member Author

updated to reduce PR scope to just the default address option

@laanwj laanwj added the Feature label Sep 30, 2019
@hebasto
Copy link
Member

hebasto commented Sep 30, 2019

Concept ACK

@maflcko maflcko added Feature and removed Feature labels Sep 30, 2019
@maflcko
Copy link
Member

maflcko commented Sep 30, 2019

re-ACK 71d4edd (only change is restore mimick behavior)

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK 71d4eddf42eb5a15e434d2273f11d0a3942ef502 (only change is restore mimick behavior)
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUi42wv9E53RyaKcAWQFXqT0Tg+mZ0oi+mpCcxAUi5oQ7VYHS8tZCPyhTn0Xy3cl
yW4uf3N73WkJeRTpI5zvdqcBDm9yYKM7O33aS8kx7odWv1bOLOSu8aVqPT2P5xFq
CvZ+HpZP3HF8H7nAOVJmTX2gb6qRciPmyrbvqYsjwiG/Z5hVN4GXfGQTm9f8dE1K
rQHLOCym9MrIwRsDDRiNf08NxNwSgCqhO+eOtRo39gSmuGUj5nz9l+BsZmk8m8a5
CxqfHl4JOvwMzUjvbAmaq5r8hEQYYKiqJ09KoQn7ARsc6CquR+T086EGPArT+/Y5
RFbu/XcqGSUJsicinBaoywgrr1+xNfiRwOKW1iTviq5H54WrXTPYNhdo59Tg6HbY
yznrPkswly5eJE/dfrMEeEi2YZpM3JGhwZugN/svnBQszUa3wzMELl1rX8KWowmW
pAFXHDAD81J/XaKXj1eSzW2xF2WcnYCFcEE2/NxobUKkqzdIfqMAyP18TNuL7sUV
Uh3SQ4p5
=LUrX
-----END PGP SIGNATURE-----

Timestamp of file with hash f7ab15c1dbf906029708de7346a7a88e7b8e07d3b53b29c8db5fbb4e4762c3f7 -

@maflcko
Copy link
Member

maflcko commented Sep 30, 2019

Could add a commit to bump doc/bips.md (173)?

@laanwj
Copy link
Member

laanwj commented Oct 2, 2019

ACK 71d4edd
I think it's good to merge this early in the 0.20 cycle.

Could add a commit to bump doc/bips.md (173)?

Please do this in a follow-up PR.

laanwj added a commit that referenced this pull request Oct 2, 2019
71d4edd Add release note for bech32 by default in wallet (Gregory Sanders)
b34f018 Revert "gui: Generate bech32 addresses by default (take 2, fixup)" (Gregory Sanders)
f50785a Change default address type to bech32 (Gregory Sanders)

Pull request description:

ACKs for top commit:
  MarcoFalke:
    re-ACK 71d4edd (only change is restore mimick behavior)
  laanwj:
    ACK 71d4edd

Tree-SHA512: 3c49a1b51c49f3a762ad08985167ca1b89b0177ae20ab6d5883f1f74dde7a155921c1b855a842199bbf32f563c56b33f8b603bc842637bdcb121001023d454b6
@laanwj laanwj merged commit 71d4edd into bitcoin:master Oct 2, 2019
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 2, 2019
71d4edd Add release note for bech32 by default in wallet (Gregory Sanders)
b34f018 Revert "gui: Generate bech32 addresses by default (take 2, fixup)" (Gregory Sanders)
f50785a Change default address type to bech32 (Gregory Sanders)

Pull request description:

ACKs for top commit:
  MarcoFalke:
    re-ACK 71d4edd (only change is restore mimick behavior)
  laanwj:
    ACK 71d4edd

Tree-SHA512: 3c49a1b51c49f3a762ad08985167ca1b89b0177ae20ab6d5883f1f74dde7a155921c1b855a842199bbf32f563c56b33f8b603bc842637bdcb121001023d454b6
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 17, 2019
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.