kroma-network / tachyon Goto Github PK

Issue type

Refactoring

Current behavior?

In this library, 'identities' is implemented separately from 'monoids'
Considering the definition of a monoid in abstract algebra, the currently implemented 'monoids' should be renamed as 'semigroups'.
Monoids: Monoids are semigroups with identity.
Semigroup: Semigroups may be considered a special case of magmas where the operation is associative, or as a generalization of groups, without requiring the existence of an identity element or inverses.

Expected Behavior?

Classification of libraries that fit the definition of abstract algebra.

As written in Google C++ Style Guide: Use of const, indicating the variables that are not changed with const keyword makes easier for people to understand how the variable is used, along with the complier to do better type checking, assuring better code.

So, as in suggestion of this comment, seems reasonable.

However, through out our repository, the use of const keyword for local variables is not organized well. Plus, in the decision section of the Google C++ Style Guide: Use of const, it says

Using const on local variables is neither encouraged nor discouraged.

It's quite trivial, but I thought it would be great to at least have a discussion on this point and have a policy with regards to this throughout our repository.

Issue type

Bug

OS platform and distribution

MacOS (prior to v13.5.1)

Current behavior?

In certain versions of MacOS (prior to v13.5.1), a bug related to incorrect Bigint divide operations has been detected in the field generator when using the optimized build.

Expected Behavior?

Identify the flag causing the bug in the specific version(prior to v13.5.1) of MacOS and fix it.

Standalone code or description to reproduce the issue

Run the following command on MacOS versions earlier than v13.5.1
bazel build -c opt //...

Issue type

Feature Request

Current behavior?

Currently Inverse() method returns a inverse and panics if failed using CHECK(). But this isn't GPU interoperable code. So I suggest it to change it to return std::optional<T> when it failed.

Expected Behavior?

Returns false if failed and true and populates the out parameter.

Issue type

Feature Request

Current behavior?

Some exposes field type as F and others as Field.

Expected Behavior?

Unifying how to expose type for Field

Issue type

Documentation Feature Request

Current behavior?

Document doesn't exist.

Expected Behavior?

Something like halo2 book.

• The goal of the project
• Code structure
• ...

Issue type

Feature Request

Describe the feature

@chokobole,

https://github.com/vosen/ZLUDA

it can give feature run CUDA program on amd video cards if cuda code is optimized, and speed up work on amd.

Issue type

Feature Request

Current behavior?

Some of constructors that is able to be attached with noexcept remains without it.

Expected Behavior?

Add noexcept to move constructors.

Additional context

https://google.github.io/styleguide/cppguide.html#noexcept

Issue type

Support

Current behavior?

We only ran tests on little-endian based platform.

Expected Behavior?

Figure out how to run tests on big-endian based platform and enable code on it as well.

Current behavior?

In our CI process, Cpplint is set up to check files that have been modified as identified by git diff.
However, CI skips files that have been moved, because it can not open and read them.

For instance, in commit 5023e99, the paths of permutation_argument_stringifer.h and permutation_verifying_key_stringifier.h were changed. This change led to an oversight of a header guard typo, as the CI process skipped these files.

Expected Behavior?

Due to the extensive size of our codebase, running Cpplint on the entire code is not efficient. Therefore, we aim to check only the modified files. However, the process needs enhancement to ensure that files with changed paths (moved files) are not skipped and are thoroughly checked.

Standalone code or description to reproduce the issue

1. Introduce an intentional error recognizable by Cpplint, such as an inappropriate header guard, in a file.
2. Move the file to a different directory.
3. Run the CI process and observe if Cpplint checks the moved file.

Further details can be found in commit 5023e99 and its CI Log.

Issue type

Build/Install

OS platform and distribution

macOS Sonoma

Current behavior?

If I try to build tachyon with openmp on mac, it causes an error.

clang++: error: unsupported option '-fopenmp'

Expected Behavior?

It should be buildable.

Standalone code or description to reproduce the issue

bazel test --//:has_openmp //...

Issue type

Build/Install

OS platform and distribution

macOS Sonoma

Current behavior?

We lack an easy installation tool like brew, .pkg, or .dmg. Among these, brew stands out as it can be installed effortlessly. Additionally, we can craft a Ruby script to build from the source, ensuring independence from OS versions or CPU architectures. On the other hand, for Linux, we must provide packages tailored to each OS or CPU.

See https://developer.nvidia.com/cuda-downloads?target_os=Linux&target_arch=x86_64&Distribution=Ubuntu.

Expected Behavior?

Submit PR in https://github.com/Homebrew/homebrew-core/pulls.

Issue type

Feature Request

Current behavior?

We mixed using BNXXX or BnXXX.

BNCurve and BN254Config are the case starting with BN
Bn254SHPlonkProverXXX, Bn254ProvingKeyXXX and kIsBn254XX are the cast starting with Bn.

Expected Behavior?

Choose one of the styles and apply the style to every case.

Current behavior?

There are many cases where linter errors are ignored(//NOLINT).
This is a case where cpplint was added to CI, but was left unattended because the author's intention was not understood.
See #74

Expected Behavior?

Unintentional mistakes among parts marked as //NOLINT must be corrected.

Issue type

Bug

OS platform and distribution

macOS Sonoma

Current behavior?

https://github.com/kroma-network/tachyon/actions/runs/7169954768/job/19537222364?pr=196

Expected Behavior?

Build and test should be successful.

Standalone code or description to reproduce the issue

> cd tachyon/py/test
> bazel test --config macos_arm64  //...
ERROR: /private/var/tmp/_bazel_chokobole/78478a39924d7742b4385236af2bac37/external/kroma_network_tachyon/tachyon/base/mac/BUILD.bazel:21:21: in objc_library rule @@kroma_network_tachyon//tachyon/base/mac:mac_logging: 
Traceback (most recent call last):
        File "/virtual_builtins_bzl/common/objc/objc_library.bzl", line 61, column 52, in _objc_library_impl
        File "/virtual_builtins_bzl/common/objc/semantics.bzl", line 54, column 13, in _check_toolchain_supports_objc_compile
Error in fail: Compiling objc_library targets requires the Apple CC toolchain which can be found here: https://github.com/bazelbuild/apple_support#toolchain-setup
ERROR: /private/var/tmp/_bazel_chokobole/78478a39924d7742b4385236af2bac37/external/kroma_network_tachyon/tachyon/base/mac/BUILD.bazel:21:21: Analysis of target '@@kroma_network_tachyon//tachyon/base/mac:mac_logging' failed
ERROR: Analysis of target '//tachyon/math/elliptic_curves/short_weierstrass:short_weierstrass_unittests_unittests' failed; build aborted: Analysis failed
INFO: Elapsed time: 0.080s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
ERROR: Build did NOT complete successfully
ERROR: No test targets were found, yet testing was requested

Issue type

Bug

Current behavior?

FileTest.GetInfoForCreationTime is failed when testing with openmp on.

tachyon/base/files/file_unittest.cc:474: Failure
Expected: (info.creation_time.ToDeltaSinceUnixEpoch().InSeconds()) >= (before_creation_time_s), actual: 1698560434 vs 1698560435
Stack trace:
  0x5621b1e[363](https://github.com/kroma-network/tachyon/actions/runs/6681633807/job/18155958345?pr=123#step:9:364)dc: tachyon::FileTest_GetInfoForCreationTime_Test::TestBody()
  0x5621b1ec6fa9: testing::internal::HandleSehExceptionsInMethodIfSupported<>()
  0x5621b1ec06bd: testing::internal::HandleExceptionsInMethodIfSupported<>()
  0x5621b1ea0916: testing::Test::Run()
  0x5621b1ea13f0: testing::TestInfo::Run()
... Google Test internal frames ...

Expected Behavior?

FileTest.GetInfoForCreationTime should pass.

Links

• CI #123
• CI #147

#107 (comment)

Above comment is valid since the naming seems inappropriate.

But it is out of scope of #107, so will be fixed in later PR.

first thanks for this very interesting project and latest commits which allows build to succeed

using following script (on Ubuntu 23.10 with Python 3.11.6 cuda version release 12.4)

#!/bin/bash

export TMP=/tmp
export USE_BAZEL_VERSION=6.5.0
export CC=/usr/bin/gcc-12
export CXX=/usr/bin/g++-12
export GCC_HOST_COMPILER_PATH=/usr/bin/gcc-12

bazel build --config linux --config cuda //...
bazel test --config linux //...

subset of tests succceeds

Executed 45 out of 67 tests: 22 tests pass and 45 fail locally.

failures begin at //tachyon/c/math/elliptic_curves/msm:msm_unittests FAILED in 0.0s

question is (before I step in code any further) is this environment supported ? or this is just WIP (work in progress)

IMO, as what I've suggested in this comment, #206 (comment).

template <typename Domain, typename Poly,
          typename Evals = typename Domain::Evals, typename F>
Evals CoeffToExtendedPart(const Domain* domain, const Poly& poly, const F& zeta,

this kind of code would be more correct to be written as below.

template <typename Domain>
typename Domain::Evals CoeffToExtendedPart(const Domain* domain, const typename Domain::Poly& poly, const typename Domain::Field& zeta,

Issue type

Feature Request

Current behavior?

For example, when calling PrimeField::FromDecString() with a malformed string, it crashes a program.

constexpr static PrimeField FromDecString(std::string_view str) {
  return PrimeField(BigInt<N>::FromDecString(str));
}

Expected Behavior?

It can accept the malformed string by changing the return type of From(Dec|Hex)String to std::optional<PrimeField>

constexpr static std::optional<PrimeField> FromDecString(std::string_view str) {
  std::optional<BigInt<N>> value = BigInt<N>::FromDecString(str);
  if (!value.has_value()) return std::nullopt;
  return PrimeField(std::move(*value));
}

Issue type

Feature Request

Current behavior?

PrimeFieldGmp is provided as a answer when testing correctness of PriemField operation. But since mpz_class methods are not constexpr, we can't run those on GPU.

Expected Behavior?

Remove PrimeFieldGmp.