ADR101:Fixing the retain height set logic #1261
Conversation
… not accepted only if it is less than the current min height. Fixed potential corner case in checking for the minimum height in case no retain height is set. This corner case should not be present in practice as the app retain height is expected to be set but it is a safety check in case the logic of setting it on startup changes down the road.
| currentAppRetainHeight = height | ||
| currentMinRetainHeight := p.findMinRetainHeight() | ||
| if height < currentMinRetainHeight { | ||
| return ErrPrunerCannotLowerRetainHeight |
There was a problem hiding this comment.
Not sure if this logic returns the desirable outcome. For example, let's assume the following scenario:
| DCRetainHeight | AppRetainHeight | Height |
|---|---|---|
| 98 | 100 | 99 |
This logic would return a currentMinRetainHeight = 98 and since height (99) > currentMinRetainHeight (99) this would set the AppRetainHeight to 99 but it was already at 100, so after executing this logic you would end up with:
| DCRetainHeight | AppRetainHeight | Height |
|---|---|---|
| 98 | 99 | 99 |
I'm thinking that the logic in p.findMinRetainHeight() shouldn't be used in the logic to Set the retain heights, because it mixes the DC and the App retain height which might make the things more complicated. Maybe we just need to check here if the current AppRetainHeight is lower or not and set it accordingly.
Then in the pruner logic to check between the AppRetainHeight and DCRetainHeight whichever is lower then we use the lower one (the one returned by p.findMinRetainHeight
There was a problem hiding this comment.
Can we perhaps introduce a comprehensive set of tests for the desired behaviour?
There was a problem hiding this comment.
so, we will probably adopt Thane's solution but for completness I'll repeat my comment from Slack here. I don't see it as particularly wrong if the app changes it's mind if the current retain height it had set before was not use yet (because there was another lower one set).
| currentMinHeight := p.findMinRetainHeight() | ||
|
|
||
| if height < currentMinHeight { | ||
| return ErrPrunerCannotLowerRetainHeight |
There was a problem hiding this comment.
same comment as above should apply here
go.sum
Outdated
| @@ -325,6 +325,7 @@ github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre | |||
| github.com/go-sql-driver/mysql v1.7.0 h1:ueSltNNllEqE3qcWBTD0iQd3IpL/6U+mJxLkazJ7YPc= | |||
| github.com/go-sql-driver/mysql v1.7.0/go.mod h1:OXbVy3sEdcQ2Doequ6Z5BW6fXNQTmx+9S1MCJN5yJMI= | |||
| github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY= | |||
| github.com/go-task/slim-sprig v0.0.0-20210107165309-348f09dbbbc0/go.mod h1:fyg7847qk6SyHyPtNmDHnmrv/HOrqktSC+C9fM+CJOE= | |||
There was a problem hiding this comment.
Hmm, what are these new dependencies for...
There was a problem hiding this comment.
No go.mod updates, so these should be removed by a go mod tidy.
state/pruner.go
Outdated
| } | ||
| currentAppRetainHeight, err := p.stateStore.GetApplicationRetainHeight() | ||
| noAppRetainHeight := false | ||
| _, err := p.stateStore.GetCompanionBlockRetainHeight() |
There was a problem hiding this comment.
What is this get operation for? Checking if the key already exists in the database?
There was a problem hiding this comment.
no, it seems mostly to check if there's no error with the key in the database (e.g. < 0). Do we really need this ?
There was a problem hiding this comment.
You are totally right, this was left over from the version where I checked the new height of the dc companion is not lower than its own previous height, which I deemed not needed anymore because of the comparison against the minimum height ..allowing it to set a lower height in case it's own height was not the miminum.
|
I think I get a part of the problem @jmalicevic is on to: at startup, we l 8000 ack explicit means to determine if the data companion will be there to prune. The previous behaviour was to prune to the height the application has commanded, but if we just do that, we might lose data the companion has not retrieved yet. Can we use the configuration setting |
Why not just honour the If pruning's configured to not respect the data companion (i.e. that flag's set to |
Oh yes, I forgot we have that one too. |
|
Closing becuase we will adopt the logic in #1271 |
This PR addresses comments made internally on potential corner cases where the retain height is not accepted in case any height height is set , even though it is not the height used for pruning.
Example
Data companion wants to set the height to
50.The old logic would refuse this because the application retain height is set to 100 and we assume blocks to 100 are pruned. However, we will prune only up until the lower of the two retain heights (20) in this case. So it makes sense to accept this value.
If the application retain height was the only one set, we would then refuse this as a way to signal to the data companion that the request height is invalid and those blocks might have indeed been pruned.