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.

Sign up for GitHub

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

Jump to bottom

Conway: restrict VRF tiebreaker based on slot distance #1047

Merged
merged 2 commits into from
May 13, 2024
Merged

Conway: restrict VRF tiebreaker based on slot distance #1047

merged 2 commits into from
May 13, 2024

Conversation

amesgen
Copy link
Member

@amesgen amesgen commented Apr 8, 2024 •
edited
Loading

Closes #524

This is a revived version of IntersectMBO/ouroboros-network#3721

Based on top of #1063

@amesgen amesgen self-assigned this Apr 8, 2024
@amesgen amesgen force-pushed the amesgen/remove-ord-selectview branch from b090efb to 879ce60 Compare April 8, 2024 12:08
@amesgen amesgen force-pushed the amesgen/remove-ord-selectview branch from 879ce60 to 193d5ee Compare April 8, 2024 15:36
@amesgen amesgen force-pushed the amesgen/remove-ord-selectview branch from 193d5ee to c18f485 Compare April 9, 2024 13:15
@amesgen amesgen force-pushed the amesgen/duncan-tiebreaker branch 2 times, most recently from 237e0c6 to cb4b755 Compare April 9, 2024 13:42
@amesgen amesgen force-pushed the amesgen/remove-ord-selectview branch from c18f485 to cd035f7 Compare April 12, 2024 13:34
@amesgen amesgen force-pushed the amesgen/remove-ord-selectview branch from cd035f7 to 30c6814 Compare April 16, 2024 15:43
@amesgen amesgen force-pushed the amesgen/remove-ord-selectview branch from 30c6814 to 630c9a9 Compare April 16, 2024 16:30
@amesgen amesgen force-pushed the amesgen/remove-ord-selectview branch from 630c9a9 to cbfbb0f Compare April 16, 2024 17:05
@amesgen amesgen mentioned this pull request Apr 17, 2024
Merged
@amesgen amesgen changed the base branch from amesgen/remove-ord-selectview to amesgen/customize-prefer-candidate April 17, 2024 14:05
@amesgen amesgen force-pushed the amesgen/customize-prefer-candidate branch from d05e0f7 to 2802b4a Compare April 17, 2024 14:06
@amesgen amesgen force-pushed the amesgen/customize-prefer-candidate branch from 2802b4a to 331473c Compare April 23, 2024 12:17
@amesgen amesgen force-pushed the amesgen/customize-prefer-candidate branch from 331473c to 54de587 Compare April 23, 2024 12:22
@amesgen amesgen force-pushed the amesgen/customize-prefer-candidate branch from 54de587 to 5e2893d Compare April 23, 2024 12:50
@amesgen amesgen marked this pull request as ready for review April 23, 2024 13:17
@amesgen amesgen requested a review from a team as a code owner April 23, 2024 13:17
nfrisby
nfrisby reviewed Apr 23, 2024
View reviewed changes
ouroboros-consensus-cardano/src/shelley/Ouroboros/Consensus/Shelley/Ledger/Config.hs Outdated Show resolved Hide resolved
ouroboros-consensus-cardano/src/shelley/Ouroboros/Consensus/Shelley/Node/DiffusionPipelining.hs Outdated Show resolved Hide resolved
...ensus-protocol/src/ouroboros-consensus-protocol/Ouroboros/Consensus/Protocol/Praos/Common.hs Outdated
data VRFTiebreakerFlavor =
-- | Always compare the VRF tiebreakers. This is the behavior of all eras
-- before Conway. Once mainnet has transitioned to Conway, we can remove
-- this option.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a parenthetical "(The honest /historical/ Ouroboros chain cannot rely on tiebreakers to win, so /retroactively/ disabling the tiebreaker won't matter)".

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess once we do that, then... we could remove the type family, as Joris had asked? #1063 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we could, though the "5 slots" would then be "more hardcoded" than ATM.

...ensus-protocol/src/ouroboros-consensus-protocol/Ouroboros/Consensus/Protocol/Praos/Common.hs Outdated
-- number by more than Δ (the maximum message delay from Praos), say
-- @slot(A) + Δ < slot(B)@, the issuer of B should have been able to mint a
-- block with a block number higher than A (eg by minting on top of A).
-- Therefore, we do not want to allow B to win against A by having a better
Copy link
Contributor

Choose a reason for hiding this comment

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

There's also a scenario in which case A was the poorly managed pool. Perhaps B didn't extend A because A was very late sending their block. The tiebreaker still works in that case, since B would (likely) have arrived to the network before A.

Copy link
Contributor

Choose a reason for hiding this comment

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

It occurs to me that today's excessively granular tiebreakers mean that short forks resolve themselves ASAP on mainnet. As of this change, the network instead won't resolve until someone creates a sole longest chain again (ie the actual Praos paper). I wonder if the community needs to be "warned" about short forks becoming "relatively long lasting" after this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's also a scenario in which case A was the poorly managed pool. Perhaps B didn't extend A because A was very late sending their block. The tiebreaker still works in that case, since B would (likely) have arrived to the network before A.

Good point, will expand this to make it clear that both A and B could be the culprit here.

It occurs to me that today's excessively granular tiebreakers mean that short forks resolve themselves ASAP on mainnet. As of this change, the network instead won't resolve until someone creates a sole longest chain again (ie the actual Praos paper). I wonder if the community needs to be "warned" about short forks becoming "relatively long lasting" after this change?

Yeah, that is an advantage of the current "deterministic"1/"consistent"2 tiebreaker. I think it makes sense to mention that in IntersectMBO/ouroboros-network#2913 when the HF approaches 👍

Footnotes

  1. That's how it is called eg in CAD-2545. ↩

  2. That's how it is called in this paper that the researchers referred us to when we talked about this; the high-level summary back then was that you get slightly better settlement bounds with a consistent tiebreaker. The paper only seems to prove a similar asymptotic bound under weaker conditions (Theorem 2 vs Theorem 1); but this is fully consistent that. ↩

...ensus-protocol/src/ouroboros-consensus-protocol/Ouroboros/Consensus/Protocol/Praos/Common.hs Show resolved Hide resolved
...ensus-protocol/src/ouroboros-consensus-protocol/Ouroboros/Consensus/Protocol/Praos/Common.hs Outdated Show resolved Hide resolved
...ensus-protocol/src/ouroboros-consensus-protocol/Ouroboros/Consensus/Protocol/Praos/Common.hs Outdated Show resolved Hide resolved
...ensus-protocol/src/ouroboros-consensus-protocol/Ouroboros/Consensus/Protocol/Praos/Common.hs Outdated Show resolved Hide resolved
...ensus-protocol/src/ouroboros-consensus-protocol/Ouroboros/Consensus/Protocol/Praos/Common.hs Outdated Show resolved Hide resolved
@amesgen amesgen force-pushed the amesgen/customize-prefer-candidate branch from 3a5edba to ea34993 Compare April 25, 2024 09:03
@amesgen amesgen force-pushed the amesgen/customize-prefer-candidate branch from ea34993 to 0796d29 Compare April 25, 2024 09:22
@amesgen amesgen force-pushed the amesgen/customize-prefer-candidate branch from c33c03e to f28b409 Compare May 3, 2024 13:23
github-merge-queue bot pushed a commit that referenced this pull request May 3, 2024
@amesgen
Allow to customize preferCandidate (#1063)
60940ae 
Closes #939

The core idea is to keep a total order (via `Ord`) for `SelectView`, but
allowing to customize the logic of `preferCandidate` (previously, it was
always defined in terms of `Ord`, i.e. `preferCandidate ours cand = cand
> ours`), such that it becomes possible to eg implement the restricted
VRF tiebreaker (#524) this way, which is fundamentally intransitive.

This is in contrast to #1046 (an alternative to this PR), where
`preferCandidate` is not customizable, and we instead live with the fact
the ordering might not be transitive, which techically would require
replacing our calls to `sortBy` with eg a topological sorting algorithm,
or just assume/test that `sortBy` still works "well enough" with
non-transitive orders.

Also see #1047 as a follow-up PR that leverages the new flexibility
offered by this PR.

This PR undoes some of the changes of
IntersectMBO/ouroboros-network#2743 and
IntersectMBO/ouroboros-network#2732 which were
back then merged to increase simplicity/ease of exposition in the
report, but it now turns out that the flexibility they removed is
actually useful for us today.
Base automatically changed from amesgen/customize-prefer-candidate to main May 3, 2024 15:43
@amesgen amesgen force-pushed the amesgen/duncan-tiebreaker branch from df1d571 to a1eecef Compare May 3, 2024 16:45
@amesgen amesgen requested a review from nfrisby May 3, 2024 16:53
@amesgen amesgen mentioned this pull request May 3, 2024
Merged
nfrisby
nfrisby approved these changes May 13, 2024
View reviewed changes
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

Very nicely done.

...ensus-protocol/src/ouroboros-consensus-protocol/Ouroboros/Consensus/Protocol/Praos/Common.hs Show resolved Hide resolved
ouroboros-consensus-protocol/test/protocol-test/Test/Consensus/Protocol/Praos/SelectView.hs Show resolved Hide resolved
@amesgen amesgen force-pushed the amesgen/duncan-tiebreaker branch from a1eecef to f759bee Compare May 13, 2024 13:41
@amesgen amesgen added this pull request to the merge queue May 13, 2024
@disassembler disassembler removed this pull request from the merge queue due to a manual request May 13, 2024
@amesgen amesgen force-pushed the amesgen/duncan-tiebreaker branch from 235a96b to 55ea208 Compare May 13, 2024 18:24
@disassembler disassembler added this pull request to the merge queue May 13, 2024
Merged via the queue into main with commit 3bd2cbe May 13, 2024
15 checks passed
@disassembler disassembler deleted the amesgen/duncan-tiebreaker branch May 13, 2024 19:13
github-merge-queue bot pushed a commit that referenced this pull request May 13, 2024
@amesgen
Praos chain order: fix issue no-related transitivity edge case (#1086)
62246f8 
Closes #1075

Beware that this PR has a fairly small
$\dfrac{\text{severity}}{\text{subtlety}}$ ratio.

### Current non-transitivity of the chain order related to issue numbers

Before this PR, the `Ord PraosChainSelectView` instance is defined as
the lexicographic-ish[^lexicographic-ish] combination of the following
comparisons in descending order:

 - Chain length, preferring longer chains.
- If the issuer identity is the same, compare by the issue/opcert
number, preferring higher values, otherwise, no preference.
 - VRF tiebreaker, preferring lower values.

To see why it is not transitive, consider the following three
`SelectView`s:

|              | a | b | c |
| ------------ | - | - | - |
| Chain length | l | l | l |
| Issuer       | x | y | x |
| Issue no     | 2 | o | 1 |
| VRF          | 3 | 2 | 1 |

With the current chain order, we have

 - `a < b` and `b < c` due to the VRF tiebreaker, and
- `c < a` due to the issue number tiebreaker (as `a` and `c` have the
same issuer).

So we have have `a < b < c < a < ...`.

Note that due to `VRF a /= VRF c` and `Issuer a == Issuer c`, we must
have `Slot a /= Slot c`, even though `ChainLength a == ChainLength b`.
This is because VRFs are collision-resistant, and are a deterministic
function of the slot, the (cold) issuer identity and the epoch nonce
(which is itself determined by the slot for any given chain).

However, this case is not important for the motivating scenario of the
issue number tiebreaker, namely when an attacker got hold of the hot key
(but not the cold key) of issuer `x`, and the attacked SPO, the owner of
the cold key of `x`, creates a new hot key with an incremented issue
number, where the issue number tiebreaker is now supposed to "establish
precedence"[^precedence]. In this scenario, the attacker minted `c`, and
the attacked SPO minted `a`; this is however unrealistic as due to `Slot
a /= Slot c`, either party could have minted on top of the other block,
superseding the tiebreaker due to having a longer chain.

### Restoring transitivity

The natural fix is hence to require `Slot x == Slot y` in addition to
`Issuer x == Issuer y` as the condition on whether to compare issue
numbers when comparing `SelectView`s `x` and `y`.

In the example above, we then have

 - `a < b` and `b < c` due to the VRF tiebreaker (unchanged), and
- `a < c` also due to the VRF tiebreaker (new), as the issue number
tiebreaker is not armed.

Note that as already mentioned above, the condition `Slot x == Slot y &&
Issuer x == Issuer y` is equivalent to `VRF x == VRF y`. We could
therefore use this condition in this PR, and even remove the issuer from
`PraosChainSelectView`.

As a historical note, a very similar chain order was in place in the
past before the current non-transitivity was accidentally introduced as
a side effect in
IntersectMBO/ouroboros-network#2348.

The approach of this PR is slightly different than (but morally the same
as) the one suggested in #891; I think it is nice that the issue numbers
are still compared "before" the VRFs in this approach as that matches
the high-level intuition.

---

Based on top of #1047

[^lexicographic-ish]: Usually, one only considers the lexicographic
order constructed out of orders that are at least partial. However, the
order "compare opcert numbers when the issuers are identical, otherwise,
consider equal" on pairs of issuer identities and opcert numbers is not
a partial order as it is non-transitive. Still, the same general
principle applies.
[^precedence]: See ["Design Specification for Delegation and Incentives
in
Cardano"](https://github.com/IntersectMBO/cardano-ledger/blob/master/README.md),
Section 3.7.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nfrisby nfrisby nfrisby approved these changes

Assignees

@amesgen amesgen

Labels
None yet
Projects
Consensus Team Backlog
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ignore the VRF tiebreaker when the slots are too far apart
3 participants
@amesgen @nfrisby @disassembler