I wonder if it’s a good idea to add a function secp256k1_modinv32_modinfo verify that checks consistency and call it on entry of every function that reads modinfo, similar to how we do this for other data structures. But if yes, that should happen in a separate PR.
Originally posted by @real-or-random in #979 (review)
Hi @real-or-random! After reviewing this issue, I have a question. To resolve this issue, I am examining the changes in this pull request: https://github.com/tusharv01/secp256k1/pull/1/files#diff-d2a3d09d59a89eaf101c5214c830763b7cc74e8ad55e15f0b987d5de6a600e11
In this change, the function secp256k1_modinv32_modinfo_verify verifies that check, and in line 61, it is also called in the secp256k1_modinv32_do_something because this function reads modinfo. As we can see in line 62, there’s only an if statement when verify_result is true. However, what should be done if verify_result is false? Should I add a proper else statement here?
Oh, hm, I suggest ignoring https://github.com/tusharv01/secp256k1/pull/1 entirely. It doesn’t make a lot of sense. (Note that this was not a PR to this repository, it’s a PR in a fork, so I assume it was just some experiment.)
I think it’s a good idea to create a secp256k1_modinv32_modinfo_verify function. Then whenever we read modinfo, or after we write it, we would call VERIFY_CHECK(secp256k1_modinv32_modinfo_verify(...)). VERIFY_CHECK is a macro that we use to check assertions. It fails in tests if the assertion does not hold, but it does nothing in production. (We do the same kind of invariant checking for other types, see for example https://github.com/bitcoin-core/secp256k1/pull/1373/files for an example how a “verify” function is called every time we read or write a certain type.)
But the first question is what are the invariants that we would like to check. You should be able to extract some from reading #979. I suggest posting a list here, and then we can take a look.
Oh, hm, I suggest ignoring tusharv01#1 entirely. It doesn’t make a lot of sense. (Note that this was not a PR to this repository, it’s a PR in a fork, so I assume it was just some experiment.)
I think it’s a good idea to create a
secp256k1_modinv32_modinfo_verifyfunction. Then whenever we read modinfo, or after we write it, we would callVERIFY_CHECK(secp256k1_modinv32_modinfo_verify(...)).VERIFY_CHECKis a macro that we use to check assertions. It fails in tests if the assertion does not hold, but it does nothing in production. (We do the same kind of invariant checking for other types, see for example https://github.com/bitcoin-core/secp256k1/pull/1373/files for an example how a “verify” function is called every time we read or write a certain type.)But the first question is what are the invariants that we would like to check. You should be able to extract some from reading #979. I suggest posting a list here, and then we can take a look.
Hi!
Please correct me if I’m wrong. As per my understanding, the secp256k1_modinv32_modinfo_verify function that I am going to create will check if the modulus is odd. This is because the secp256k1_modinv32_modinfo function requires an odd modulus. So, I am thinking that I have to write some conditions on ‘r’ here, where ‘r’ (r0, r1, r2, r3, r4, r5, r6, r7, r8) is the invariants. I think the argument of secp256k1_modinv32_modinfo_verify function should be r..Am I on the correct path?
Hi! Please correct me if I’m wrong. As per my understanding, the
secp256k1_modinv32_modinfo_verifyfunction that I am going to create will check if the modulus is odd. This is because thesecp256k1_modinv32_modinfofunction requires an odd modulus.
Yes, but I suspect there are more invariants. Maybe @sipa can comment.
I think the argument of
secp256k1_modinv32_modinfo_verifyfunction should be r
The function should take the entire modinfo. The function will need to extract the modulus to check it, but I doubt that’s r.
Labels
assurance