-
Notifications
You must be signed in to change notification settings - Fork 25
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
Conversation
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs
Outdated
Show resolved
Hide resolved
f6f07b6
to
843011f
Compare
ouroboros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Node.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Node.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Node.hs
Outdated
Show resolved
Hide resolved
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs
Outdated
Show resolved
Hide resolved
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs
Outdated
Show resolved
Hide resolved
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs
Outdated
Show resolved
Hide resolved
...ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client/HistoricalRollbacks.hs
Outdated
Show resolved
Hide resolved
...ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client/HistoricalRollbacks.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/test/consensus-test/Test/Consensus/MiniProtocol/ChainSync/Client.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/test/consensus-test/Test/Consensus/MiniProtocol/ChainSync/Client.hs
Show resolved
Hide resolved
ace7071
to
0e1d128
Compare
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/Query.hs
Outdated
Show resolved
Hide resolved
, -- 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 |
There was a problem hiding this comment.
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.
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs
Outdated
Show resolved
Hide resolved
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs
Outdated
Show resolved
Hide resolved
...ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client/HistoricalRollbacks.hs
Outdated
Show resolved
Hide resolved
97ad8b8
to
db38bf0
Compare
There was a problem hiding this 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.
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/Query.hs
Outdated
Show resolved
Hide resolved
db38bf0
to
609b4ec
Compare
MsgRollBackward
/MsgAwaitReply
...rc/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client/HistoricityCheck.hs
Show resolved
Hide resolved
...oros-consensus-diffusion/changelog.d/20240722_170436_alexander.esgen_historical_rollbacks.md
Outdated
Show resolved
Hide resolved
-- 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
...oros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/NodeKernel.hs
Show resolved
Hide resolved
...rc/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client/HistoricityCheck.hs
Show resolved
Hide resolved
ouroboros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Node.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PeerSimulator/ChainSync.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PeerSimulator/ChainSync.hs
Show resolved
Hide resolved
ouroboros-consensus/changelog.d/20240722_170344_alexander.esgen_historical_rollbacks.md
Outdated
Show resolved
Hide resolved
...rc/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client/HistoricityCheck.hs
Outdated
Show resolved
Hide resolved
...rc/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client/HistoricityCheck.hs
Outdated
Show resolved
Hide resolved
...rc/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client/HistoricityCheck.hs
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/Query.hs
Show resolved
Hide resolved
ouroboros-consensus/test/consensus-test/Test/Consensus/MiniProtocol/ChainSync/Client.hs
Outdated
Show resolved
Hide resolved
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.
609b4ec
to
0be8e0a
Compare
This PR enables the ChainSync client to disconnect from peers who send historical
MsgRollBackward
andMsgAwaitReply
messages, ie rollbacks that no honest caught-up peer would send as the corresponding blocks are already immutable for them, andMsgAwaitReply
s 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 (so36 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.