Nethermind


Priority

V3123 Perhaps the ‘??’ operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. Nethermind.Trie TrieNode.cs 43
V3123 Perhaps the ‘??’ operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. Nethermind.Trie TrieNode.cs 44

public int MemorySize
{
  get
  {
    int unaligned = (Keccak == null ? MemorySizes.RefSize : 
        MemorySizes.RefSize + Keccak.MemorySize) 
        + (MemorySizes.RefSize + FullRlp?.Length 
                                 ?? MemorySizes.ArrayOverhead)
        + (MemorySizes.RefSize + _rlpStream?.MemorySize 
                                 ?? MemorySizes.RefSize)   
        + MemorySizes.RefSize + (MemorySizes.ArrayOverhead + _data?.Length 
        * MemorySizes.RefSize ?? MemorySizes.ArrayOverhead) 
        + MemorySizes.SmallObjectOverhead + (Key?.MemorySize ?? 0);
    return MemorySizes.Align(unaligned);
  }
}

The analyzer advises us to check how we use the “??” operators. In order to understand what the problem is, I propose to consider the following situation. Look at this line here:

(MemorySizes.RefSize + FullRlp?.Length ?? MemorySizes.ArrayOverhead)

MemorySizes.RefSize and MemorySizes.ArrayOverhead are constants.

public static class MemorySizes
{
    ...
    public const int RefSize = 8;
    public const int ArrayOverhead = 20;
    ...
}

Therefore, for clarity, I suggest rewriting the line, substituting their values:

(8 + FullRlp?.Length ?? 20)

Now suppose FullRlp is null. Then (8 + null) will be null. Next, we get the expression (null ?? 20), which will return 20.

As a result, in case if FullRlp is null, the value from MemorySizes.ArrayOverhead will always be returned regardless of what is stored in MemorySizes.RefSize. The fragment on the line below is similar.

But the question is, did the developer want this behavior? Let’s look at the following line:

MemorySizes.RefSize + (MemorySizes.ArrayOverhead 
    + _data?.Length * MemorySizes.RefSize ?? MemorySizes.ArrayOverhead)

Same as in the fragments above, MemorySizes.RefSize is added to the expression, but note that after the first “+” operator there is a bracket.

It turns out that it is MemorySizes.RefSize which we should add some expression to, and if it is null, then we should add another one. So the code should look like this:

public int MemorySize
{
  get
  {
    int unaligned = (Keccak == null ? MemorySizes.RefSize : 
       MemorySizes.RefSize + Keccak.MemorySize) 
       + (MemorySizes.RefSize + (FullRlp?.Length 
                                 ?? MemorySizes.ArrayOverhead))    // <=
       + (MemorySizes.RefSize + (_rlpStream?.MemorySize 
                                 ?? MemorySizes.RefSize))          // <=
       + MemorySizes.RefSize + (MemorySizes.ArrayOverhead + _data?.Length 
       * MemorySizes.RefSize ?? MemorySizes.ArrayOverhead) 
       + MemorySizes.SmallObjectOverhead + (Key?.MemorySize ?? 0);
    return MemorySizes.Align(unaligned);
  }
}

Again, this is only an assumption, however, if the developer wanted different behavior, then one should explicitly indicate this:

((MemorySizes.RefSize + FullRlp?.Length) ?? MemorySizes.ArrayOverhead)

In doing so, the one who reads this code wouldn’t have to delve into it for a long time puzzling out what is happening here and what the developer wanted.

Please click here to see more bugs from this project.

One thought on “Nethermind

  1. When our biggest bug is in diagnostic piece of code that is currently unused outside of tests then you start to feel good about the other 300k+ lines of code.

    Like

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google photo

You are commenting using your Google account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s

This site uses Akismet to reduce spam. Learn how your comment data is processed.