Skip to content

Conversation

PastaPastaPasta
Copy link
Member

Lots of conflicts on this one, so likely so problems. I am also expecting tests to fail

laanwj and others added 12 commits January 17, 2020 11:35
864cd27 Move CBitcoinAddress to base58.cpp (Pieter Wuille)
5c8ff0d Introduce wrappers around CBitcoinAddress (Pieter Wuille)

Pull request description:

  This patch removes the need for the intermediary Base58 type `CBitcoinAddress`, by providing {`Encode`,`Decode`,`IsValid`}`Destination` functions that directly operate on the conversion between `std::string`s and `CTxDestination`.

  As a side, it also fixes a number of indentation issues, and removes probably several unnecessary implicit `CTxDestination`<->`CBitcoinAddress` conversions.

  This change is far from complete. In follow-ups I'd like to:
  * Split off the specific address and key encoding logic from base58.h, and move it to a address.h or so.
  * Replace `CTxDestination` with a non-`boost::variant` version (which can be more efficient as `boost::variant` allocates everything on the heap, and remove the need for `boost::get<...>` and `IsValidDestination` calls everywhere).
  * Do the same for `CBitcoinSecret`, `CBitcoinExtKey`, and `CBitcoinExtPubKey`.

  However, I've tried to keep this patch to be minimally invasive, but still enough to support non-Base58 addresses. Perhaps a smaller patch is possible to hack Bech32 support into `CBitcoinAddress`, but I would consider that a move in the wrong direction.

Tree-SHA512: c2c77ffb57caeadf2429b1c2562ce60e8c7be8aa9f8e51b591f354b6b441162625b2efe14c023a1ae485cf2ed417263afa35c892891dfaa7844e7fbabccab85e
Signed-off-by: Pasta <pasta@dashboost.org>
Signed-off-by: Pasta <pasta@dashboost.org>
Signed-off-by: Pasta <pasta@dashboost.org>
Signed-off-by: Pasta <pasta@dashboost.org>
Signed-off-by: Pasta <pasta@dashboost.org>
Signed-off-by: Pasta <pasta@dashboost.org>
Signed-off-by: Pasta <pasta@dashboost.org>
…sScript()"

Signed-off-by: Pasta <pasta@dashboost.org>
…ript()"

Signed-off-by: Pasta <pasta@dashboost.org>
Signed-off-by: Pasta <pasta@dashboost.org>
@PastaPastaPasta
Copy link
Member Author

Okay, I've resolved all of the errors I've been able to, I'm not sure what to do about address.IsScript()

@UdjinM6
Copy link

UdjinM6 commented Jan 18, 2020

Pls see 80507fc

@PastaPastaPasta
Copy link
Member Author

cherry-picked

@UdjinM6
Copy link

UdjinM6 commented Jan 19, 2020

cherry-picked

Might need to push it again, looks like your previous attempt failed or smth - I don't see 80507fc in your branch.

@PastaPastaPasta
Copy link
Member Author

I only said cherry-picked, I never said I pushed it 😂

@UdjinM6
Copy link

UdjinM6 commented Jan 20, 2020

FYI: test failure on Gitlab is unrelated and should be fixed by #3296

@UdjinM6 UdjinM6 added this to the 16 milestone Jan 20, 2020
Copy link

@codablock codablock left a comment

Choose a reason for hiding this comment

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

LGTM 👍, just nit: 3eb825c

No need to have it in base58.cpp anymore as this is only used in getAddressesFromParams
@PastaPastaPasta
Copy link
Member Author

cherry-picked and pushed this time

Copy link

@codablock codablock left a comment

Choose a reason for hiding this comment

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

utACK

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

@UdjinM6 UdjinM6 merged commit 141861e into dashpay:develop Jan 22, 2020
@PastaPastaPasta PastaPastaPasta deleted the backport-11117 branch January 22, 2020 16:15
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jun 26, 2021
* Merge bitcoin#11117: Prepare for non-Base58 addresses

864cd27 Move CBitcoinAddress to base58.cpp (Pieter Wuille)
5c8ff0d Introduce wrappers around CBitcoinAddress (Pieter Wuille)

Pull request description:

  This patch removes the need for the intermediary Base58 type `CBitcoinAddress`, by providing {`Encode`,`Decode`,`IsValid`}`Destination` functions that directly operate on the conversion between `std::string`s and `CTxDestination`.

  As a side, it also fixes a number of indentation issues, and removes probably several unnecessary implicit `CTxDestination`<->`CBitcoinAddress` conversions.

  This change is far from complete. In follow-ups I'd like to:
  * Split off the specific address and key encoding logic from base58.h, and move it to a address.h or so.
  * Replace `CTxDestination` with a non-`boost::variant` version (which can be more efficient as `boost::variant` allocates everything on the heap, and remove the need for `boost::get<...>` and `IsValidDestination` calls everywhere).
  * Do the same for `CBitcoinSecret`, `CBitcoinExtKey`, and `CBitcoinExtPubKey`.

  However, I've tried to keep this patch to be minimally invasive, but still enough to support non-Base58 addresses. Perhaps a smaller patch is possible to hack Bech32 support into `CBitcoinAddress`, but I would consider that a move in the wrong direction.

Tree-SHA512: c2c77ffb57caeadf2429b1c2562ce60e8c7be8aa9f8e51b591f354b6b441162625b2efe14c023a1ae485cf2ed417263afa35c892891dfaa7844e7fbabccab85e

* CBitcoinAddress -> EncodeDestination in providertx.h

Signed-off-by: Pasta <pasta@dashboost.org>

* more CBitcoinAddress -> EncodeDestination in providertx.h

Signed-off-by: Pasta <pasta@dashboost.org>

* more CBitcoinAddress -> EncodeDestination in providertx.h

Signed-off-by: Pasta <pasta@dashboost.org>

* more CBitcoinAddress -> EncodeDestination in providertx.h

Signed-off-by: Pasta <pasta@dashboost.org>

* fix CBitcoinAddress GetKeyID check

Signed-off-by: Pasta <pasta@dashboost.org>

* fix providertx.cpp

Signed-off-by: Pasta <pasta@dashboost.org>

* hopefully fix governance-classes.cpp

Signed-off-by: Pasta <pasta@dashboost.org>

* partially fix governance-validators.cpp, unable to resolve "address.IsScript()"

Signed-off-by: Pasta <pasta@dashboost.org>

* partially fix governance-classes.cpp, unable to resolve "address.IsScript()"

Signed-off-by: Pasta <pasta@dashboost.org>

* fix governance-classes.h

Signed-off-by: Pasta <pasta@dashboost.org>

* DecodeTransaction -> DecodeDestination, fix governance-validators.cpp

Signed-off-by: Pasta <pasta@dashboost.org>

* More fixes for 3294

* Move GetIndexKey into rpc/misc.cpp near getAddressesFromParams

No need to have it in base58.cpp anymore as this is only used in getAddressesFromParams

Co-authored-by: Wladimir J. van der Laan <laanwj@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: Alexander Block <ablock84@gmail.com>
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Feb 12, 2022
* Merge bitcoin#11117: Prepare for non-Base58 addresses

864cd27 Move CBitcoinAddress to base58.cpp (Pieter Wuille)
5c8ff0d Introduce wrappers around CBitcoinAddress (Pieter Wuille)

Pull request description:

  This patch removes the need for the intermediary Base58 type `CBitcoinAddress`, by providing {`Encode`,`Decode`,`IsValid`}`Destination` functions that directly operate on the conversion between `std::string`s and `CTxDestination`.

  As a side, it also fixes a number of indentation issues, and removes probably several unnecessary implicit `CTxDestination`<->`CBitcoinAddress` conversions.

  This change is far from complete. In follow-ups I'd like to:
  * Split off the specific address and key encoding logic from base58.h, and move it to a address.h or so.
  * Replace `CTxDestination` with a non-`boost::variant` version (which can be more efficient as `boost::variant` allocates everything on the heap, and remove the need for `boost::get<...>` and `IsValidDestination` calls everywhere).
  * Do the same for `CBitcoinSecret`, `CBitcoinExtKey`, and `CBitcoinExtPubKey`.

  However, I've tried to keep this patch to be minimally invasive, but still enough to support non-Base58 addresses. Perhaps a smaller patch is possible to hack Bech32 support into `CBitcoinAddress`, but I would consider that a move in the wrong direction.

Tree-SHA512: c2c77ffb57caeadf2429b1c2562ce60e8c7be8aa9f8e51b591f354b6b441162625b2efe14c023a1ae485cf2ed417263afa35c892891dfaa7844e7fbabccab85e

* CBitcoinAddress -> EncodeDestination in providertx.h

Signed-off-by: Pasta <pasta@dashboost.org>

* more CBitcoinAddress -> EncodeDestination in providertx.h

Signed-off-by: Pasta <pasta@dashboost.org>

* more CBitcoinAddress -> EncodeDestination in providertx.h

Signed-off-by: Pasta <pasta@dashboost.org>

* more CBitcoinAddress -> EncodeDestination in providertx.h

Signed-off-by: Pasta <pasta@dashboost.org>

* fix CBitcoinAddress GetKeyID check

Signed-off-by: Pasta <pasta@dashboost.org>

* fix providertx.cpp

Signed-off-by: Pasta <pasta@dashboost.org>

* hopefully fix governance-classes.cpp

Signed-off-by: Pasta <pasta@dashboost.org>

* partially fix governance-validators.cpp, unable to resolve "address.IsScript()"

Signed-off-by: Pasta <pasta@dashboost.org>

* partially fix governance-classes.cpp, unable to resolve "address.IsScript()"

Signed-off-by: Pasta <pasta@dashboost.org>

* fix governance-classes.h

Signed-off-by: Pasta <pasta@dashboost.org>

* DecodeTransaction -> DecodeDestination, fix governance-validators.cpp

Signed-off-by: Pasta <pasta@dashboost.org>

* More fixes for 3294

* Move GetIndexKey into rpc/misc.cpp near getAddressesFromParams

No need to have it in base58.cpp anymore as this is only used in getAddressesFromParams

Co-authored-by: Wladimir J. van der Laan <laanwj@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: Alexander Block <ablock84@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants