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

main: Update to use all new major module versions. #1837

Merged
merged 1 commit into from
Aug 14, 2019

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Aug 13, 2019

This updates all code in the main module to use the latest major modules versions to pull in the latest updates.

A more general high level overview of the changes is provided below, however, there is one semantic change worth calling out independently.

The verifymessage RPC will now return an error when provided with an address that is not for the current active network and the RPC server version has been bumped accordingly.

Previously, it would return false which indicated the signature is invalid, even when the provided signature was actually valid for the other network. Said behavior was not really incorrect since the
address, signature, and message combination is in fact invalid for the current active network, however, that result could be somewhat misleading since a false result could easily be interpreted to mean the signature is actually invalid altogether which is distinct from the case of the address being for a different network. Therefore, it is preferable to explicitly return an error in the case of an address on the wrong network to cleanly separate these cases.

The following is a high level overview of the changes:

  • Replace all calls to removed blockchain merkle root, pow, subsidy, and coinbase funcs with their standalone module equivalents
    • Introduce a new local func named calcTxTreeMerkleRoot that accepts dcrutil.Tx as before and defers to the new standalone func
  • Update block locator handling to match the new signature required by the peer/v2 module
    • Introduce a new local func named chainBlockLocatorToHashes which performs the necessary conversion
  • Update all references to old v1 chaincfg params global instances to use the new v2 functions
  • Modify all cases that parse addresses to provide the now required current network params
    • Include address params with the wsClientFilter
  • Replace removed v1 chaincfg constants with local constants
  • Create subsidy cache during server init and pass it to the relevant subsystems
    • blockManagerConfig
    • BlkTmplGenerator
    • rpcServer
    • VotingWallet
  • Update mining code that creates the block one coinbase transaction to create the output scripts as defined in the v2 params
  • Replace old v2 dcrjson constant references with new types module
  • Fix various comment typos
  • Update fees module to use the latest major module versions and bump it v2

@davecgh davecgh added this to the 1.5.0 milestone Aug 13, 2019
@davecgh davecgh force-pushed the main_update_use_new_major_modules branch from add4525 to 5cedc59 Compare August 13, 2019 16:16
This updates all code in the main module to use the latest major modules
versions to pull in the latest updates.

A more general high level overview of the changes is provided below,
however, there is one semantic change worth calling out independently.

The verifymessage RPC will now return an error when provided with
an address that is not for the current active network and the RPC server
version has been bumped accordingly.

Previously, it would return false which indicated the signature is
invalid, even when the provided signature was actually valid for the
other network.  Said behavior was not really incorrect since the
address, signature, and message combination is in fact invalid for the
current active network, however, that result could be somewhat
misleading since a false result could easily be interpreted to mean the
signature is actually invalid altogether which is distinct from the case
of the address being for a different network.  Therefore, it is
preferable to explicitly return an error in the case of an address on
the wrong network to cleanly separate these cases.

The following is a high level overview of the changes:

- Replace all calls to removed blockchain merkle root, pow, subsidy, and
  coinbase funcs with their standalone module equivalents
  - Introduce a new local func named calcTxTreeMerkleRoot that accepts
    dcrutil.Tx as before and defers to the new standalone func
- Update block locator handling to match the new signature required by
  the peer/v2 module
  - Introduce a new local func named chainBlockLocatorToHashes which
    performs the necessary conversion
- Update all references to old v1 chaincfg params global instances to
  use the new v2 functions
- Modify all cases that parse addresses to provide the now required
  current network params
  - Include address params with the wsClientFilter
- Replace removed v1 chaincfg constants with local constants
- Create subsidy cache during server init and pass it to the relevant
  subsystems
  - blockManagerConfig
  - BlkTmplGenerator
  - rpcServer
  - VotingWallet
- Update mining code that creates the block one coinbase transaction to
  create the output scripts as defined in the v2 params
- Replace old v2 dcrjson constant references with new types module
- Fix various comment typos
- Update fees module to use the latest major module versions and bump it v2
Copy link
Member

@alexlyp alexlyp left a comment

Choose a reason for hiding this comment

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

tACK on testnet

looks like it just needs a test fixo, the gtg

@davecgh davecgh force-pushed the main_update_use_new_major_modules branch from 5cedc59 to 25c14e0 Compare August 13, 2019 16:28
@jrick
Copy link
Member

jrick commented Aug 13, 2019

To comment on the change to verifymessage, dcrwallet now has this behavior as well which was done during the update to dcrutil/v2.

@davecgh davecgh merged commit 25c14e0 into decred:master Aug 14, 2019
@davecgh davecgh deleted the main_update_use_new_major_modules branch August 14, 2019 01:57
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.

3 participants