Improve granularity of copy_from_upstream

[SWilson4]

As things currently stand, the copy_from_upstream script attempts to integrate updates for all schemes sourced from a given upstream. The update source is given by a git commit SHA for the upstream repo.

It would be nice to have the option to override this on a scheme-by-scheme basis so that integrating upstream code doesn't get messy if OQS (deliberately) goes out of sync with an upstream source on some algorithms but not others. For instance, we could specify that the PQClean aarch64 implementations of Dilithium and Kyber should remain at the latest commit before PQClean updated their implementation to be in sync with the pq-crystals standard branches while still pulling the latest updates to HQC and Sphincs+.

[baentsch]

Actually, I think I just "discovered" a "solution" for this in the code of copy_from_upstream.py: Just set the environment variable NOT_READY to families you don't want to copy and they're skipped. Correct @bhess? I know, documenting any feature (see your commit adding this) is not great fun -- but not doing it decreases the code's value and causes others to re-invent the wheel (or go ballistic). In any case, not good. In that light, could you please also take a look at #1586, @bhess? Does this ring any bells in your mind? I'm now somewhat demotivated to go through this logic...

[SWilson4]

I tested running NOT_READY="kyber dilithium" python3 copy_from_upstream.py copy on my branch for #1585, and it made a bunch of changes we wouldn't want it to, including removing Kyber and Dilithium from CMakeLists.txt. So unfortunately I don't think it's a quick fix. That said, perhaps it could be turned into one? I haven't looked closely at it yet.

[baentsch]

including removing Kyber and Dilithium from CMakeLists.txt. So unfortunately I don't think it's a quick fix. That said, perhaps it could be turned into one?

Yeah... Removing Kyber and Dilithium completely sounds somewhat suboptimal... @bhess: The "NOT_READY" feature has been introduced by you: Would you think this can be extended (more easily than re-working the copy_from_upstream logic) to resolve this issue and would you be willing to lend a hand? Or is this actually code you'd rather see completely eliminated (as you didn't answer my question above)?

[bhess]

@bhess: The "NOT_READY" feature has been introduced by you: Would you think this can be extended (more easily than re-working the copy_from_upstream logic)

The feature seemed to be introduced even earlier in https://github.com/open-quantum-safe/liboqs/pull/841, my commit merely refactored the script formerly called copy_from_pqclean.py.

I think a feature to selectively specify which commit/tag to chose per algorithm/architecture would be useful. Another use case will be if we decide to support e.g. both Kyber-R3 and the Kyber "standard" branch, which will correspond to different commits in the same repository. If NOT_READY isn't a workaround, perhaps editing the copy_from_upstream logic is more promising.

[baentsch]

The feature seemed to be introduced even earlier in https://github.com/open-quantum-safe/liboqs/pull/841, my commit merely refactored the script formerly called copy_from_pqclean.py.

Yikes. This had been done at a time when we didn't import from different upstreams; hence this feature should not have survived the refactoring. Apologies for not spotting this in the PR review of #883. Mea culpa.

editing the copy_from_upstream logic is more promising.

It absolutely is (necessary to do that) -- particularly if we'd want to support several different upstream branches for the same algorithm(s). But would we really need to? In my eyes the "standard" upstream should be integrated into an entirely different algorithm family using the NIST naming and sit alongside the old R3 code (until that is sunset), no?

So what about this proposal: We (probably I :} review the NOT_READY feature and update it to the "multi-upstream" world. Goal would be to stop algorithms tagged this way to receive any upstream updates, but remain active. In parallel/separately we could add logic to add&pull the new "ML" algs from wherever (thus also closing #1521).

Reasonable @SWilson4 @bhess ? Helping hands still welcome as I'm still "on the road". It possibly is simple, but I first need to fully understand the current logic.... Yes, I saw your "1-2 weeks timeline" comment somewhere, @bhess, so I don't count on you.

[SWilson4]

Reasonable @SWilson4 @bhess ? Helping hands still welcome as I'm still "on the road". It possibly is simple, but I first need to fully understand the current logic.... Yes, I saw your "1-2 weeks timeline" comment somewhere, @bhess, so I don't count on you.

Sounds reasonable to me!

[baentsch]

FYI, my plan is to introduce another upstream to copy_from_upstream.yml called "oldpqclean": That shall reference the last commit in which Kyber&Dilithium aarch64 were OK. In turn, I'll add those algs in the "main" pqclean to the upstream "ignore" section as such:

upstreams:
  -
    name: oldpqclean
    git_url: https://github.com/PQClean/PQClean.git
    git_branch: master
    git_commit: 8e220a87308154d48fdfac40abbb191ac7fce06a
    kem_meta_path: 'crypto_kem/{pqclean_scheme}/META.yml'
    sig_meta_path: 'crypto_sign/{pqclean_scheme}/META.yml'
    kem_scheme_path: 'crypto_kem/{pqclean_scheme}'
    sig_scheme_path: 'crypto_sign/{pqclean_scheme}'
    patches: [pqclean-dilithium-arm-randomized-signing.patch, pqclean-kyber-armneon-shake-fixes.patch, pqclean-kyber-armneon-768-1024-fixes.patch]
    ignore: pqclean_sphincs-shake-256s-simple_aarch64, pqclean_sphincs-shake-256s-simple_aarch64, pqclean_sphincs-shake-256f-simple_aarch64, pqclean_sphincs-shake-192s-simple_aarch64, pqclean_sphincs-shake-192f-simple_aarch64, pqclean_sphincs-shake-128s-simple_aarch64, pqclean_sphincs-shake-128f-simple_aarch64
  -
    name: pqclean
    git_url: https://github.com/PQClean/PQClean.git
    git_branch: master
    git_commit: 0657749a785db30e7f49e9435452cb042edb1852
    kem_meta_path: 'crypto_kem/{pqclean_scheme}/META.yml'
    sig_meta_path: 'crypto_sign/{pqclean_scheme}/META.yml'
    kem_scheme_path: 'crypto_kem/{pqclean_scheme}'
    sig_scheme_path: 'crypto_sign/{pqclean_scheme}'
    patches: [pqclean-sphincs.patch]
    ignore: pqclean_sphincs-shake-256s-simple_aarch64, pqclean_sphincs-shake-256s-simple_aarch64, pqclean_sphincs-shake-256f-simple_aarch64, pqclean_sphincs-shake-192s-simple_aarch64, pqclean_sphincs-shake-192f-simple_aarch64, pqclean_sphincs-shake-128s-simple_aarch64, pqclean_sphincs-shake-128f-simple_aarch64, pqclean_kyber512_aarch64, pqclean_kyber1024_aarch64, pqclean_kyber768_aarch64, pqclean_dilithium2_aarch64, pqclean_dilithium3_aarch64, pqclean_dilithium5_aarch64

This seems to work pretty nicely apart from some problems in the doc update script. Consequence though is that all Kyber+Dilithium aarch64 files get moved to new folders "oldpqclean", so the PR will look large. Objections to this approach @SWilson4 @dstebila ?

Edit/Add: This now works OK and is implemented in #1595.