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

chaincfg: Introduce v2 module. #1698

Merged
merged 16 commits into from
Jun 13, 2019
Merged

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Mar 24, 2019

NOTE: This currently relies on an unreleased version of the chaincfg module (which is why it is failing Travis). That will need to be released before this can be merged. However, it is otherwise ready for review.

The chaincfg package currently has some less-than ideal APIs and behaviors that have been retained up to this point in order to avoid breaking existing caller code. However, now that versioned module support is available and in full use, these items can be addressed.

A quick overview of the issues with chaincfg this aims to address are:

  • The block one 1 ledger is defined in terms of address strings, which forces consensus code to deal with standardness/policy code which it should not have to do (See chaincfg/blockchain: Change Block One Ledger To Require Exact Scripts #1659)
  • It provides, and relies on, global registration which has side effects upon importing the package and can cause issues if multiple copies of the same package try to register themselves
  • The current network parameter definitions are all package-level globals which have a few undesirable characteristics (See chaincfg: Convert Network Definitions To Functions #1667):
    • They are subject to mutation and thus can easily cause subtle and hard to find bugs
    • Many of the fields are defined in terms of others, but due to the struct being defined at the top level, it's not possible to reference other fields within it
    • They require a lot of unexported global variable state
    • It makes it difficult to introduce a base set of parameters that would likely help highlight exactly what the differences are between the networks
  • It exports several constants which are not per-network constants and therefore do not belong in chaincfg
    • Further, because the constants are exported, they couldn't be changed without a major version bump, which would cause everything that relies on it to also bump majors.

Consequently, this introduces chaincfg/v2 which addresses the aforementioned items. A series of individual commits to make the review process easier. Each commit message more thoroughly describes its purpose, but primarily they consist of the following:

  • Freeze version 1 of the chaincfg module
  • Remove the root module chaincfg override so building the software will still produce binaries based on the v1 module until the v2 module is fully released
  • Remove several constants which are not per-network and do not belong
  • Minor consistency cleanup in order definition and tests
  • Add tests for required unique fields in lieu of relying on registration to catch duplicates
  • Remove all registration capabilities and associated functions
  • Move code solely related to each network into the file for that network
  • Use a concrete for the genesis hash in the Params struct
  • Use scripts in block one token payouts instead of addresses
  • Convert global network parameter defintions to functions
  • Bump the module version to v2
  • Update docs for the new module version

Finally, it should also be noted that this only introduces the new module and does not update anything to make use of it yet, so building the software will still produce binaries based on the v1 module.


For reference when moving to the new version of the module, the following is a list of changes to the public API:

Removed Functions:

  • Register
  • IsPubKeyAddrID
  • IsPubKeyHashAddrID
  • IsPKHEdwardsAddrID
  • IsPKHSchnorrAddrID
  • IsScriptHashAddrID
  • HDPrivateKeyToPublicKeyID
  • ParamsByNetAddrPrefix

Added Functions:

  • MainNetParams
  • TestNet3Params
  • SimNetParams
  • RegNetParams

Removed Constants:

  • MainNetParams (Use MainNetParams() func instead)
  • TestNet3Params (Use TestNet3Params() func instead)
  • SimNetParams (Use SimNetParams() func instead)
  • RegNetParams (Use RegNetParams() func instead)
  • SigHashOptimization
  • CheckForDuplicateHashes
  • CPUMinerThreads
  • ErrDuplicateNet
  • ErrUnknownHDKeyID
  • ErrDuplicateNetAddrPrefix
  • ErrUnknownNetAddrPrefix

Closes #1659.
Closes #1667.

@davecgh davecgh added this to the 1.5.0 milestone Mar 24, 2019
@davecgh davecgh force-pushed the chaincfg_introduce_v2 branch 2 times, most recently from b7cb08e to d5bb455 Compare March 29, 2019 17:29
This serves as the final release of version 1 of the chaincfg module.
All future releases will be moving to version 2 of the module.

Consequently, it bumps the module version as follows:

- github.com/decred/dcrd/[email protected]

It also removes the chaincfg override in the root module and updates it
so building the software will still produce binaries based on the v1
module until the v2 module is fully released.
This removes the exported SigHashOptimization constant because it is not
a per-chain parameter and therefore does not belong here.

Further, it is only used in txscript and since it's an exported
constant, it couldn't be changed without a major version bump, which
would cause everything that relies on it to also bump majors.
This removes the exported CheckForDuplciateHashes constant because it is
not a per-chain parameter and therefore does not belong here.

Further, it is only used in blockchain and since it's an exported
constant, it couldn't be changed without a major version bump, which
would cause everything that relies on it to also bump majors.
This removes the exported CPUMinerThreads constant because it is not a
per-chain parameter and therefore does not belong here.

Further, it is only used in the mining code and since it's an exported
constant, it couldn't be changed without a major version bump and there
is no reason this shouldn't be defined in the mining code itself.
This adds a test to ensure that all of the network parameter fields that
are required to be unique are in fact unique for all of the provided
default parameters.

It should be noted that part of this is currently redundant with the
network registration code, however, that is going to be removed.
This removes all of the global registration capabilities from chaincfg
as part of the overall drive towards removing side effects that alter
the global state when importing packages and providing cleaner module
boundaries by reducing tight coupling betwee them.

For a bit of history, the package was designed back near the time Go
released before anyone had much experience with Go based on some other
early examples in the stdlib which many, including most of the original
authors, have come to realize is not ideal because it essentially alters
global state and has side effects merely by importing the package. This
can cause issues if multiple copies of the same package try to register
themselves.

Also, on a less theoretical note, with the benefit of hindsight and
experience, there isn't really any practical benefit to registering the
networks and allowing custom ones to be registered over just accepting a
params interface into the packages that need them whereas there are
tangible downsides having global registration factories as mentioned.
Accepting interfaces to the relevant functions in the other packages
will also significantly reduce the tight coupling that currently exists
with chaincfg and limit the scope of the needed parameters to
specifically those required by the functions in question.

An overview of the changes is as follows:

- Remove the following functions:
  - Register
  - IsPubKeyAddrID
  - IsPubKeyHashAddrID
  - IsPKHEdwardsAddrID
  - IsPKHSchnorrAddrID
  - IsScriptHashAddrID
  - HDPrivateKeyToPublicKeyID
  - ParamsByNetAddrPrefix
- Remove the following (now unused) errors:
  - ErrDuplicateNet
  - ErrUnknownHDKeyID
  - ErrDuplicateNetAddrPrefix
  - ErrUnknownNetAddrPrefix
- Remove all associated tests
- Update README to call out correct number of default net params
This moves the various code that is specific to mainnet to the
mainnetparams.go file to keep the information housed together.  Along
the same lines, it moves the tests specific to the mainnet params to
mainnetparams_test.go.

It also unexports the block on ledger definition since it should only be
exposed via the params.
This moves the various code that is specific to testnet to the
testnetparams.go file to keep the information housed together.  Along
the same lines, it moves the tests specific to the testnet params to
testnetparams_test.go.

It also unexports the block one ledger definition since it should only
be exposed via the params.
This moves the various code that is specific to simnet to the
simnetparams.go file to keep the information housed together.  Along the
same lines, it moves the tests specific to the simnet params to
simnetparams_test.go.

It also unexports the block one ledger definition since it should only
be exposed via the params.
This moves the various code that is specific to regnet to the
regnetparams.go file to keep the information housed together.  Along the
same lines, it moves the tests specific to the regnet params to
regnetparams_test.go.

It also unexports the block one ledger definition since it should only
be exposed via the params.
This modifies the Params struct to store the genesis hash directly
instead of a pointer to it.  This makes it more consistent with the rest
of the code base and avoids the need for a separate variable.
This modifies the definitions for the required payouts in block one to
define required scripts instead of an address string as well as specify
the required script version and updates all of the network params
accordingly.

This is being done because addresses are a non-consensus construct that
essentially provide a human-readable method of specifying the actual
underlying cryptographic primitives.  Thus, the consensus rules
themselves must not directly rely on the ability to interpret and parse
addresses.

It is important to note that, once the blockchain code is updated to
make use of the changes this introduces, it is technically a fork since
the code will be more restrictive, but since it only applies to block
one, it's irrelevant because:

- It's buried under years of blocks
- The actual ledger of block one is unchanged
The current network parameter definitions are all package-level globals
which have a few undesirable characteristics:

- They are subject to mutation and thus can easily cause subtle and hard
  to find bugs
- Many of the fields are defined in terms of others, but due to the
  struct being defined at the top level, it's not possible to reference
  other fields within it
- They require a lot of unexported global variable state
- It makes it difficult to introduce a base set of parameters that would
  likely help highlight exactly what the differences are between the
  networks

Consequently, this converts all of the global network parameter
defintions to functions that return a new instance of the relevant
parameters which either directly addresses the aforementioned point, or
at the very least, allows them to be addresses in future commits.

The following is a high level overview of the changes:

- Move all default definitions into functions that return a new instance
  of the relevant params
- Move global defintions of variables used in the structs into the
  relevant funcs
- Improve consistency in the tests for each set of network params
- Update example in README.md
- Update example doc.go
This updates the docs/README.md file, module hierarchy graphviz, and
module hierarchy diagram to reflect the new module version
@davecgh davecgh merged commit 67ace6e into decred:master Jun 13, 2019
@davecgh davecgh deleted the chaincfg_introduce_v2 branch June 13, 2019 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants