PANIC at pepper-sync/src/wallet/traits.rs

[Tomas-M]

I am using zingo-cli.
From time to time, it throws panic with this error:

thread 'tokio-runtime-worker' panicked at /usr/src/aa/zingolib/pepper-sync/src/wallet/traits.rs:325:13:
max checkpoints should always be higher or equal to max verification window!

This makes the wallet completely unusable, requiring full rescan. I thought this could be a of-by-one error or something like that, but after printing some debug log messages, it appears to be something else. I got help from ChatGPT, so please consider this with grain of salt, but the final solution WORKED FOR ME TO FIX THE PROBLEM so I believe we are on the right track.

If you want to test it, here is the broken wallet.dat file for download:
https://dropcube.net/dl/qvCbG/zingo-wallet.dat

The logs show that the return value is not a window-size rejection (to_remove <= 100), but instead that the implementation refuses the request when the requested height is not an existing sapling checkpoint. In other words: orchard had an exact checkpoint at the requested height but sapling did not, and the caller asked to truncate both trees to the same truncate_height. Seems like sapling rejects the request and the caller panics.

So it seems the panic is not caused by removing too many checkpoints (not a MAX_VERIFICATION_WINDOW violation). It looks like it is caused by a mismatch between the requested checkpoint height and the sapling checkpoint set (sapling has no checkpoint at requested height). The panic message is probably misleading for this particular failure mode.

Now, to the fix.

If I identified the root cause properly, then I think the best fix would be to change the caller to request truncation to a height that exists in both trees (e.g., pick a common checkpoint height, or pick the nearest lower checkpoint that both trees share). But I don't know how to do that properly. But I found a way to do it in truncate_shard_trees() - to finds common checkpoint height for truncation, below the requested truncate_height, and truncate to that common checkpoint.

EDIT: I removed the code which was here, in favour of code in my following reply.

[Tomas-M]

I think I found even a better way, it is short and works!
What it does is it finds common checkpoint height for truncation.

    /// Removes all shard tree data above the given `block_height`.
    fn truncate_shard_trees(
        &mut self,
        truncate_height: BlockHeight,
    ) -> Result<(), SyncError<Self::Error>> {
        let shard_trees = self.get_shard_trees_mut().map_err(SyncError::WalletError)?;

        // Collect all checkpoint heights <= truncate_height for both pools
        let mut sapling_checkpoints = BTreeSet::<BlockHeight>::new();
        shard_trees
            .sapling
            .store()
            .for_each_checkpoint(1_000, |height, _checkpoint| {
                if *height <= truncate_height {
                    sapling_checkpoints.insert(*height);
                }
                Ok(())
            })
            .expect("infallible");

        let mut orchard_checkpoints = BTreeSet::<BlockHeight>::new();
        shard_trees
            .orchard
            .store()
            .for_each_checkpoint(1_000, |height, _checkpoint| {
                if *height <= truncate_height {
                    orchard_checkpoints.insert(*height);
                }
                Ok(())
            })
            .expect("infallible");

        // Find the greatest common checkpoint height <= truncate_height
        let common_height = sapling_checkpoints
            .iter()
            .rev()
            .find(|h| orchard_checkpoints.contains(h))
            .copied()
            .unwrap_or(truncate_height);

        // Truncate both trees to the common checkpoint height
        if !shard_trees.sapling.truncate_to_checkpoint(&common_height)? {
            panic!("max checkpoints should always be higher or equal to max verification window!");
        }
        if !shard_trees.orchard.truncate_to_checkpoint(&common_height)? {
            panic!("max checkpoints should always be higher or equal to max verification window!");
        }

        Ok(())
    }

[Tomas-M]

If you like this solution, I will submit PR, please let me know.

[Oscar-Pepper]

hi @Tomas-M interesting timing, i have pulled off my other critical work to have a look at this yesterday and today. Thanks for your input. You have provided a potential solution, there are some other things we must take care of to go down this root such as also updating the scan ranges to reflect that the wallet has been truncated to a different place. There is also the issue that if these checkpoints are missing as you describe, then the wallet will error when trying to send using that height as an anchor.. requiring both sapling and orchard checkpoints to succeed. But maybe we can try to implement similar logic to what you have provided when choosing an anchor. The intended behaviour it we should create a sapling and orchard shardtree checkpoint for every block and retain the top 100. The root cause of this bug is that the librustzcash library we use in trial decrpytion doesnt allow us to create a checkpoint for blocks that dont contain any outputs of a given shielded pool.. so we must fill in the gaps. I am looking into this logic which fills in the gaps to fix this bug so we should always have a checkpoint available instead of having to search for a block where there are both

[Tomas-M]

... so we should always have a checkpoint available instead of having to search for a block where there are both

Yeah that would be definitely better.

... the wallet will error when trying to send using that height as an anchor

Using unmodified zingo-cli, I got similar error few times in the past (I wasn't able to send funds due to some "anchor error"). If cause of that anchor error is missing checkpoints, then resolving this will also fix that.

[Oscar-Pepper]

after some investigation and adding a new test which passes I can confirm that the sync engine is correctly filling in the missing checkpoints in the normal case so we are digging to try to work out what edge case conditions may cause this

[Tomas-M]

Here in the code where checkpoint is added, is it guaranteed that the previous checkpoint at height-1 exists? Shouldn't this check be lower than or equal <=, instead of just equal ==?

zingolib/pepper-sync/src/wallet/traits.rs

Line 365 in ee0a776

if *height == checkpoint_height - 1 {

[Oscar-Pepper]

Here in the code where checkpoint is added, is it guaranteed that the previous checkpoint at height-1 exists? Shouldn't this check be lower than or equal <=, instead of just equal ==?

zingolib/pepper-sync/src/wallet/traits.rs

Line 365 in ee0a776

if *height == checkpoint_height - 1 {

we can't check further here as we risk getting an incorrect treestate due to edge cases with non-linear scanning. my solution to this problem was to fetch the frontiers from the server if the directly previous checkpoint wasnt available. this block is just below:

        let tree_state = if let Some(checkpoint) = previous_checkpoint {
            checkpoint.tree_state()
        } else {
            let frontiers =
                client::get_frontiers(fetch_request_sender.clone(), checkpoint_height).await?;
            let tree_size = match D::SHIELDED_PROTOCOL {
                ShieldedProtocol::Sapling => frontiers.final_sapling_tree().tree_size(),
                ShieldedProtocol::Orchard => frontiers.final_orchard_tree().tree_size(),
            };
            if tree_size == 0 {
                TreeState::Empty
            } else {
                TreeState::AtPosition(incrementalmerkletree::Position::from(tree_size - 1))
            }
        };
``

[Oscar-Pepper]

this code may look slightly different as i have fixed a couple of off-by-ones here https://github.com/zingolabs/zingolib/pull/2013/files. however, unfortunately none of these are the actual bug fix to this issue

[Tomas-M]

If everything seems OK then maybe it's somewhere else. For example, if network error delays the sync and zingo-cli is killed during the sync (by system timeout settings), it may build the trees properly in memory, but not write the data correctly to the wallet file? Just thinking aloud...

[Oscar-Pepper]

If everything seems OK then maybe it's somewhere else. For example, if network error delays the sync and zingo-cli is killed during the sync (by system timeout settings), it may build the trees properly in memory, but not write the data correctly to the wallet file? Just thinking aloud...

Yes there may be something that is not covered in a test that has evaded my thought experiments for these edge cases. However, in terms of the zingolibs implementation of pepper-sync at least, this should never be an issue as it holds the updated wallet data in memory and writes it all at once. So if a wallet file has a saved scan range that is scanned then all its data will be consistent with that.

[Oscar-Pepper]

If you could provide any additional data for the sync state when this error is hit that would be really useful.. I am working on at least stabilizing the wallet now with some error recovery but I have yet to find a case/bug where a checkpoint is missing in the top 100 blocks which i beilieve must be the issue as re-org should never be larger than this

[Tomas-M]

If you could provide patch to current sources which will log the infos you need, I will compile custom zingo-cli
I run it from cron each 5 minutes to query several wallets, so as soon as it detects its stuck, I can collect the logs and send it to you.
Or does it support some DEBUG flag so it would print lots of infos without additional patches?