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

multi: Convert to use new stdaddr package. #2625

Merged
merged 31 commits into from
Apr 12, 2021

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Mar 16, 2021

This requires #2619, #2624, and #2620.

NOTE: I've left this marked as a draft for now because the various go.mod files in the affected modules will need to be updated to use the latest txscript module once the aforementioned prerequisite PRs have landed to avoid issues for external consumers, however, this is otherwise ready for review.


This converts all packages in the repository to make use of the new stdaddr package for addresses instead of dcrutil.Address+txscript.PayToX methods as well as to support and make use of the script version returned by the new addresses accordingly. It also removes the aforementioned txscript.PayToX methods once they are no longer used.

Since this involves modifications to a large number of packages in the repository, this PR is split into a series of individual commits such that everything builds and passes all tests each step of the way. In order to achieve that goal, each commit updates a single package to use the new addresses while simultaneously updating all consumers of that package in the repository to deal with any fallout of API changes. Typically, this means some temporary address conversion code is introduced and then removed in a later commit once it's no longer needed. Finally, all of the unused methods and any associated tests are then removed one by one.

See each individual commit for more details about each individual change, but the following is a high level overview:

  • Converts the following packages to use the new stdaddr package:
    • internal/mining
    • internal/mempool
    • blockchain/stake
    • blockchain/indexers
    • blockchain
    • blockchain/fullblocktests
    • blockchain/chaingen
    • rpcclient
    • rpctest
    • internal/rpcserver
    • hdkeychain
    • txscript
  • Updates all call sites that deal with addresses to properly use the script version returned by the address script generation methods
  • Makes minor modifications for consistency and other cleanup while converting
  • Removes the following methods:
    • chaingen.PurchaseCommitmentScript
    • txscript.PayToSStx
    • txscript.PayToSStxChange
    • txscript.PayToSSGen
    • txscript.PayToSSGenSHDirect
    • txscript.PayToSSGenPKHDirect
    • txscript.PayToSSRtx
    • txscript.PayToSSRtxPKHDirect
    • txscript.PayToSSRtxSHDirect
    • txscript.PayToAddrScript
    • txscript.PayToScriptHashScript
    • txscript.payToSchnorrPubKeyScript
    • txscript.payToEdwardsPubKeyScript
    • txscript.payToPubKeyScript
    • txscript.payToScriptHashScript
    • txscript.payToPubKeyHashSchnorrScript
    • txscript.payToPubKeyHashEdwardsScript
    • txscript.payToPubKeyHashScript
    • txscript.GenerateSStxAddrPush

@davecgh davecgh added this to the 1.7.0 milestone Mar 16, 2021
@davecgh davecgh changed the title Multi: Convert to use new stdaddr package. multi: Convert to use new stdaddr package. Mar 16, 2021
@davecgh davecgh marked this pull request as draft March 16, 2021 18:09
@davecgh davecgh force-pushed the multi_use_stdaddr branch 4 times, most recently from a2e3ae1 to d40284f Compare March 16, 2021 18:55
Copy link
Member

@matheusd matheusd left a comment

Choose a reason for hiding this comment

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

Tentative approval. Will check again and test all commits once all pre-requisite PRs are merged.

@dnldd
Copy link
Member

dnldd commented Mar 23, 2021

Changes look good so far.

@davecgh davecgh force-pushed the multi_use_stdaddr branch 2 times, most recently from e5e04de to 1ace5e6 Compare March 30, 2021 17:23
@davecgh davecgh marked this pull request as ready for review March 30, 2021 17:23
@davecgh
Copy link
Member Author

davecgh commented Mar 30, 2021

I've updated the various go.mod files in the affected modules to use the latest txscript module to avoid issues for external consumers now that the prerequisite PRs have landed.

Copy link
Member

@rstaudt2 rstaudt2 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Changes were straightforward to follow despite the large diff.

internal/rpcserver/rpcwebsocket.go Outdated Show resolved Hide resolved
@matheusd
Copy link
Member

matheusd commented Apr 9, 2021

Builds are failing on the following commits:

b01536a stake: Convert to use new stdaddr package
4f53263 indexers: Convert to use new stdaddr package

There are linting errors after commit 7080a66 due to unused stuff that will be removed in the following commits. This makes run_tests.sh fail for those commits. Wanted to bring this up because I'm not sure whether that's critical or not.

This modifies the test code for joining mempools and blocks to send any
errors that occur over the sync channel and calling fatal in the main
goroutine instead of calling fatal in the goroutines to prevent any
potential teardown issues in the case of failure.

This also makes go vet happy.
This converts the internal mining package to use the stdaddr package
instead of dcrutil.Address as well as to support script versions.

All callers that use the mining package in the repository are updated
accordingly to maintain code that continues to build and work properly
as well as pass all tests.  In order to achieve this, it introduces
temporary address conversion code that will be removed in future
commits.

This is part of a series of commits to convert all packages in the
repository to make use of the new stdaddr package.
This converts the internal mempool package to use the stdaddr package
instead of dcrutil.Address as well as to support script versions.

All callers that use the mempool package in the repository are updated
accordingly to maintain code that continues to build and work properly
as well as pass all tests.

This is part of a series of commits to convert all packages in the
repository to make use of the new stdaddr package.
This converts the blockchain/stake package to use the stdaddr package
instead of dcrutil.Address as well as to support script versions.

All callers that use the package in the repository are updated
accordingly to maintain code that continues to build and work properly
as well as pass all tests.  In order to achieve this, it introduces
temporary address conversion code that will be removed in future
commits.

This is part of a series of commits to convert all packages in the
repository to make use of the new stdaddr package.
This converts the blockchain/indexers package to use the stdaddr package
instead of dcrutil.Address.

All callers that use the package in the repository are updated
accordingly to maintain code that continues to build and work properly
as well as pass all tests.  In order to achieve this, it introduces
temporary address conversion code that will be removed in future
commits.

This is part of a series of commits to convert all packages in the
repository to make use of the new stdaddr package.
This converts the blockchain package to use the stdaddr package instead
of dcrutil.Address.

All callers that use the package in the repository are updated
accordingly to maintain code that continues to build and work properly
as well as pass all tests.

This is part of a series of commits to convert all packages in the
repository to make use of the new stdaddr package.
This removes PurchaseCommitmentScript since it is no longer used by
anything in the repository and is now available via the
RewardCommitmentScript method of the relevant stdaddr address.
This converts the rpcclient package to use the stdaddr package instead
of dcrutil.Address as well as to support script versions.

All callers that use the package in the repository are updated
accordingly to maintain code that continues to build and work properly
as well as pass all tests.

This is part of a series of commits to convert all packages in the
repository to make use of the new stdaddr package.
This converts the rpcclient package to use the stdaddr package instead
of dcrutil.Address as well as to support script versions.

All callers that use the package in the repository are updated
accordingly to maintain code that continues to build and work properly
as well as pass all tests.

This is part of a series of commits to convert all packages in the
repository to make use of the new stdaddr package.
This converts the internal/rpcserver package to use the stdaddr package
instead of dcrutil.Address as well as to support script versions.

All callers that use the package in the repository are updated
accordingly to maintain code that continues to build and work properly
as well as pass all tests.  In order to achieve this, it introduces
temporary address conversion code that will be removed in future
commits.

This is part of a series of commits to convert all packages in the
repository to make use of the new stdaddr package.
This converts the hdkeychain package example to use the stdaddr package
instead of dcrutil.Address.

This is part of a series of commits to convert all packages in the
repository to make use of the new stdaddr package.
@davecgh
Copy link
Member Author

davecgh commented Apr 9, 2021

@matheusd Thanks for checking that! I had only tested them all as I wrote them, so I guess it must've happened in a rebase. I've resolved the build and lint issues in the first two. I think the final ones about unused code are fine since they do not result in compile errors and removing them one by one is intentional to make it easier to follow what's being removed as opposed to one big blob.

txscript/sign.go Outdated Show resolved Hide resolved
This converts the txscript package to use the stdaddr package instead of
dcrutil.Address as well as to support script versions.

All callers that use the package in the repository are updated
accordingly to maintain code that continues to build and work properly
as well as pass all tests.  In order to achieve this, some previously
temporary address conversion code that is no longer needed is removed
from the blockchain/indexers and internal/rpcserver packages.

This is part of a series of commits to convert all packages in the
repository to make use of the new stdaddr package.
This removes PayToSStx since it is no longer used by anything in the
repository and is now available via the VotingRightsScript method of the
relevant stdaddr addresses.
This removes PayToSStxChange since it is no longer used by anything in
the repository and is now available via the StakeChangeScript method of
the relevant stdaddr addresses.
This removes PayToSSGen since it is no longer used by anything in the
repository and is now available via the PayVoteCommitmentScript method
of the relevant stdaddr addresses.
This removes PayToSSGenSHDirect since it is no longer used by anything
in the repository and is now available via the PayVoteCommitmentScript
method of the relevant script hash stdaddr address.
This removes PayToSSGenPKHDirect since it is no longer used by anything
in the repository and is now available via the PayVoteCommitmentScript
method of the relevant pubkey hash stdaddr address.
This removes PayToSSRtx since it is no longer used by anything in the
repository and is now available via the PayRevokeCommitmentScript method
of the relevant stdaddr addresses.
This removes PayToSSRtxPKHDirect since it is no longer used by anything
in the repository and is now available via the PayRevokeCommitmentScript
method of the relevant pubkey hash stdaddr address.
This removes PayToSSRtxSHDirect since it is no longer used by anything
in the repository and is now available via the PayRevokeCommitmentScript
method of the relevant script hash stdaddr address.
This removes PayToAddrScript and its associated example since it is no
longer used by anything in the repository and is now available via the
PaymentScript method of the relevant stdaddr address.
This removes PayToScriptHashScript since it is no longer used by
anything in the repository and is now available via the PaymentScript
method of the relevant script hash stdaddr address.
This removes payToSchnorrPubKeyScript since it is no longer used by
anything in the package and is now available via the PaymentScript
method of the relevant stdaddr address.
This removes payToEdwardsPubKeyScript since it is no longer used by
anything in the package and is now available via the PaymentScript
method of the relevant stdaddr address.
This removes payToPubKeyScript since it is no longer used by anything in
the package and is now available via the PaymentScript method of the
relevant stdaddr address.
This removes payToScriptHashScript since it is no longer used by
anything in the package and is now available via the PaymentScript
method of the relevant stdaddr address.
This removes payToPubKeyHashSchnorrScript since it is no longer used by
anything in the package and is now available via the PaymentScript
method of the relevant stdaddr address.
This removes payToPubKeyHashEdwardsScript since it is no longer used by
anything in the package and is now available via the PaymentScript
method of the relevant stdaddr address.
This removes payToPubKeyHashScript since it is no longer used by
anything in the package and is now available via the PaymentScript
method of the relevant stdaddr address.
This removes GenerateSStxAddrPush since it is no longer used by anything
in the repository and is now available via the RewardCommitmentScript
method of the relevant stdaddr address.

It also removes the associated tests and nilAddrErrStr that is no longer
used upon their removal.
This removes the ErrUnsupportedAddress error kind since it is no longer
needed by anything in the package.
Copy link
Member

@matheusd matheusd left a comment

Choose a reason for hiding this comment

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

Tests passed now (modulo the previously mentioned lint warnings).

@davecgh davecgh merged commit 12c4e56 into decred:master Apr 12, 2021
@davecgh davecgh deleted the multi_use_stdaddr branch April 12, 2021 16:20
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.

None yet

5 participants