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

Add update payload to chain traces #649

Merged
merged 17 commits into from Jul 19, 2019

Conversation

dnadales
Copy link
Member

Part of #621

100 characters is not suitable for working with two vertical panes on a laptop,
with a font size that is legible for people with less than optimal vision (like
@dnadales :) ).
…dorsements

See `updateProposalAndVotesGen` and `protocolVersionEndorsementGen`.

In preparation for adding the update payload to the `CHAIN` traces.
... where the number of characters in the system tags were not being
counted.

[skip ci]
@@ -31,7 +32,7 @@ instance STS BHEAD where

data PredicateFailure BHEAD
= HashesDontMatch -- TODO: Add fields so that users know the two hashes that don't match
| HeaderSizeTooBig -- TODO: Add more information here as well.
| HeaderSizeTooBig BlockHeader Natural Natural
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these parameters?

NoGenUTxO -> pure []

(anOptionalUpdateProposal, aListOfVotes) <-
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, does this not want to be guarded in the same way with shouldGenUpdate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was being lazy here since we generate update payload only sporadically, but you're right, we should try to adhere to the principle of least surprise :)

@@ -1102,7 +1128,7 @@ ppsUpdateFrom pps = do
`increasingProbabilityAt` (0, 1)

nextFactorA :: Gen Int
nextFactorA =
nextFactorA = --pure _factorA
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦‍♂️ 🗞️

It allowed to produce a maximum transaction size that was bigger than the
current maximum block size.
…als do not change the maximum transaction-size"

- Increase the trace length to 300 in the `onlyValidSignalsAreGenerated @CHAIN` propoerty
@dnadales dnadales merged commit 52d8b21 into master Jul 19, 2019
@dnadales dnadales deleted the dnadales/621-add-update-payload-to-chain-traces branch July 19, 2019 10:08
kevinhammond pushed a commit that referenced this pull request Oct 28, 2019
- Lower the coverage threshold for "at least 10% of the update proposals do not
  change the maximum transaction-size"
- Increase the trace length to 300 in the `onlyValidSignalsAreGenerated @CHAIN`
  propoerty
- Fix the update generator. It allowed to produce a maximum transaction size
  that was bigger than the current maximum block size.
- Guard the `newMaxBkSize - 1` against underflows
- Lower 10% the bounds for the "at least 10% of the proposals get enough
  endorsements" coverage check.
- Generate `CHAIN` delegation payload only in 30% of the cases.
- Tweak the coverage metrics to account for the fact that we do not want to
  decrease certain protocol parameter values to prevent the signal production
  (blocks, transactions, etc) from stopping.
- Set a memory limit for the tests in `cs-blockchain`
- Fix the abstract size test where the number of characters in the system tags
  were not being counted.
- Fix arithmetic underflow when checking validity of proposed script version.
- Factor out functions for generating update proposal and votes, and
  endorsements. See `updateProposalAndVotesGen` and
  `protocolVersionEndorsementGen`.
- Ignore `stack` lock file.
- Limit the line width to 80. 100 characters is not suitable for working with
    two vertical panes on a laptop, with a font size that is legible for people
    with less than optimal vision (like @dnadales :) ).
nc6 pushed a commit that referenced this pull request May 19, 2020
649: Update dependencies r=erikd a=erikd



Co-authored-by: Erik de Castro Lopo <erikd@mega-nerd.com>
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

2 participants