Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove libsnark, libgmp, mini-gmp #613

Merged
merged 50 commits into from
Aug 19, 2024
Merged

Remove libsnark, libgmp, mini-gmp #613

merged 50 commits into from
Aug 19, 2024

Conversation

DeckerSU
Copy link

@DeckerSU DeckerSU commented Mar 1, 2024

Changes

  • The old mini-gmp C-library has been removed, and the functions bitcoin_base58{en,de}code have been replaced with internal {En,De}codeBase58 from the Bitcoin codebase.
  • GMP functions are no longer used in rewards CC; the same results can be achieved more efficiently using the internal arith_uint256 type.
  • GMP functions are no longer used in payments CC; refactoring has been done to work with large numbers in calculations using arith_uint256.
  • Dilithium and musig have been removed from the CC in preparation for the upcoming update to the secp256k1 codebase.
  • Old pre-sapling Sprout (PHGRProof) proofs validation has been removed. komodod will no longer verify old Sprout proofs and will assume they are valid. This change is safe for KMD and ACs, as transactions involving Sprout after KOMODO_SAPLING_DEADLINE (February 15, 2019) were prohibited at the consensus level.
  • libnsnark and libgmp have been removed from dependencies and build systems.
  • KMD and dPoW-protected AC checkpoints have been updated.

These changes will result in faster Initial Block Download (IBD) due to saving CPU time on verifying old Sprout transactions and disabling scripts and other resource-intensive checks during IBD. Additionally, with the removal of old pre-sapling Sprout, the need for sprout-proving.key and sprout-verifying.key files in .zcash-params (~910 Mb) is eliminated.

Sources

Todo for review

I have already completed all the tasks on the TODO list by myself, but it would be beneficial to have them reviewed by someone else (please regard this list as key points or guidelines for what needs to be checked):

  • Compare the changes in the KomodoOcean and original komodod sources related to this pull request file by file to ensure that we have all the necessary changes in both codebases and have not missed or forgotten anything.
  • Test the synchronization of all dPoW-protected chains (ACs) and KMD itself from the beginning. If feasible, compare the synchronization time before and after the changes made in this pull request.
  • Review the changes with a focus on potential attacks, such as attempting to manipulate nodes during initial block synchronization with fake chains containing old proofs, creating invalid transactions, or trying to violate the established rules.
  • Conduct an additional check on the logic for calculating large numbers in the Rewards CC and Payments CC.
  • Test the build, run, and functionality of the daemon on different platforms (Linux, Windows, MacOS), even though we already have a build and other tests on the CI/CD system.
  • Consider adding to the documentation a note that new AC chains must include the -ac_sapling=1 parameter to support private transactions from the beginning. Otherwise, these chains will not be able to send shielded transactions until sapling activation, as old Sprout proofs are no longer supported and additionally, all Sprout transactions have been prohibited since February 15, 2019 by consensus rules.
  • Verify that all functions operate correctly without the presence of the sprout-proving.key and sprout-verifying.key files in the Zcash parameters.

DeckerSU and others added 30 commits March 1, 2024 14:26
TODO: solve memory leak in RewardsCalc (seems in few places we totally
forgot about call `mpz_clear` to clear the memory allocated for each
used variable).
If tests in other translation units leave pblocktree and pcoinsTip
in an initialized state, free the memory and set it to nullptr.
Unlike ZCash, we still have libsnark included in both the
build and dependency systems. Therefore, this commit should
remove all the libsnark-related components from both systems.

zcash@26a8f68
removal of missed locations that reference libnark
@smk762
Copy link

smk762 commented Mar 6, 2024

Should this also be removed? https://github.com/KomodoPlatform/komodo/blob/patch-remove-libsnark/libsnark.mk.patch

I notice there are no updates to readme, so I assume listed deps are valid, if not I'll PR changes as required.

@TheComputerGenie
Copy link

Should this also be removed? https://github.com/KomodoPlatform/komodo/blob/patch-remove-libsnark/libsnark.mk.patch

I notice there are no updates to readme, so I assume listed deps are valid, if not I'll PR changes as required.

All of those patches from 2017 should be dumped

@REM IF NOT EXIST "%APPDATA%"\ZcashParams\sprout-verifying.key (
@REM ECHO Downloading Zcash trusted setup sprout-verifying.key, this may take a while ...
@REM .\wget64.exe --progress=dot:giga --continue --retry-connrefused --waitretry=3 --timeout=30 https://z.cash/downloads/sprout-verifying.key -O "%APPDATA%"\ZcashParams\sprout-verifying.key
@REM )
IF NOT EXIST "%APPDATA%"\ZcashParams\sapling-spend.params (
ECHO Downloading Zcash trusted setup sprout-proving.key, this may take a while ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess not sprout-proving but sapling-spend should be here in echo msg?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess not sprout-proving but sapling-spend should be here in echo msg?

Yes, true. Echo messages fixed.

@DeckerSU
Copy link
Author

DeckerSU commented Mar 6, 2024

Should this also be removed? https://github.com/KomodoPlatform/komodo/blob/patch-remove-libsnark/libsnark.mk.patch

Yes, not only this file, but also all *.patch files in the repository root were leftover from previous operations unrelated to this pull request. They were accidentally left in the repository but have now been removed.

@DeckerSU
Copy link
Author

DeckerSU commented Mar 6, 2024

@dimxy Also, I propose removing all dynamic (time-based) sapling activation mechanisms from our codebase due to the complications they introduce, such as the futureblock flag and its unclear logic. Instead, I suggest hardcoding the known sapling activation heights for dPoW protected chains and setting sapling activation to begin at height.1 for new AC chains. This approach will eliminate the need for the "tricks" implemented during the sapling transition period, ensuring a cleaner codebase and facilitating the integration of future updates. If you agree with this proposal, I will create a separate issue and related pull request for further discussion.

@dimxy
Copy link
Collaborator

dimxy commented Mar 7, 2024

@dimxy Also, I propose removing all dynamic (time-based) sapling activation mechanisms from our codebase due to the complications they introduce, such as the futureblock flag and its unclear logic.

I am okay with this idea. I think it is also good to create vUpgrades arrays for asset chains, like make a dedicated vUpgrades array for each old known chain and a default array for new chains with special upgrades.
(Btw as far as I remember futureblock has several purposes, not only related to sapling code)

@ca333 ca333 merged commit 65b2da0 into dev Aug 19, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants