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

Replace anachronisticLedgerView by ledgerViewForecastAt #1942

Merged
merged 5 commits into from
Apr 14, 2020
Merged

Replace anachronisticLedgerView by ledgerViewForecastAt #1942

merged 5 commits into from
Apr 14, 2020

Conversation

edsko
Copy link
Contributor

@edsko edsko commented Apr 10, 2020

See #1933 for detailed description.

@edsko edsko added the consensus issues related to ouroboros-consensus label Apr 10, 2020
@edsko
Copy link
Contributor Author

edsko commented Apr 10, 2020

I think the intersection point maintained by the chain sync client is not correct, or we are not disconnecting then the intersection with the upstream node is more than k blocks back. Getting test failure:

# cabal run ouroboros-consensus-byron:test:test -- -p 'simple' --quickcheck-replay=314287
(...)
        Byron ledgerViewForecastAt precondition violated: slot At (SlotNo {unSlotNo = 0}) more than 2k slots away from the tip SlotNo {unSlotNo = 14} (minLo = At (SlotNo {unSlotNo = 2}), k = 6)
        CallStack (from HasCallStack):
          error, called at src/Ouroboros/Consensus/Byron/Ledger/Ledger.hs:216:29 in ouroboros-consensus-byron-0.1.0.0-inplace:Ouroboros.Consensus.Byron.Ledger.Ledger
          ledgerViewForecastAt_, called at src/Ouroboros/Consensus/Ledger/SupportsProtocol.hs:72:16 in ouroboros-consensus-0.1.0.0-inplace:Ouroboros.Consensus.Ledger.SupportsProtocol
          ledgerViewForecastAt, called at src/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs:714:19 in ouroboros-consensus-0.1.0.0-inplace:Ouroboros.Consensus.MiniProtocol.ChainSync.Client

@edsko edsko mentioned this pull request Apr 10, 2020
Closed
@edsko edsko force-pushed the fix-1933 branch 2 times, most recently from 6785f16 to 523ae81 Compare April 13, 2020 07:14
@edsko edsko mentioned this pull request Apr 13, 2020
Open
@edsko edsko marked this pull request as ready for review April 13, 2020 07:23
@edsko edsko requested a review from mrBliss April 13, 2020 07:23
@edsko
Copy link
Contributor Author

edsko commented Apr 13, 2020

Comment above is wrong. See IntersectMBO/ouroboros-consensus#763 .

@edsko
Copy link
Contributor Author

edsko commented Apr 13, 2020

Rebased on #1946 as that will be merged first.

@edsko edsko force-pushed the fix-1933 branch 2 times, most recently from 7305a9c to 5415034 Compare April 13, 2020 09:46
@edsko edsko mentioned this pull request Apr 13, 2020
Closed
edsko
edsko commented Apr 14, 2020
View reviewed changes
ouroboros-consensus/src/Ouroboros/Consensus/Mempool/API.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Ledger/SupportsProtocol.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Ledger/SupportsProtocol.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Ledger/SupportsProtocol.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Ledger/SupportsProtocol.hs Outdated Show resolved Hide resolved
ouroboros-consensus-byron/src/Ouroboros/Consensus/Byron/Ledger/Ledger.hs Outdated Show resolved Hide resolved
ouroboros-consensus-byron/src/Ouroboros/Consensus/Byron/Ledger/Ledger.hs Outdated Show resolved Hide resolved
ouroboros-consensus-byron/src/Ouroboros/Consensus/Byron/Ledger/Ledger.hs Show resolved Hide resolved
ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Ledger.hs Outdated Show resolved Hide resolved
ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Ledger.hs Outdated Show resolved Hide resolved
mrBliss
mrBliss approved these changes Apr 14, 2020
View reviewed changes
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.

LGTM. Reviewed over Meet.

@edsko
Copy link
Contributor Author

edsko commented Apr 14, 2020

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Apr 14, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit a9bc5fc into master Apr 14, 2020
@iohk-bors iohk-bors bot deleted the fix-1933 branch April 14, 2020 11:11
@nfrisby nfrisby added byron Required for a Byron mainnet: replace the old core nodes with cardano-node and removed byron Required for a Byron mainnet: replace the old core nodes with cardano-node labels May 8, 2020
@nfrisby nfrisby mentioned this pull request May 8, 2020
Closed
nfrisby added a commit that referenced this pull request May 8, 2020
@nfrisby
JIT EBBs in the ThreadNet tests require ExtLedgerState in block produ…
f2acddc 
…ction

PR #1942 changed blockProduction to take a LedgerState instead of
ExtLedgerState, which suffices for all forging. However, the ThreadNet tests'
current approach to EBBs requires anticipating during block production whether
the produced block will be invalid in anyway. (The node does not emit the EBB
unless the EBB unless it is also emitting a valid block.) And that anticipatory
validation must include protocol-level errors, which requires the full
ExtLedgerState.

It may be possible to instead change the tests to allow for EBBs without
successors, but I'm don't see how so I'm leaving that for future work if we
prioritize it.
@nfrisby nfrisby mentioned this pull request May 8, 2020
Merged
nfrisby added a commit that referenced this pull request May 18, 2020
@nfrisby
JIT EBBs in the ThreadNet tests require ExtLedgerState in block produ…
ad6a8a1 
…ction

PR #1942 changed blockProduction to take a LedgerState instead of
ExtLedgerState, which suffices for all forging. However, the ThreadNet tests'
current approach to EBBs requires anticipating during block production whether
the produced block will be invalid in anyway. (The node does not emit the EBB
unless the EBB unless it is also emitting a valid block.) And that anticipatory
validation must include protocol-level errors, which requires the full
ExtLedgerState.

It may be possible to instead change the tests to allow for EBBs without
successors, but I'm don't see how so I'm leaving that for future work if we
prioritize it.
nfrisby added a commit that referenced this pull request May 22, 2020
@nfrisby
JIT EBBs in the ThreadNet tests require ExtLedgerState in block produ…
0f6f32a 
…ction

PR #1942 changed blockProduction to take a LedgerState instead of
ExtLedgerState, which suffices for all forging. However, the ThreadNet tests'
current approach to EBBs requires anticipating during block production whether
the produced block will be invalid in anyway. (The node does not emit the EBB
unless the EBB unless it is also emitting a valid block.) And that anticipatory
validation must include protocol-level errors, which requires the full
ExtLedgerState.

It may be possible to instead change the tests to allow for EBBs without
successors, but I'm don't see how so I'm leaving that for future work if we
prioritize it.
nfrisby added a commit that referenced this pull request May 25, 2020
@nfrisby
JIT EBBs in the ThreadNet tests require ExtLedgerState in block produ…
9290c8d 
…ction

PR #1942 changed blockProduction to take a LedgerState instead of
ExtLedgerState, which suffices for all forging. However, the ThreadNet tests'
current approach to EBBs requires anticipating during block production whether
the produced block will be invalid in anyway. (The node does not emit the EBB
unless the EBB unless it is also emitting a valid block.) And that anticipatory
validation must include protocol-level errors, which requires the full
ExtLedgerState.

It may be possible to instead change the tests to allow for EBBs without
successors, but I'm don't see how so I'm leaving that for future work if we
prioritize it.
nfrisby added a commit that referenced this pull request May 29, 2020
@nfrisby
JIT EBBs in the ThreadNet tests require ExtLedgerState in block produ…
f39085d 
…ction

PR #1942 changed blockProduction to take a LedgerState instead of
ExtLedgerState, which suffices for all forging. However, the ThreadNet tests'
current approach to EBBs requires anticipating during block production whether
the produced block will be invalid in anyway. (The node does not emit the EBB
unless the EBB unless it is also emitting a valid block.) And that anticipatory
validation must include protocol-level errors, which requires the full
ExtLedgerState.

It may be possible to instead change the tests to allow for EBBs without
successors, but I'm don't see how so I'm leaving that for future work if we
prioritize it.
iohk-bors bot added a commit that referenced this pull request Jun 1, 2020
@iohk-bors @nfrisby
Merge #2048
218126a 
2048: JIT EBBs in the ThreadNet tests require ExtLedgerState in block production r=edsko a=nfrisby

Fixes #2047.

PR #1942 changed blockProduction to take a LedgerState instead of
ExtLedgerState, which suffices for all forging. However, the ThreadNet tests'
current approach to EBBs requires anticipating during block production whether
the produced block will be invalid in anyway. (The node does not emit the EBB
unless the EBB unless it is also emitting a valid block.) And that anticipatory
validation must include protocol-level errors, which requires the full
ExtLedgerState.

It may be possible to instead change the tests to allow for EBBs without
successors, but I'm don't see how, so I currently propose leaving that for future work if we
prioritize it.

Edit: PR 2176 essentially fixed this failure mode _en passant_. This PR now just simplifies the JIT EBB logic accordingly.

Co-authored-by: Nicolas Frisby <[email protected]>
@mrBliss mrBliss mentioned this pull request Jun 30, 2020
Closed
@TimSheard TimSheard mentioned this pull request Oct 27, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrBliss mrBliss mrBliss approved these changes

Assignees
No one assigned
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@edsko @mrBliss @nfrisby