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 BlockNo to AnchoredFragment #1589

Merged
merged 1 commit into from Feb 6, 2020
Merged

Conversation

edsko
Copy link
Contributor

@edsko edsko commented Feb 5, 2020

No description provided.

@edsko edsko marked this pull request as ready for review February 6, 2020 09:21
@mrBliss mrBliss self-requested a review February 6, 2020 10:48
Copy link
Contributor

@mrBliss mrBliss left a comment

Choose a reason for hiding this comment

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

I trust you to fix everything before merging 😄.

@edsko
Copy link
Contributor Author

edsko commented Feb 6, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request Feb 6, 2020
1589: Add `BlockNo` to `AnchoredFragment`  r=edsko a=edsko



Co-authored-by: Edsko de Vries <edsko@well-typed.com>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 6, 2020

@iohk-bors iohk-bors bot merged commit 299f638 into master Feb 6, 2020
@iohk-bors iohk-bors bot deleted the edsko/anchor-with-blockno branch February 6, 2020 11:33
@mrBliss mrBliss mentioned this pull request Feb 6, 2020
@mrBliss mrBliss added consensus issues related to ouroboros-consensus networking labels Feb 6, 2020
@nfrisby nfrisby mentioned this pull request Feb 6, 2020
iohk-bors bot added a commit that referenced this pull request Feb 7, 2020
1597: Follow-up on #1589 r=mrBliss a=mrBliss

Fixes #1594 and #1595.

* ChainSyncClient: use WithOrigin BlockNo for MkPipelineDecision

* ChainDB: don't read the header at the ImmutableDB's tip on startup


Co-authored-by: Thomas Winant <thomas@well-typed.com>
iohk-bors bot added a commit that referenced this pull request Mar 2, 2020
1506: Fix cloneBlockchainTime r=nfrisby a=nfrisby

Fixes #1489. Fixes #1524.

Fixing Issue #1489 (let nodes lead when they join) and letting k vary in the
range [2 .. 10] since the dual-ledger tests do that now revealed several
Issues.

Issues in the library, not just the test infrastructure:

  * Issue #1505 -- `removeTxs` cannot use the fast path when validating after
    removing or else we might have dangling tx inputs references. Was fixed
    by #1565.

  * Issue #1511, bullet 1 (closed) -- The `Empty` cases in
    `prevPointAndBlockNo` were wrong. Recent PRs have addressed this: #1544 and
    #1589.

  * Issue #1543 (closed) -- A bracket in `registeredStream` was spoiled by an
    interruptible operation. Thomas' PR re-designed the vulnerability away. I
    think this is unrelated to the other changes; it was lurking and happened
    to pop up here just because I've been running hundreds of thousands of
    tests.

Issues only in the test infrastructure:

  * Issue #1489: let nodes lead when they join. This bug slipped in recently,
    when I added cloning of `BlockchainTime`s as part of the
    restarting/rekeying loop in the test infrastructure.

  * _PBFT reference simulator_. (Was masked by 1489.) Model competing 1-block
    chains in Ref.PBFT and use its results where applicable instead of the
    .Expectations module. Check that the PBFT threadnet and Ref.PBFT.simulate
    results agree on `Ref.Nominal` slots.

    The Ref.PBFT module had been making assumptions that were accurate given
    the guards in RealPBFT generators, given the `k >= n` regime. But outside
    of that regime, when the security parameter exceeds the node count, it
    wasn't enough. Also, it couldn't be compared against the PBFT threadnet
    because of those assumptions.

  * _PBFT reference simulator_. (Cascade of above.) Add
    `definitelyEnoughBlocks` to confirm the "at least k blocks in 2k slots"
    invariant in `genRealPBFTNodeJoinPlan`. The existing guards in the RealPBFT
    generators are intentionally insufficient by themselves; this way we can
    optimize them to avoid O(n^2) complexity without risking divergence from
    the `suchThat`s.

  * _Origin corner-case_. (Was masked by 1489.) Discard DualPBFT tests that
    forge in slot 0. The current `cardano-ledger-specs` doesn't allow for that.
    My hypothesis is that `cvsLastSlot` would need to be able to represent
    origin.

  * _Dlg cert tx_. Adjust `genNodeRekeys` to respect "If node X rekeys in slot
    S and Y leads slot S+1, then either the topology must connect X and Y
    directly, or Y must join before slot S."

  * _Dlg cert tx_. (Was (statistically?) masked by relatively large k.) Add
    `rekeyOracle` and use it to determine which epoch number to record in the
    dlg cert. The correct value depends on which block the dlg cert tx will end
    up in, not when we first add it to our mempool.

  * _Dlg cert tx_. (Was (statistically?) masked by relatively large k.) Add the
    dlg cert tx to the mempool each time the ledger changes.

Co-authored-by: Nicolas Frisby <nick.frisby@iohk.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants