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

Added a new query for a snapshot of big ledger peers #521

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

crocodile-dentist
Copy link
Contributor

@crocodile-dentist crocodile-dentist commented Apr 18, 2024

This change introduces a new query tag and handler to support retrieval of a snapshot of big ledger peers from the current tip of a node.

Changelog

- description: |
    Added `GetBigLedgerPeerSnapshot` block query
# uncomment types applicable to the change:
  type:
  - feature        # introduces a new feature
  - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - improvement    # QoL changes e.g. refactoring
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

A snapshot of big ledger peers is very useful when bootstrapping a node from scratch in the upcoming Genesis consensus mode. Since only some, or none, of these peers are available to a node from its own ledger when syncing up, a snapshot is a means of providing these relays to the diffusion layer to ensure that a node ends up on a good chain.

Checklist

  • [x ] Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

Good work, thanks. Few comments to make sure that this PR adheres to the conventions used here.

There are build failures too.

cardano-api/internal/Cardano/Api/Query.hs Outdated Show resolved Hide resolved
Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

Actually, not blocking changes if this query is for eras >= Shelley.

@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/query-ledger-peer-snapshot branch from e50a5e7 to 12e01d7 Compare April 22, 2024 09:19
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/query-ledger-peer-snapshot branch from fef7482 to 46d8908 Compare June 7, 2024 10:44
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/query-ledger-peer-snapshot branch 2 times, most recently from 1cb28c9 to bd4afe0 Compare June 18, 2024 09:26
@palas
Copy link
Contributor

palas commented Jul 5, 2024

FYI: Because we now enforce the use of cabal-gild to format cabal files and, in order to reduce the amount of hassle, I have cherry-picked the appropriate commit into your PR. This will make the cabal-gild CI check pass for the PR. The commit should be discarded automatically as soon as you either rebase or merge the PR (since it is already in the main branch). Feel free to discard it if you want, but that will make the new required action to fail until you rebase.

@palas palas requested a review from a team as a code owner July 5, 2024 18:16
@palas
Copy link
Contributor

palas commented Jul 6, 2024

FYI: I have rebased your branch because we have done changes to the formatting. I have made a copy of the unrebased branch here: backup/mwojtowicz/query-ledger-peer-snapshot

@palas palas force-pushed the mwojtowicz/query-ledger-peer-snapshot branch 3 times, most recently from e799c03 to 84398c4 Compare July 11, 2024 16:35
@carbolymer carbolymer marked this pull request as draft July 18, 2024 18:04
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/query-ledger-peer-snapshot branch 3 times, most recently from 2fcd02e to c948d05 Compare August 5, 2024 16:31
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/query-ledger-peer-snapshot branch from c948d05 to 235849b Compare August 8, 2024 13:45
Copy link

This PR is stale because it has been open 45 days with no activity.

@github-actions github-actions bot added the Stale label Sep 23, 2024
@github-actions github-actions bot removed the Stale label Nov 1, 2024
@coot coot changed the title Adds new query for a snapshot of big ledger peers Added a new query for a snapshot of big ledger peers Nov 8, 2024
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/query-ledger-peer-snapshot branch 3 times, most recently from 6b6eb32 to 708c91b Compare November 13, 2024 09:14
@crocodile-dentist crocodile-dentist marked this pull request as ready for review November 13, 2024 12:53
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/query-ledger-peer-snapshot branch 2 times, most recently from 9f97741 to 64bdc19 Compare November 13, 2024 13:47
@carbolymer carbolymer self-requested a review November 14, 2024 12:42
Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

LGTM. Can you test e2e (with cardano-cli changes) if the query works, before merging?

@@ -52,3 +52,14 @@ write-ghc-environment-files: always
constraints:
Cabal < 3.14,
cardano-ledger-shelley ^>= 1.14.1

source-repository-package
Copy link
Contributor

Choose a reason for hiding this comment

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

SRP needs to be removed before merge.

@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/query-ledger-peer-snapshot branch 3 times, most recently from 640fa39 to 24a15fd Compare November 15, 2024 09:33
coot and others added 2 commits November 22, 2024 11:55
This change introduces a new query tag and handler
to support retrieval of a snapshot of big ledger peers
from the current tip of a node.
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/query-ledger-peer-snapshot branch from 24a15fd to a8ee0d7 Compare November 25, 2024 09:18
@crocodile-dentist crocodile-dentist force-pushed the mwojtowicz/query-ledger-peer-snapshot branch from a8ee0d7 to ed59e98 Compare November 25, 2024 09:21
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.

4 participants