dead store in secp256k1_ecmult_gen

[jgriffiths]

In secp256k1_ecmult_gen, the final assignment:

    n_i = 0;

Is a dead store that the compiler will eliminate, if the intention is to clear the value then some asm magic will be required, otherwise this line would ideally be deleted.

Reporting as the static analyzer from clang reports this as a bug.

[real-or-random]

Ok, yeah. This code is taken from https://github.com/bitcoin-core/secp256k1. We should fix it there and then backport to here. But it's complicated: The issue is that the goal here is indeed to clear the value from memory. We have many stores of this kind (they have been added years ago), and probably none of them are effective. I have a very old PR that would address this (and it covers a previous version of this line),but it needs to be redone entirely: bitcoin-core/secp256k1#636

the static analyzer from clang reports this as a bug.

That's not a problem for us. Is it a problem for you?

[jgriffiths]

That's not a problem for us. Is it a problem for you?

I am hoping to make a clean static analysis run part of the wally CI so I don't need to keep remembering to check it manually. But there are a couple of other warnings from this library that I haven't gotten around to analyzing yet, so I'll probably need to exclude/whitelist it when I do that.

So, its not a blocker for wally, although it might be nice to add a TODO to the assignment until its fixed?

[real-or-random]

But there are a couple of other warnings from this library that I haven't gotten around to analyzing yet, so I'll probably need to exclude/whitelist it when I do that.

Yeah, we have seen some obscure warnings from the static analyzer, so I think if you really want to get a "clean" build, then some kind of whitelist will be unavoidable. Maybe try compiling with asm disabled, that makes it much easier for the compiler to reason about the code.

So, its not a blocker for wally, although it might be nice to add a TODO to the assignment until its fixed?

Well, I think it's "tracked" in bitcoin-core/secp256k1#636. When we'll look at rebasing this PR, we'll catch this line. (I just can't promise when this will happen...) So I'm closing this here, but please don't hesitate to comment further. And if you find other code improvements using static analyzer that can be fixed, please report them here (or in bitcoin-core/secp256k1 if you can find the revelant code also thee).

[jgriffiths]

Will do, thanks @real-or-random