Re: [PATCH proxmox-backup v3 18/30] sync: push: add helper for loading known chunks from previous snapshot

[Christian Ebner <c.ebner@proxmox.com>]

On 4/19/26 11:06 PM, Thomas Lamprecht wrote:
> Am 14.04.26 um 14:59 schrieb Christian Ebner:
>> diff --git a/src/server/push.rs b/src/server/push.rs
>> @@ -808,6 +808,41 @@ pub(crate) async fn push_group(
>>
>> +async fn load_previous_snapshot_known_chunks(
>> +    ...
>> +) {
>> +    if let Some(manifest) = upload_options.previous_manifest.as_ref() {
>> +        if let Some((_id, crypt_config)) = &params.crypt_config {
>> +            if let Ok(Some(fingerprint)) = manifest.fingerprint() {
>> +                if *fingerprint.bytes() == crypt_config.fingerprint() {
>> +                    // needs encryption during push, cannot reuse chunks from previous manifest
>> +                    return;
>> +                }
>> +            }
>> +        }
> 
> I think this has an issue when transitioning from unencrypted to encrypted
> push. The guard returns early only when the previous manifest has a
> matching fingerprint, but when params.crypt_config is Some and the
> previous manifest has no fingerprint (was unencrypted), the `if let
> Ok(Some(...))` doesn't match and we fall through to loading plaintext
> chunk digests into known_chunks, if I'm not mistaken.
> 
> In push_index, those plaintext digests won't be in encrypted_mapping (the
> chunk was never read/encrypted), so the else branch uses the plaintext
> digest as-is for MergedChunkInfo::Known. The remote target then ends up
> referencing old unencrypted chunks while the manifest says it's encrypted.

True, however the intention was to keep the optimization of loading 
known chunks for already encrypted source snapshots, since then chunks 
are not re-encypted, and re-upload can happen as is.

> Simplest fix might be to just skip known chunk loading entirely when
> crypt_config is set - the optimization can't really help when we need to
> re-encrypt anyway, since chunk digests change with encryption.

Should be enough to extend the current check by the early return in case 
there is no fingerprint on the source manifest (or failed to load it), 
but there is a crypt config?

```
+    if let Some(manifest) = upload_options.previous_manifest.as_ref() {
+        if let Some((_id, crypt_config)) = &params.crypt_config {
+            if let Ok(Some(fingerprint)) = manifest.fingerprint() {
+                if *fingerprint.bytes() == crypt_config.fingerprint() {
+                    // needs encryption during push, cannot reuse 
chunks fro
+                    return;
+                }
+            } else {
+                // previous manifest is not pre-encrypted or failed to 
check
+                // fallback to not reuse chunks
+                return;
+            }
+        }