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

Discussion PR on chain order transitivity #891

Closed
wants to merge 10 commits into from

Conversation

amesgen
Copy link
Member

@amesgen amesgen commented Jan 18, 2024

docs/ChainOrderTransitivity.md Outdated Show resolved Hide resolved
docs/ChainOrderTransitivity.md Outdated Show resolved Hide resolved
docs/ChainOrderTransitivity.md Outdated Show resolved Hide resolved
docs/ChainOrderTransitivity.md Outdated Show resolved Hide resolved
docs/ChainOrderTransitivity.md Outdated Show resolved Hide resolved
docs/ChainOrderTransitivity.md Outdated Show resolved Hide resolved
docs/ChainOrderTransitivity.md Outdated Show resolved Hide resolved
docs/ChainOrderTransitivity.md Outdated Show resolved Hide resolved
docs/ChainOrderTransitivity.md Show resolved Hide resolved
docs/ChainOrderTransitivity.md Outdated Show resolved Hide resolved
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.

Approved. Nice!

docs/ChainOrderTransitivity.md Outdated Show resolved Hide resolved
@nfrisby
Copy link
Contributor

nfrisby commented Jan 19, 2024

I'm currently thinking that this document cannot be significantly improved until we discuss it with other architects/IOG Researchers and decide on a path forward. Specifically, the SlotNo tiebreaker.

Regarding the SlotNo tiebreaker, I think it'd be useful to cite the Tse paper and straightpool's Issue.

@amesgen amesgen self-assigned this Jan 22, 2024
docs/ChainOrderTransitivity.md Show resolved Hide resolved
docs/ChainOrderTransitivity.md Outdated Show resolved Hide resolved
@amesgen
Copy link
Member Author

amesgen commented Apr 17, 2024

The approach in #1063 allows to keep a total (in particular transitive) chain order for eg sorting while still allowing the flexibility to implement the restricted VRF tiebreaker ("Duncan's rule"), which resolves the issue.

@amesgen amesgen closed this Apr 17, 2024
@amesgen amesgen deleted the amesgen/order-discussion branch April 17, 2024 15:34
github-merge-queue bot pushed a commit that referenced this pull request May 13, 2024
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
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants