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

Report live value of decentralization parameter d. #1725

Merged
merged 10 commits into from
Jun 5, 2020

Conversation

jonathanknowles
Copy link
Contributor

@jonathanknowles jonathanknowles commented Jun 4, 2020

Issue Number

#1693

Overview

This PR:

  • Introduces the DecentralizationLevel type (based on the Percentage type).

    Greater values indicate greater levels of decentralization, as follows:

    Value Meaning
    0% Network is completely federalized
    100% Network is completely decentralized
  • Adjusts the wallet API server to convert from values of type DecentralizationLevel to values of type Quantity "percent" Percentage.

  • Adjusts the wallet layer to store a value of type ProtocolParameters instead of TxParameters. (Note that ProtocolParameters includes TxParameters.)

  • Adjusts the network layer to provide a getProtocolParameters function instead of getTxParameters.

  • Adjusts the database layer to use a protocol_parameters table instead of a tx_parameters table. (Note that protocol_parameters is a superset of tx_parameters.)

  • Adjusts the chain-following code to extract the live value of d, in the same way as we already do for transaction-related protocol parameters.

  • Adds a conversion function to convert from d parameter values to values of type DecentralizationLevel.

    This function performs the following mapping:

    {1 → 0%, 0.9 → 10%, 0.8 → 20%, ..., 0 → 100%}
    

Comments

Starting a wallet with this PR applied will result in the creation of a new protocol_parameters table, if one does not already exist. (We rely on the automatic migration mechanism to create the table, and the network layer to add rows to the table.)

The redundant tx_parameters table, if it exists, will be ignored, but not deleted.

In a future PR, we can add a manual migration step that removes the tx_parameters table.

See Issue #1727.

@jonathanknowles jonathanknowles requested a review from rvl June 4, 2020 07:20
@jonathanknowles jonathanknowles self-assigned this Jun 4, 2020
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/live-decentralization-parameter branch 5 times, most recently from e8dcb9b to af0dc50 Compare June 4, 2020 10:05
@jonathanknowles jonathanknowles added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label Jun 4, 2020
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/live-decentralization-parameter branch from af0dc50 to a5734f6 Compare June 4, 2020 13:50
Copy link
Contributor

@rvl rvl 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 so far

lib/byron/src/Cardano/Wallet/Byron/Network.hs Outdated Show resolved Hide resolved
@rvl
Copy link
Contributor

rvl commented Jun 4, 2020

Write a manual DB upgrade step to cope with the changed schema.

Rather than doing this migration step, you could change readWalletProtocolParameters to use the genesis block as a fallback if the data is missing. That might be easier.

@jonathanknowles jonathanknowles force-pushed the jonathanknowles/live-decentralization-parameter branch from a5734f6 to ee9d006 Compare June 5, 2020 01:46
jonathanknowles added a commit that referenced this pull request Jun 5, 2020
Add `HasCallStack` constraint to function `decentralizationLevelFromPParams`
and all functions that call it directly.

In response to review feedback:

#1725 (comment)
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/live-decentralization-parameter branch from ee9d006 to 4bc118b Compare June 5, 2020 03:28
jonathanknowles added a commit that referenced this pull request Jun 5, 2020
github.com/input-output-hk/cardano-wallet/wiki/Coding-Standards#limit-line-length-to-80-characters
Generalize `TxParameters` to `ProtocolParameters` in the wallet, network
and DB layers.
Add `HasCallStack` constraint to function `decentralizationLevelFromPParams`
and all functions that call it directly.

In response to review feedback:

#1725 (comment)
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/live-decentralization-parameter branch from 4de805b to 93ee74f Compare June 5, 2020 04:57
@jonathanknowles
Copy link
Contributor Author

jonathanknowles commented Jun 5, 2020

Rather than doing this migration step, you could change readWalletProtocolParameters to use the genesis block as a fallback if the data is missing. That might be easier.

After a discussion with @rvl, we decided to leave readWalletProtocolParameters unchanged.

Justification:

  1. If a user starts with an old version of the wallet DB (one with a tx_parameters table instead of a protocol_parameters table), then the automatic migration mechanism will kick in and create an empty protocol_parameters table.
  2. Once the network layer has started, it will populate the empty protocol_parameters with a row for the wallet.
  3. The readWalletProtocolParameters function is only ever called by the selectCoinsSetup function.
  4. This means that immediately after a migration from an older database without a protocol_parameters table, attempting to create a coin selection will fail if the network layer has not yet started, because we rely on the network layer to populate the protocol_parameters table.
  5. However, in normal startup situations without a migration, if the protocol_parameters table already exists and is already populated, then creating a coin selection will succeed even if the network layer has not yet started.

So the corner case here is: attempting to create a coin selection immediately after a migration, but before the network layer has started. In this corner case, creating a coin selection will fail.

On the assumption that this corner case is relatively rare and easy to work around (by waiting for the network layer to start), we've decided to leave the readWalletProtocolParameters function unchanged, for the moment.

@jonathanknowles jonathanknowles requested a review from rvl June 5, 2020 07:29
@jonathanknowles jonathanknowles marked this pull request as ready for review June 5, 2020 07:29
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Great! 👍

@jonathanknowles
Copy link
Contributor Author

bors r+

@jonathanknowles jonathanknowles changed the title WIP: Report live value of decentralization parameter d. Report live value of decentralization parameter d. Jun 5, 2020
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 5, 2020

@iohk-bors iohk-bors bot merged commit fe342a5 into master Jun 5, 2020
@iohk-bors iohk-bors bot deleted the jonathanknowles/live-decentralization-parameter branch June 5, 2020 08:45
iohk-bors bot added a commit that referenced this pull request Jun 5, 2020
1728: Remove a duplicated comment section. r=jonathanknowles a=jonathanknowles

# Issue Number

#1725 

# Overview

This PR removes a duplicated section of comment.

Co-authored-by: Jonathan Knowles <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants