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

ChainSync client: allow to prevent historical MsgRollBackward/MsgAwaitReply #1186

Merged
merged 4 commits into from
Sep 5, 2024

Conversation

amesgen
Copy link
Member

@amesgen amesgen commented Jul 22, 2024

This PR enables the ChainSync client to disconnect from peers who send historical MsgRollBackward and MsgAwaitReply messages, ie rollbacks that no honest caught-up peer would send as the corresponding blocks are already immutable for them, and MsgAwaitReplys that would indicate a chain whose tip is way behind the wall-clock.

This is relevant for the ChainSync Jumping (CSJ) optimization (part of Ouroboros Genesis): CSJ handles MsgAwaitReply and rollbacks to before a point that a peer previously acknowledged by "disengaging" that peer, ie running full ChainSync with them. This is the right thing to do when we are almost caught-up; however, during syncing the historical chain, this causes extra network load (albeit symmetrical) and CPU load.

We define a MsgRollBackward/MsgAwaitReply to be historical if the Genesis State Machine has not yet concluded that we are caught up and the wall-clock time of the slot of the oldest rewound header (if any) or the previous tip of the candidate fragment, respectively, is older than a configuration value, the historicity cutoff. In this PR, we set it to the duration of one mainnet stability window plus one hour as extra margin (so 36 h + 1 h = 37 h). Future work might include getting this duration out of the HFC.

We also modify the existing ChainSync client test to test for false positives; false negatives can be tested by manually modifying the code (such as subtracting a constant from historicityCutoff, or introducing bugs in the actual implementation); we defer a standalone test for false negatives (similar to #526).

The PR is intended to be reviewed commit by commit.

@amesgen amesgen added the Genesis PRs related to Genesis testing and implementation label Jul 22, 2024
@amesgen amesgen force-pushed the amesgen/historical-rollbacks branch from f6f07b6 to 843011f Compare July 22, 2024 15:40
@amesgen amesgen force-pushed the amesgen/historical-rollbacks branch from ace7071 to 0e1d128 Compare July 25, 2024 17:21
Comment on lines +651 to +658
, -- Here, we make use of the fact that the blocks generated for this
-- test have dense slot numbers (ie there are no empty slots).
let oldestRewound =
withOrigin firstSlot succ $ pointSlot rollbackPoint
Copy link
Member Author

Choose a reason for hiding this comment

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

A future test for false negatives (eg one that ensures that running with maxRollbackAge - 1 sometimes fails) would catch if the generator changed in the future; but I think this is fine for now.

@amesgen amesgen force-pushed the amesgen/historical-rollbacks branch 4 times, most recently from 97ad8b8 to db38bf0 Compare July 25, 2024 17:57
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for switching approaches.

@amesgen amesgen force-pushed the amesgen/historical-rollbacks branch from db38bf0 to 609b4ec Compare August 8, 2024 14:28
@amesgen amesgen changed the title ChainSync client: allow to prevent historical rollbacks ChainSync client: allow to prevent historical MsgRollBackward/MsgAwaitReply Aug 8, 2024
@amesgen amesgen marked this pull request as ready for review August 8, 2024 15:42
-- Duration in seconds of one Cardano mainnet Shelley stability window
-- (3k/f slots times one second per slot) plus one extra hour as a
-- safety margin.
, gcHistoricityCutoff = Just $ HistoricityCutoff $ 3 * 2160 * 20 + 3600
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this too Cardano specific? And somewhat related, is it ok to have these constants hardcoded here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is specific to Cardano mainnet; like many other constants in this area (e.g. the timeouts, protocol-level size limits, the GSM MaxCaughtUpAge).

This is in preparation for allowing to prevent historical rollbacks.
This is in preparation for allowing to prevent historical rollbacks.
Concretely, `MsgRollBackward` and `MsgAwaitReply` beyond a certain age
We only test for false positives, not for false negatives.
@amesgen amesgen added this pull request to the merge queue Sep 5, 2024
Merged via the queue into main with commit fb2e039 Sep 5, 2024
15 checks passed
@amesgen amesgen deleted the amesgen/historical-rollbacks branch September 5, 2024 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Genesis PRs related to Genesis testing and implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants