If we use the same hash function, do we want to have different values for empty inner digest and empty leaf digest, or it's fine in this context?

I cannot think of any immediate harm here.

got it thanks! looks fine

If the intention is to be compatible with Zcash note commitment trees, then there's a sentinel value for empty leaves but there is no special case for empty internal nodes; they have the same value as would be expected from hashing their children. This doesn't require actually computing hashes for empty internal nodes, since the hash of a completely empty subtree only depends on its height (and can be computed in logarithmic time, then cached).

@daira Thanks for your comments! Honestly I was just following the current arkwork's merkle tree implementation and didn't give too much thought. Now digging into it deeper, here is what I find:

For example, on JubJub, both P::InnerDigest::default() and P::LeafDigest::default() is {x:0, y: <one of 0's corresponding y on the curve> }. In the current implementation, the empty inner node a special value that is not equal to the "true" value where you actually compute from subtree. Like you said, it is easy to compute these empty inner nodes once and then use the cached value.

It is indeed not super "principled" but I am not too much concerned security wise due to the CRH (please let me know if i am wrong). If you think there is a concrete security issue here. We should fix both this PR and the arkworks merkle tree implementation.

There is technically a small security issue in that if you use a sentinel value for internal nodes, you must argue that it was chosen such that it would be infeasible to find a preimage. (I.e. it is technically possible for there to be a back door where the sentinel is chosen to be the hash of some nonempty subtree.) But the main issue is just interoperability.