Fix Praos obsolete node check #3891

JaredCorduan · 2022-07-12T12:31:10Z

Description

The obsolete node check in the Praos ledger protocol is suppose to compare the current major protocol version (from the protocol parameters in the ledger state) with the global "max major protocol version" value. Currently, however, it is checking that the global max major value is not bigger than the protocol version listed in the block header (which is not supposed to have any semantic meaning).

This PR fixes the check to perform the same logic done by the TPraos ledger protocol, ie the logic in the ledger function chainChecks. To do this, Views.LedgerView now contains the current protocol parameters.

The consequence of the incorrect obsolete node check is that nodes will not properly halt after a hard fork when they do not have the required software for the hard fork.

Checklist

Branch
- Commit sequence broadly makes sense
- Commits have useful messages
- New tests are added if needed and existing tests are updated
- If this branch changes Consensus and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
- If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
- If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.
Pull Request
- Self-reviewed the diff
- Useful pull request description at least containing the following information:
  - What does this PR change?
  - Why these changes were needed?
  - How does this affect downstream repositories and/or end-users?
  - Which ticket does this PR close (if any)? If it does, is it linked?
- Reviewer requested

nfrisby

Thanks, Jared! Nice PR.

I did make one significant comment.

ouroboros-consensus-protocol/src/Ouroboros/Consensus/Protocol/Praos/Views.hs

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Protocol/Praos.hs

nfrisby

Looks good to me, thanks Jared 👍

JaredCorduan · 2022-07-13T00:30:35Z

bors merge

3891: Fix Praos obsolete node check r=JaredCorduan a=JaredCorduan # Description The obsolete node check in the `Praos` ledger protocol is suppose to compare the current major protocol version (from the protocol parameters in the ledger state) with the global "max major protocol version" value. Currently, however, it is checking that the global max major value is not bigger than the protocol version listed in the block header (which is not supposed to have any semantic meaning). This PR fixes the check to perform the same logic done by the `TPraos` ledger protocol, ie the logic in the ledger function `chainChecks`. To do this, `Views.LedgerView` now contains the current protocol parameters. The consequence of the incorrect obsolete node check is that nodes will not properly halt after a hard fork when they do not have the required software for the hard fork. Co-authored-by: Jared Corduan <[email protected]>

iohk-bors · 2022-07-13T02:40:40Z

Timed out.

JaredCorduan · 2022-07-13T12:20:39Z

bors r+

3891: Fix Praos obsolete node check r=JaredCorduan a=JaredCorduan # Description The obsolete node check in the `Praos` ledger protocol is suppose to compare the current major protocol version (from the protocol parameters in the ledger state) with the global "max major protocol version" value. Currently, however, it is checking that the global max major value is not bigger than the protocol version listed in the block header (which is not supposed to have any semantic meaning). This PR fixes the check to perform the same logic done by the `TPraos` ledger protocol, ie the logic in the ledger function `chainChecks`. To do this, `Views.LedgerView` now contains the current protocol parameters. The consequence of the incorrect obsolete node check is that nodes will not properly halt after a hard fork when they do not have the required software for the hard fork. Co-authored-by: Jared Corduan <[email protected]>

iohk-bors · 2022-07-13T12:23:31Z

Build failed:

build (8.10.7, windows-latest)

3896: Fix chocolatey error in GH Actions Windows CI r=amesgen a=amesgen # Description Like #3075, in order to fix the current CI issue, see e.g. #3891 (comment) Co-authored-by: amesgen <[email protected]>

amesgen · 2022-07-14T15:31:43Z

bors retry

3891: Fix Praos obsolete node check r=JaredCorduan a=JaredCorduan # Description The obsolete node check in the `Praos` ledger protocol is suppose to compare the current major protocol version (from the protocol parameters in the ledger state) with the global "max major protocol version" value. Currently, however, it is checking that the global max major value is not bigger than the protocol version listed in the block header (which is not supposed to have any semantic meaning). This PR fixes the check to perform the same logic done by the `TPraos` ledger protocol, ie the logic in the ledger function `chainChecks`. To do this, `Views.LedgerView` now contains the current protocol parameters. The consequence of the incorrect obsolete node check is that nodes will not properly halt after a hard fork when they do not have the required software for the hard fork. Co-authored-by: Jared Corduan <[email protected]>

iohk-bors · 2022-07-14T15:33:40Z

Build failed:

build (8.10.7, windows-latest)

amesgen · 2022-07-14T15:48:34Z

There is a rework of the GH Actions Windows CI (#3901) just around the corner, which will hopefully resolve this 🙏

The obsolete node check in the Praos ledger protocol is suppose to compare the current major protocol version (from the protocol parameters in the ledger state) with the global "max major protocol version" value. Currently, however, it is checking that the global max major value is not bigger than the protocol version listed in the block header (which is not supposed to have any semantic meaning). This PR fixes the check to perform the same logic done by the TPraos ledger protocol, ie the logic in the ledger function chainChecks. To do this, `Views.LedgerView` now contains the current protocol parameters.

amesgen

This should be good to go now, the necessary CI improvements have been merged.

nfrisby · 2022-07-20T15:58:42Z

bors retry

iohk-bors · 2022-07-20T17:13:42Z

Build succeeded:

3896: Fix chocolatey error in GH Actions Windows CI r=amesgen a=amesgen # Description Like #3075, in order to fix the current CI issue, see e.g. #3891 (comment) Co-authored-by: amesgen <[email protected]>

nfrisby reviewed Jul 12, 2022

View reviewed changes

ouroboros-consensus-protocol/src/Ouroboros/Consensus/Protocol/Praos/Views.hs Outdated Show resolved Hide resolved

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Protocol/Praos.hs Show resolved Hide resolved

JaredCorduan marked this pull request as ready for review July 12, 2022 14:54

JaredCorduan requested review from dnadales, coot and bolt12 as code owners July 12, 2022 14:54

JaredCorduan force-pushed the jc/fix-praos-obsolete-node-check branch from c8c03eb to 021c35a Compare July 12, 2022 16:48

JaredCorduan mentioned this pull request Jul 12, 2022

`unwrapEnvelopeErr = ObsoleteNode 7 6' error on the Alonzo/Babbage boundary IntersectMBO/cardano-node#4173

Closed

nfrisby approved these changes Jul 12, 2022

View reviewed changes

amesgen mentioned this pull request Jul 13, 2022

Fix chocolatey error in GH Actions Windows CI #3896

Merged

9 tasks

coot mentioned this pull request Jul 18, 2022

Changes for single relay P2P release. IntersectMBO/cardano-node#4191

Closed

3 tasks

lehins force-pushed the jc/fix-praos-obsolete-node-check branch from 021c35a to d75ca17 Compare July 20, 2022 11:47

lehins mentioned this pull request Jul 20, 2022

Update node to use ouroboros-network with obsolete node fix IntersectMBO/cardano-node#4202

Closed

amesgen reviewed Jul 20, 2022

View reviewed changes

iohk-bors bot merged commit 513a4fe into master Jul 20, 2022

iohk-bors bot deleted the jc/fix-praos-obsolete-node-check branch July 20, 2022 17:13

CarlosLopezDeLara mentioned this pull request Jul 23, 2022

1.35.2 SCOPE IntersectMBO/cardano-node#4225

Closed

3 tasks

jvieiro mentioned this pull request Aug 1, 2022

chore: docker-compose cardano-node 1.35.2 & cardano-db-sync 13.0.2 cardano-foundation/cardano-rosetta#491

Closed

nfrisby mentioned this pull request Aug 27, 2023

Stop checking maxMajorPV in the header envelopeCheck IntersectMBO/ouroboros-consensus#325

Open

nfrisby mentioned this pull request Aug 28, 2023

Simplify the protVer arguments IntersectMBO/ouroboros-consensus#324

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Praos obsolete node check #3891

Fix Praos obsolete node check #3891

JaredCorduan commented Jul 12, 2022 •

edited by nfrisby

Loading

nfrisby left a comment

nfrisby left a comment

JaredCorduan commented Jul 13, 2022

iohk-bors bot commented Jul 13, 2022

JaredCorduan commented Jul 13, 2022

iohk-bors bot commented Jul 13, 2022

amesgen commented Jul 14, 2022

iohk-bors bot commented Jul 14, 2022

amesgen commented Jul 14, 2022 •

edited

Loading

amesgen left a comment

nfrisby commented Jul 20, 2022

iohk-bors bot commented Jul 20, 2022

Fix Praos obsolete node check #3891

Fix Praos obsolete node check #3891

Conversation

JaredCorduan commented Jul 12, 2022 • edited by nfrisby Loading

Description

Checklist

nfrisby left a comment

Choose a reason for hiding this comment

nfrisby left a comment

Choose a reason for hiding this comment

JaredCorduan commented Jul 13, 2022

iohk-bors bot commented Jul 13, 2022

JaredCorduan commented Jul 13, 2022

iohk-bors bot commented Jul 13, 2022

amesgen commented Jul 14, 2022

iohk-bors bot commented Jul 14, 2022

amesgen commented Jul 14, 2022 • edited Loading

amesgen left a comment

Choose a reason for hiding this comment

nfrisby commented Jul 20, 2022

iohk-bors bot commented Jul 20, 2022

JaredCorduan commented Jul 12, 2022 •

edited by nfrisby

Loading

amesgen commented Jul 14, 2022 •

edited

Loading