Move override support for max tx capacities into the ProtocolInfo constructors #3261

nfrisby · 2021-07-12T06:06:55Z

Fixes #3235 (again).

See the individual commit messages, some of which are extensive.

nfrisby · 2021-07-12T20:15:32Z

@jasagredo This is ready for review now. The first 5 commits are as before, the remaining commits are a bit of a departure. It's something Pawel and I had discussed before, and once I got my hands in here, it seemed worthwhile. The commit message on consensus: implement TxLimits.pointwiseMin via a meet-semilattice explains my reasoning.

I would understand if this feels like too much. The reason I've left it in one PR is because the downstream interface is different as of the last commit. I don't like merging work that is known to be incomplete, so I'd like to review these commits together.

On that note, I've left it as a Draft PR after all, since I'll want to squash these commits down once we're happy with them; the first commit goes one way (introduces Overrides = Measure -> Measure), but we end up somewhere else by the end of the PR (Overrides = Measure).

Also, if the Generics stuff is unfamiliar, let me know and I can ping Clarke/Edsko/etc for review there 👍

nfrisby · 2021-07-13T17:00:33Z

We discussed this in the Planning Meeting; Consensus Team + @nc6

The lattice based approach is fine, has enough minor benefits that they add up.
Same for the generic definitions -- they're very simple and allow for deriving everything for AlonzoMeasure (and future such types).
We should define the lattice stuff in cardano-base, however. (Spoiler: I'm going to send FromGeneric with it in my PR there.)

We also spent a couple hours (by now) discussing what the right interface is for these lattice concepts we're invoking. This question remains unanswered. But here are some remarks.

We know we have a ("full") lattice: both meet and join and both bottom and top. EG in the concrete cases (like AlonzoMeasure), we want https://en.wikipedia.org/wiki/Product_order , which corresponds to a full lattice assuming finite types for the components (which is implied by the fact that the ledger state not representing infinite values).
However, we do not want a join operation. Whereas meet lets the user's override to tighten the bound (ie restrict it), join would loosen it (ie relax it). Which would create a block that other nodes not overriding the ledger limit would then consider invalid. On the other hand, meet can't create invalid blocks, just smaller-than-necessary blocks.
So what we want is the full lattice structure, but also to explicitly prevent any (direct?) use of join.
In addition, we also need the + operation to add up the sizes of each tx in the list of candidates for inclusion in the next block (currently this is via Monoid).

Then we questioned if we definitely want to disallow relaxing the ledger limit -- eg maybe benchmarking team would do that to every node on a testnet. But in that case they should just change the limit in the ledger state so that the largest block they'd want to create is valid on every node. They could still use the restriction (ie meet) in order to have some nodes create smaller-than-necessary blocks, if they're going for a mixture.

Ultimately, though, we're wondering who really needs this --- maybe we should rip it out, the ultimate simplification :D I've asked on the Node Team slack channel for clarification regarding this, included pinging Benchmarking Team lead.

nfrisby · 2021-07-13T17:21:41Z

Ultimately, though, we're wondering who really needs this --- maybe we should rip it out, the ultimate simplification :D I've asked on the Node Team slack channel for clarification regarding this, included pinging Benchmarking Team lead.

@njd42 and I discussed it. He pointed out that honest SPOs being able to quickly restrict the size of blocks they forge could help us avoid potential disaster scenarios. And some honest SPOs may (currently?) need this to make-up for limited CPU/Mem/etc resources. So we should include it. (Even if we excluded it, it'd be easy enough for some one intent on this to patch it into their fork of the code.)

These current definitions will be used to simplify eg PR IntersectMBO/ouroboros-network#3261

jasagredo

In general, and provided that the Semilattice machinery is moved to cardano-base, this PR looks really good.

ouroboros-consensus/src/Ouroboros/Consensus/Mempool/TxLimits.hs

scripts/buildkite/check-stylish.sh

nfrisby · 2021-07-19T05:41:11Z

@EncodePanda -- as has become par for the course, I'm surprised that I didn't get this merged during your vacation week. I'll Slack you some details, but please focus on Genesis. I expect we can review this on a quick phone call sometime early this week.

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Mempool.hs

Significant changes since that Approval

ouroboros-consensus/src/Ouroboros/Consensus/Mempool/TxLimits.hs

nfrisby · 2021-07-23T14:27:35Z

@EncodePanda I'll resolve the two open Comments today (comments on Overrides and the change in type on noOverrides and mkOverrides to make it simpler for users to understand and use).

Would you review the rest of the PR around those? Those two will be straight forward for me, so please feel free to Approve before I upload fixes for those.

Edit: I resolved them and rebased onto origin/master. I think it's read for final review. 👍

EncodePanda

LGTM, minor comments

ouroboros-consensus/src/Ouroboros/Consensus/Block/Forging.hs

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Mempool.hs

A couple weeks ago, Edsko's insight set us on the path: the abstract Consensus layer need not know about the limits on the size of block. That is ledger specific; so only the atomic ledgers' (Byron, Shelley, etc -- not the HardForkBlock) forge functions needed to query and enforce that limit. We later realized that this same insight applies to the user-submitted overrides of the ledger limits: the abstract Consensus layer receives a `ProtocolInfo` structure from the downstream user. That includes `BlockForging` structures, which include partial applications of the atomic forge functions. When the downstream user builds the `ProtocolInfo`, they can supply the user's overrides there, so that the abstract layer is never involved. As a result, the `maxTxCapacityOverride` field is now vestigial. This may seem like splitting hairs, since we _also_ provide a convenience interface for creating `ProtocolInfo` (and hence `BlockForging`) values. But it'll pay off to keep the separable concern separated. EG The abstract Consensus layer interface works only at the top-level `blk` type, which meant we would have needed a representation of overrides for `HardForkBlock`. The `ProtocolInfo` convenience interface, however, is not abstract, and so it can pass the right overrides to the right atomic forge functions as its building up the inputs (eg `NP` values) for the abstract interface. So now the `HardForkBlock` logic need not include anything at all about overrides (it simply knows how to combine lists of `BlockForging`, which now handles everything appropriately). Co-authored-by: Pawel Szulc <paul.szulc@gmail.com>

This avoids confusion with `maxCapacity` from the `TxLimits` type class. enter the commit message for your changes. Lines starting

The previous commit was a renamed that enabled simpler local names; no longer need to avoid shadowing.

This is important because it is an accumulator.

This has a few small benefits * define's TxMeasure `leq` via the pointwise minimum * removes the redundancy of specifying the `pointwiseMin` method per ledger instead of per 'Measure' * avoids the need for type applications, which let's us re-use the <= operator and two large benefits * creates an opportunity for more use of deriving * the top-bounded meet-semilattice foundation means that `noOverrides` can be implemented as `top`; we don't need it to be a function from the (dynamic!) ledger limits anymore. We had previously decided on a function because it made it trivial to override only parts of the ledger-derived capacity. However, we simplified the override representation (no more `NP`) and a interface that uses the static `top` instead of the dynamic "current ledger capacity" is more appropriate for a interface used for static configuration. In other words, the function space was "too big" for the following reasons. * in practice, these values are parsed from a file, and I doubt the Node Team wants to parse functions anytime soon * we do not anticipate the node operator wanting to vary their override depending on the ledger capacity The 'Measure' class's lattice semantics let us remove the function space without losing any convenience. Co-authored-by: Pawel Szulc <paul.szulc@gmail.com>

nfrisby · 2021-07-27T02:36:15Z

Thank you @EncodePanda.

I rebased just now; automatic, no conflicts 👍

nfrisby · 2021-07-27T02:36:20Z

bors r+

iohk-bors · 2021-07-27T03:16:56Z

Build succeeded:

nfrisby added the consensus issues related to ouroboros-consensus label Jul 12, 2021

nfrisby requested a review from jasagredo July 12, 2021 06:06

nfrisby force-pushed the nfrisby/issue-3235-expose-txlimits-override branch 3 times, most recently from 608b566 to 946a93c Compare July 12, 2021 20:08

nfrisby marked this pull request as ready for review July 12, 2021 20:09

nfrisby marked this pull request as draft July 12, 2021 20:14

nfrisby force-pushed the nfrisby/issue-3235-expose-txlimits-override branch from 946a93c to b5ec9cd Compare July 13, 2021 16:48

nfrisby added a commit to IntersectMBO/cardano-prelude that referenced this pull request Jul 14, 2021

Add DerivingVia module with a few starter definitions

2a0cef2

These current definitions will be used to simplify eg PR IntersectMBO/ouroboros-network#3261

jasagredo previously approved these changes Jul 14, 2021

View reviewed changes

ouroboros-consensus/src/Ouroboros/Consensus/Mempool/TxLimits.hs Outdated Show resolved Hide resolved

jasagredo reviewed Jul 14, 2021

View reviewed changes

scripts/buildkite/check-stylish.sh Show resolved Hide resolved

nfrisby force-pushed the nfrisby/issue-3235-expose-txlimits-override branch from b5ec9cd to c29cf3b Compare July 19, 2021 05:38

nfrisby requested a review from EncodePanda July 19, 2021 05:39

nfrisby force-pushed the nfrisby/issue-3235-expose-txlimits-override branch 2 times, most recently from 788ee0f to 8cecb3c Compare July 21, 2021 23:48

nfrisby commented Jul 22, 2021

View reviewed changes

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Mempool.hs Outdated Show resolved Hide resolved

nfrisby marked this pull request as ready for review July 22, 2021 00:15

nfrisby requested review from bolt12, coot and karknu as code owners July 22, 2021 00:15

nfrisby removed request for coot, karknu and bolt12 July 22, 2021 00:16

nfrisby force-pushed the nfrisby/issue-3235-expose-txlimits-override branch 2 times, most recently from 5b49d04 to e595200 Compare July 22, 2021 00:38

nfrisby force-pushed the nfrisby/issue-3235-expose-txlimits-override branch from e595200 to 7f58409 Compare July 22, 2021 00:39

nfrisby commented Jul 22, 2021

View reviewed changes

ouroboros-consensus/src/Ouroboros/Consensus/Mempool/TxLimits.hs Outdated Show resolved Hide resolved

nfrisby commented Jul 22, 2021

View reviewed changes

ouroboros-consensus/src/Ouroboros/Consensus/Mempool/TxLimits.hs Outdated Show resolved Hide resolved

nfrisby force-pushed the nfrisby/issue-3235-expose-txlimits-override branch from 7f58409 to 2ef5ba7 Compare July 23, 2021 07:23

nfrisby force-pushed the nfrisby/issue-3235-expose-txlimits-override branch from 2ef5ba7 to 5f4a264 Compare July 23, 2021 17:39

EncodePanda approved these changes Jul 26, 2021

View reviewed changes

ouroboros-consensus/src/Ouroboros/Consensus/Block/Forging.hs Outdated Show resolved Hide resolved

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Mempool.hs Outdated Show resolved Hide resolved

nfrisby and others added 8 commits July 26, 2021 19:35

consensus: rename maxTxCapacity to txsMaxBytes

5c86065

This avoids confusion with `maxCapacity` from the `TxLimits` type class. enter the commit message for your changes. Lines starting

consensus: clean up local names

23b5684

The previous commit was a renamed that enabled simpler local names; no longer need to avoid shadowing.

consensus: define TxLimits.lessEq via pointwiseMin

9321471

shelley: make AlonzoMeasure strict in its fields

d4be038

This is important because it is an accumulator.

check-stylish.sh: skip .TxLimits to avoid crash

4ea6899

consensus: remove stale comment

af456b6

nfrisby force-pushed the nfrisby/issue-3235-expose-txlimits-override branch from 5f4a264 to af456b6 Compare July 27, 2021 02:35

nfrisby mentioned this pull request Jul 27, 2021

Change Overrides to be just a function from Measure blk to Measure blk #3260

Closed

iohk-bors bot merged commit 999f250 into master Jul 27, 2021

iohk-bors bot deleted the nfrisby/issue-3235-expose-txlimits-override branch July 27, 2021 03:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move override support for max tx capacities into the ProtocolInfo constructors #3261

Move override support for max tx capacities into the ProtocolInfo constructors #3261

nfrisby commented Jul 12, 2021 •

edited

Loading

nfrisby commented Jul 12, 2021 •

edited

Loading

nfrisby commented Jul 13, 2021 •

edited

Loading

nfrisby commented Jul 13, 2021 •

edited

Loading

jasagredo left a comment

nfrisby commented Jul 19, 2021

nfrisby commented Jul 23, 2021 •

edited

Loading

EncodePanda left a comment

nfrisby commented Jul 27, 2021

nfrisby commented Jul 27, 2021

iohk-bors bot commented Jul 27, 2021

Move override support for max tx capacities into the ProtocolInfo constructors #3261

Move override support for max tx capacities into the ProtocolInfo constructors #3261

Conversation

nfrisby commented Jul 12, 2021 • edited Loading

nfrisby commented Jul 12, 2021 • edited Loading

nfrisby commented Jul 13, 2021 • edited Loading

nfrisby commented Jul 13, 2021 • edited Loading

jasagredo left a comment

Choose a reason for hiding this comment

nfrisby commented Jul 19, 2021

nfrisby commented Jul 23, 2021 • edited Loading

EncodePanda left a comment

Choose a reason for hiding this comment

nfrisby commented Jul 27, 2021

nfrisby commented Jul 27, 2021

iohk-bors bot commented Jul 27, 2021

nfrisby commented Jul 12, 2021 •

edited

Loading

nfrisby commented Jul 12, 2021 •

edited

Loading

nfrisby commented Jul 13, 2021 •

edited

Loading

nfrisby commented Jul 13, 2021 •

edited

Loading

nfrisby commented Jul 23, 2021 •

edited

Loading