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

Prepare for non-Base58 addresses [Step 1] #1593

Merged
merged 7 commits into from
May 20, 2020

Conversation

furszy
Copy link

@furszy furszy commented May 3, 2020

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::strings and CTxDestination.

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

It's not a small change but it will give us much more flexibility once it's fully done over every current and new address type.

The first commit is coming from upstream partially (11117), the rest is purely tackling our code (we have so so many more places using the CBitcoinAddress class).

And finally, have implemented a second abstraction (Destination wrapper) on top of the base58 address to {Encode,Decode,IsValid} Staking addresses as well.

Note:
This migration is not fully complete. It's first step, we still have places using the CBitcoinAddress object instead of the CTxDestination abstraction.

@furszy furszy self-assigned this May 3, 2020
@furszy furszy force-pushed the 2020_refactor_addresses branch 2 times, most recently from c8f0f9e to f45da49 Compare May 11, 2020 02:35
@furszy
Copy link
Author

furszy commented May 11, 2020

Rebased + rpc files CBitcoinAddress migration added.

@random-zebra random-zebra added this to the 5.0.0 milestone May 13, 2020
@random-zebra random-zebra added this to In Progress in perpetual updating PIVX Core to BTC Core via automation May 13, 2020
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Big PR. Good job 👍 We can (should) probably simplify and abstract some of the ugliness due to staking addresses with a better definition of them (either as variant or child class) in the future.

Anyway, looking good from visual inspection: concept and code ACK modulo some questions, comments and/or nits inlined.
Mostly around:

  1. IsValidDestinationString formula for staking addresses with &&

  2. The extra overridden version of IsValidDestinationString: we don't need params to be passed as an external argument to IsValid (at the moment). We should probably include that only when we remove the dependency from CChainParams

  3. The usage of !boost::get<CNoDestination>(&address) instead of IsValidDestination(address). There are many instances, I haven't commented all of them.

  4. a few double decoding operations due to the general path

    if (!IsValidDestinationString(address_string) {
        // return some error
    } 
    CTxDestination dest = DecodeDestination(address_string);
    ...
    

    which should be

    CTxDestination dest = DecodeDestination(address_string);
    if (!IsValidDestination(dest)) {
        // return some error
    }
    ...
    

src/base58.cpp Outdated Show resolved Hide resolved
src/base58.h Show resolved Hide resolved
src/qt/addresstablemodel.cpp Outdated Show resolved Hide resolved
src/rpc/misc.cpp Outdated Show resolved Hide resolved
src/rpc/misc.cpp Outdated Show resolved Hide resolved
src/qt/pivx/addresseswidget.cpp Outdated Show resolved Hide resolved
src/rpc/misc.cpp Outdated Show resolved Hide resolved
src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved
src/wallet/rpcdump.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
This patch removes the need for the intermediary Base58 type
CBitcoinAddress, by providing {Encode,Decode,IsValid}Destination
function that directly operate on the conversion between strings
and CTxDestination.

-- Not all of the CBitcoinAddress instances were ported --

Coming from btc@5c8ff0d448ffdc6340b195ddfa2128d5f21a839b
@furszy
Copy link
Author

furszy commented May 15, 2020

Thanks for the feedback zebra, went commit by commit and solved most of them.

@furszy furszy force-pushed the 2020_refactor_addresses branch 2 times, most recently from 60966f6 to c6a3e91 Compare May 15, 2020 16:00
@furszy
Copy link
Author

furszy commented May 15, 2020

just force pushed it few times because found more boost::get to move to IsValidDestination.

It's good now.

@furszy furszy force-pushed the 2020_refactor_addresses branch 2 times, most recently from 40557f1 to 024d01c Compare May 18, 2020 21:49
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

Nice one. Full sync check done now.

ACK ac99512

Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

ACK ac99512 and merging...

perpetual updating PIVX Core to BTC Core automation moved this from In Progress to Ready May 20, 2020
@random-zebra random-zebra merged commit f59c8fb into PIVX-Project:master May 20, 2020
perpetual updating PIVX Core to BTC Core automation moved this from Ready to Done May 20, 2020
furszy added a commit that referenced this pull request Jun 28, 2020
422d2d3 CBitcoinAddress to wrapper migration (furszy)
287a7b0 CompactTallyItem unused struct removed (furszy)
4b4c5a0 Replace CBitcoinSecret with {Encode,Decode}Secret (furszy)

Pull request description:

  Mixed few things here:

  1) Follow up over #1593 work. The only remaining places that are not using the destination methods are in the walletmodel and wallet objects (next PR), the rest were all already migrated.
  2) CBitcoinSecret with {Encode,Decode}Secret (adapted version of upstream@32e69fa0).
  3) Unused code removal (second commit).

ACKs for top commit:
  random-zebra:
    ACK 422d2d3
  Fuzzbawls:
    ACK 422d2d3

Tree-SHA512: de45c2d66bdb76f48be33352fb278d91294e0171cb1af5f677c5393b7beb8441640aeaf714bc01d3d576e635f3521969b122fb2ab1c7f7aff5cb0c1aeb469b09
@furszy furszy deleted the 2020_refactor_addresses branch November 29, 2022 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants