[ADP-3031] Simplify checkpoint pruning, take 3 #3988

HeinrichApfelmus · 2023-06-07T14:37:56Z

Overview

In this pull request, we consolidate and simplify the creation and pruning of checkpoints.

Specifically, we introduce a function extendAndPrune that computes a delta which

adds new checkpoints
prunes the existing checkpoints

based on their block height.

Details

The mechanism for creating checkpoints has changed. Specifically, when synchronizing the chain far away from the tip, at most two checkpoints are kept: genesis and the latest synchronization point. We only keep multiple checkpoints when we are within epochStability of the tip, as we expect rollbacks only then.
The CheckpointPolicy is tested in the existing module Cardano.Wallet.Checkpoints.PolicySpec.

Comments

This task evolved out of ADP-1497. Previous attempts to implement this are
- Simplify checkpoint pruning #3159
- Simplify checkpoint creation and pruning, take 2 #3369

Issue Number

ADP-3031

lib/wallet/src/Cardano/Wallet/Checkpoints.hs

paweljakubas · 2023-06-12T10:51:59Z

lib/wallet/test/unit/Cardano/Wallet/CheckpointsSpec.hs

+        it "actually prunes checkpoints" $
+            property prop_doesPrune
+        it "keeps the tip of the chain" $
+            property prop_keepTip


if I understand well we are always going to keep
(a) genesis
(b) latest tip
(c) some checkpoints ONLY within stability epoch (I mean this range defined by security parameter that limits depth of rollbacks)
What about adding properties checking that :
(a) genesis cp is always there
(b) after pruning we can only have at most genesis (either it is in stability window or not) and checkpoints within the stability window
(c) variate stability window explicitly ((i) all possible cps are within it, (ii) stability window is proper subset of all possible checkpoints)

What about adding properties checking that :

Well, these properties are properties of the specific policy argument, not of the extendAndPrune function. It's enough to test that extendPrune does use the policy, and then rely on the properties of the specific policy to get a specific result. The specific properties are tested here:

https://github.com/input-output-hk/cardano-wallet/blob/0c2382c373f76e808fd3472b78d1ce98c7770618/lib/wallet/test/unit/Cardano/Wallet/Checkpoints/PolicySpec.hs#L52-L66

I can add a test that sparseArithmetic does keep genesis.

I have added a test that sparseArithmetic does keep genesis.

paweljakubas

Nice PR! I like when code cutting > code added :-)
I left some suggestion for additional property tests.

HeinrichApfelmus · 2023-06-12T16:07:57Z

bors merge

3988: [ADP-3031] Simplify checkpoint pruning, take 3 r=HeinrichApfelmus a=HeinrichApfelmus ### Overview In this pull request, we consolidate and simplify the creation and pruning of checkpoints. Specifically, we introduce a function `extendAndPrune` that computes a delta which * adds new checkpoints * prunes the existing checkpoints based on their block height. ### Details * The mechanism for creating checkpoints has changed. Specifically, when synchronizing the chain far away from the `tip`, at most two checkpoints are kept: genesis and the latest synchronization point. We only keep multiple checkpoints when we are within `epochStability` of the tip, as we expect rollbacks only then. * The `CheckpointPolicy` is tested in the existing module `Cardano.Wallet.Checkpoints.PolicySpec`. ### Comments * This task evolved out of ADP-1497. Previous attempts to implement this are * #3159 * #3369 ### Issue Number ADP-3031 Co-authored-by: Heinrich Apfelmus <[email protected]>

iohk-bors · 2023-06-12T16:33:26Z

Build failed:

buildkite/cardano-wallet

HeinrichApfelmus · 2023-06-13T11:18:16Z

It looks like some integration tests have become more flaky with this change. 🤔

TRANS_NEW_JOIN_01a fails in 3/3 runs
BYRON_MIGRATE_02 fails in 1/3 runs
NEW_SHELLEY_TRANSACTIONS fails in 1/3 runs
TRANS_LIST_RANGE_01 fails in 1/3 runs
TRANS_ASSETS_CREATE_02c fails in 1/3 runs

Investigating.

HeinrichApfelmus · 2023-06-14T13:33:21Z

The integration test TRANS_NEW_JOIN_01a contains a few race conditions. For example, it assumed that a transaction that was submitted to the node would immediately be included in the blockchain, but that is not the case.

paweljakubas · 2023-06-16T08:29:23Z

lib/wallet/integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

+        pools <- filter won'tRetire . map getApiT . snd <$>
+            unsafeRequest @[ApiT StakePool] ctx
+                (Link.listStakePools arbitraryStake) Empty
+        let pool1:pool2:_ = map (view #id) pools


good catch and observation!

paweljakubas

LGTM!

HeinrichApfelmus · 2023-06-16T14:07:45Z

bors merge

3988: [ADP-3031] Simplify checkpoint pruning, take 3 r=HeinrichApfelmus a=HeinrichApfelmus ### Overview In this pull request, we consolidate and simplify the creation and pruning of checkpoints. Specifically, we introduce a function `extendAndPrune` that computes a delta which * adds new checkpoints * prunes the existing checkpoints based on their block height. ### Details * The mechanism for creating checkpoints has changed. Specifically, when synchronizing the chain far away from the `tip`, at most two checkpoints are kept: genesis and the latest synchronization point. We only keep multiple checkpoints when we are within `epochStability` of the tip, as we expect rollbacks only then. * The `CheckpointPolicy` is tested in the existing module `Cardano.Wallet.Checkpoints.PolicySpec`. ### Comments * This task evolved out of ADP-1497. Previous attempts to implement this are * #3159 * #3369 ### Issue Number ADP-3031 Co-authored-by: Heinrich Apfelmus <[email protected]>

iohk-bors · 2023-06-16T14:48:49Z

Build failed:

buildkite/cardano-wallet

HeinrichApfelmus · 2023-06-16T15:21:21Z

bors merge

3988: [ADP-3031] Simplify checkpoint pruning, take 3 r=HeinrichApfelmus a=HeinrichApfelmus ### Overview In this pull request, we consolidate and simplify the creation and pruning of checkpoints. Specifically, we introduce a function `extendAndPrune` that computes a delta which * adds new checkpoints * prunes the existing checkpoints based on their block height. ### Details * The mechanism for creating checkpoints has changed. Specifically, when synchronizing the chain far away from the `tip`, at most two checkpoints are kept: genesis and the latest synchronization point. We only keep multiple checkpoints when we are within `epochStability` of the tip, as we expect rollbacks only then. * The `CheckpointPolicy` is tested in the existing module `Cardano.Wallet.Checkpoints.PolicySpec`. ### Comments * This task evolved out of ADP-1497. Previous attempts to implement this are * #3159 * #3369 ### Issue Number ADP-3031 Co-authored-by: Heinrich Apfelmus <[email protected]>

iohk-bors · 2023-06-16T16:09:42Z

Build failed:

buildkite/cardano-wallet

HeinrichApfelmus · 2023-06-16T16:24:28Z

bors merge

3988: [ADP-3031] Simplify checkpoint pruning, take 3 r=HeinrichApfelmus a=HeinrichApfelmus ### Overview In this pull request, we consolidate and simplify the creation and pruning of checkpoints. Specifically, we introduce a function `extendAndPrune` that computes a delta which * adds new checkpoints * prunes the existing checkpoints based on their block height. ### Details * The mechanism for creating checkpoints has changed. Specifically, when synchronizing the chain far away from the `tip`, at most two checkpoints are kept: genesis and the latest synchronization point. We only keep multiple checkpoints when we are within `epochStability` of the tip, as we expect rollbacks only then. * The `CheckpointPolicy` is tested in the existing module `Cardano.Wallet.Checkpoints.PolicySpec`. ### Comments * This task evolved out of ADP-1497. Previous attempts to implement this are * #3159 * #3369 ### Issue Number ADP-3031 Co-authored-by: Heinrich Apfelmus <[email protected]>

iohk-bors · 2023-06-16T16:52:49Z

Build failed:

buildkite/cardano-wallet

HeinrichApfelmus · 2023-06-16T17:05:12Z

bors merge

3988: [ADP-3031] Simplify checkpoint pruning, take 3 r=HeinrichApfelmus a=HeinrichApfelmus ### Overview In this pull request, we consolidate and simplify the creation and pruning of checkpoints. Specifically, we introduce a function `extendAndPrune` that computes a delta which * adds new checkpoints * prunes the existing checkpoints based on their block height. ### Details * The mechanism for creating checkpoints has changed. Specifically, when synchronizing the chain far away from the `tip`, at most two checkpoints are kept: genesis and the latest synchronization point. We only keep multiple checkpoints when we are within `epochStability` of the tip, as we expect rollbacks only then. * The `CheckpointPolicy` is tested in the existing module `Cardano.Wallet.Checkpoints.PolicySpec`. ### Comments * This task evolved out of ADP-1497. Previous attempts to implement this are * #3159 * #3369 ### Issue Number ADP-3031 Co-authored-by: Heinrich Apfelmus <[email protected]>

iohk-bors · 2023-06-16T17:58:15Z

Build failed:

buildkite/cardano-wallet

HeinrichApfelmus mentioned this pull request Jun 7, 2023

Simplify checkpoint creation and pruning, take 2 #3369

Closed

HeinrichApfelmus requested a review from paweljakubas June 7, 2023 14:42

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-3031/extendAndPrune branch 2 times, most recently from 04c6b27 to bce22c4 Compare June 7, 2023 14:51

HeinrichApfelmus self-assigned this Jun 7, 2023

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-3031/extendAndPrune branch 2 times, most recently from f13c81d to 332bef7 Compare June 7, 2023 15:31

paweljakubas reviewed Jun 12, 2023

View reviewed changes

lib/wallet/src/Cardano/Wallet/Checkpoints.hs Outdated Show resolved Hide resolved

paweljakubas reviewed Jun 12, 2023

View reviewed changes

paweljakubas approved these changes Jun 12, 2023

View reviewed changes

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-3031/extendAndPrune branch from 332bef7 to 68ea3ad Compare June 12, 2023 13:30

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-3031/extendAndPrune branch from 3bb2e47 to 06e21e0 Compare June 14, 2023 13:31

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-3031/extendAndPrune branch from 06e21e0 to 750fe7d Compare June 14, 2023 22:17

HeinrichApfelmus added 5 commits June 15, 2023 13:10

Add function extendAndPrune

b739688

Use extendAndPrune and delete old checkpoints

907e0da

Add test on sparseArithmetic :: CheckPoint

0ee029b

Make TRANS_NEW_JOIN_01a less flaky

4cb3857

Use extendAndPrune with localTip to reduce test flakiness

6e32039

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-3031/extendAndPrune branch from 750fe7d to 6e32039 Compare June 15, 2023 11:10

Prune old checkpoints as opposed to new checkpoints.

85392f0

paweljakubas reviewed Jun 16, 2023

View reviewed changes

paweljakubas approved these changes Jun 16, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADP-3031] Simplify checkpoint pruning, take 3 #3988

[ADP-3031] Simplify checkpoint pruning, take 3 #3988

HeinrichApfelmus commented Jun 7, 2023 •

edited

Loading

paweljakubas Jun 12, 2023

HeinrichApfelmus Jun 12, 2023

HeinrichApfelmus Jun 12, 2023

paweljakubas left a comment

HeinrichApfelmus commented Jun 12, 2023

iohk-bors bot commented Jun 12, 2023

HeinrichApfelmus commented Jun 13, 2023

HeinrichApfelmus commented Jun 14, 2023

paweljakubas Jun 16, 2023

paweljakubas left a comment

HeinrichApfelmus commented Jun 16, 2023

iohk-bors bot commented Jun 16, 2023

HeinrichApfelmus commented Jun 16, 2023

iohk-bors bot commented Jun 16, 2023

HeinrichApfelmus commented Jun 16, 2023

iohk-bors bot commented Jun 16, 2023

HeinrichApfelmus commented Jun 16, 2023

iohk-bors bot commented Jun 16, 2023

[ADP-3031] Simplify checkpoint pruning, take 3 #3988

Are you sure you want to change the base?

[ADP-3031] Simplify checkpoint pruning, take 3 #3988

Conversation

HeinrichApfelmus commented Jun 7, 2023 • edited Loading

Overview

Details

Comments

Issue Number

paweljakubas Jun 12, 2023

Choose a reason for hiding this comment

HeinrichApfelmus Jun 12, 2023

Choose a reason for hiding this comment

HeinrichApfelmus Jun 12, 2023

Choose a reason for hiding this comment

paweljakubas left a comment

Choose a reason for hiding this comment

HeinrichApfelmus commented Jun 12, 2023

iohk-bors bot commented Jun 12, 2023

HeinrichApfelmus commented Jun 13, 2023

HeinrichApfelmus commented Jun 14, 2023

paweljakubas Jun 16, 2023

Choose a reason for hiding this comment

paweljakubas left a comment

Choose a reason for hiding this comment

HeinrichApfelmus commented Jun 16, 2023

iohk-bors bot commented Jun 16, 2023

HeinrichApfelmus commented Jun 16, 2023

iohk-bors bot commented Jun 16, 2023

HeinrichApfelmus commented Jun 16, 2023

iohk-bors bot commented Jun 16, 2023

HeinrichApfelmus commented Jun 16, 2023

iohk-bors bot commented Jun 16, 2023

HeinrichApfelmus commented Jun 7, 2023 •

edited

Loading