Skip to content

Conversation

@ana-pantilie
Copy link

Needs #6327 #6290

Description

Add your description here, if it fixes a particular issue please provide a
link
to the issue.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. These may include:
    • golden tests
    • property tests
    • roundtrip tests
    • integration tests
      See Runnings tests for more details
  • Any changes are noted in the CHANGELOG.md for affected package
  • The version bounds in .cabal files are updated
  • CI passes. See note on CI. The following CI checks are required:
    • Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
    • Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
    • Code builds on Linux, MacOS and Windows for ghc-9.6 and ghc-9.12
  • Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.

@carbolymer carbolymer force-pushed the ana/10.6-final-integration-mix branch from 90edc7b to 0ecd14d Compare October 24, 2025 11:44
@carbolymer carbolymer force-pushed the ana/10.6-final-integration-mix branch from 0ecd14d to 706faab Compare October 24, 2025 11:51
@ana-pantilie ana-pantilie marked this pull request as ready for review October 24, 2025 15:45
@ana-pantilie ana-pantilie requested review from a team as code owners October 24, 2025 15:45
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM with respect to changes that concern my team.

-- Enable peer to peer discovery
, ("EnableP2P", Aeson.Bool False)
-- Enable P2P, non-P2P is gone
, ("EnableP2P", Aeson.Bool True)
Copy link
Contributor

Choose a reason for hiding this comment

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

If non-P2P is gone why bother keeping the "EnableP2P" configuration key?

Copy link
Contributor

@carbolymer carbolymer Oct 30, 2025

Choose a reason for hiding this comment

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

Removed.

(pledgerSAddr, _rewards, _poolId) <- H.headM $ mergeDelegsAndRewards delegsAndRewards

-- Pledger and owner are and can be the same
Text.unpack (serialiseAddress pledgerSAddr) === poolownerstakeaddr
Copy link
Contributor

Choose a reason for hiding this comment

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

Why has this check been removed?

Copy link
Contributor

@carbolymer carbolymer Oct 30, 2025

Choose a reason for hiding this comment

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

Because it does not make sense. You have multiple stake addresses in the list in the arbitrary order and it fails randomly. The check that poolownerstakeaddr is in the list is already done in checkStakeKeyRegistered.

H.renameFile (tempAbsPath' </> "shelley/genesis.json") (tempAbsPath' </> defaultGenesisFilepath ShelleyEra)
H.renameFile (tempAbsPath' </> "shelley/genesis.alonzo.json") (tempAbsPath' </> defaultGenesisFilepath AlonzoEra)
H.renameFile (tempAbsPath' </> "shelley/genesis.conway.json") (tempAbsPath' </> defaultGenesisFilepath ConwayEra)
-- TODO: once 'cardano-cli latest genesis create' supports dijkstra, make this a copy instead of writing a default
Copy link
Contributor

Choose a reason for hiding this comment

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

@carbolymer can you turn this into an issue so we can track it.

Copy link
Contributor

@carbolymer carbolymer Oct 30, 2025

Choose a reason for hiding this comment

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

Copy link
Contributor

@jutaro jutaro left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution. I’ve reviewed the changes related to the tracing system and have some important observations:

Incomplete removal of tracing constructors
According to the network changelog, the following were removed:

TraceLedgerPeersResult

TraceLedgerPeersFailure
However, this removal appears to be incomplete. Some references are still present in the codebase, leading to inconsistency and potential confusion during tracing. I will prepare a follow-up commit tomorrow to fully remove the remaining instances and ensure consistency.

TraceChurnMode still present
The changelog also states that:

TraceChurnMode was removed from TracePeerSelection
However, TraceChurnMode still appears in the node codebase—possibly used in a different context now. Before proceeding, I believe the network team should verify that is still required and if the codebase is fully adopted to this change. Otherwise, it may result in dead code or misleading trace output.

Additional tracing inconsistencies
While reviewing this PR, I found several smaller issues and inconsistencies in the trace definitions that should be addressed for correctness and maintainability. I will include these fixes in a follow-up commit tomorrow.

Api.AlonzoEraOnwardsBabbage -> renderAlonzoPlutusPurpose
Api.AlonzoEraOnwardsConway -> renderConwayPlutusPurpose
-- TODO: fix
Api.AlonzoEraOnwardsDijkstra -> undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Still an undefined in the code

@jutaro jutaro force-pushed the ana/10.6-final-integration-mix branch from ab91132 to 0eaa1e7 Compare October 28, 2025 17:31
Copy link
Contributor

@jutaro jutaro left a comment

Choose a reason for hiding this comment

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

All issues I found have been addressed now

@carbolymer carbolymer force-pushed the ana/10.6-final-integration-mix branch from f94636d to 9a132bb Compare October 30, 2025 12:28
@mgmeier mgmeier self-requested a review October 30, 2025 13:04
@mgmeier mgmeier force-pushed the ana/10.6-final-integration-mix branch from e274262 to 9a132bb Compare October 31, 2025 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.