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

Clean up FC #1584

Open
pmconrad opened this issue Feb 13, 2019 · 12 comments

Comments

@pmconrad
Copy link
Contributor

commented Feb 13, 2019

User Story
As a developer I want to have the FC library cleaned up in order to get rid of broken/obsolete/unused code and reduce maintenance effort.

Much of the code in FC predates C++-11 and modern Boost. Both C++-11 and Boost have standardized or at least well-proven replacements for much of FC. We should get rid of all code in FC that is either unused by the core, or for which standard and/or Boost replacements exist.

Impacts
Describe which portion(s) of BitShares Core may be impacted by your request. Please tick at least one box.

  • API (the application programming interface)
  • Build (the build process or something prior to compiled code)
  • CLI (the command line wallet)
  • Deployment (the deployment process after building such as Docker, Travis, etc.)
  • DEX (the Decentralized EXchange, market engine, etc.)
  • P2P (the peer-to-peer network for transaction/block propagation)
  • Performance (system or user efficiency, etc.)
  • Protocol (the blockchain logic, consensus, validation, etc.)
  • Security (the security of system or user data, etc.)
  • UX (the User Experience)
  • Other (please add below)
    Code quality

CORE TEAM TASK LIST

  • Evaluate / Prioritize Feature Request
  • Refine User Stories / Requirements
  • Define Test Cases
  • Design / Develop Solution
  • Perform QA/Testing
  • Update Documentation
@nathanhourt

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

I've been wanting to knock out some warnings coming from FC as well; is it alright if I just tag those commits onto this issue?

@jmjatlanta

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

CMakeModules/FindBoost.cmake does not seem to work on my Windows machine (very recent cmake and boost versions). If I remove it (thereby using the default one that comes with cmake), everything works.

Building bitshares-core works fine even with the existence of libraries/fc/CMakeModules/FindBoost.cmake, but it may be that cmake uses its own instead of the one buried in our repository.

My guess is that if FindBoost.cmake was removed from our fc repository, older versions of cmake or boost may break. Hence, I do not want to remove it without knowing why it is there.

@pmconrad

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

> git log -- CMakeModules/FindBoost.cmake
commit 1a461c08a577fc7ff5f0f744ad958bddee2c51f4
Author: vogel76 <wrona@syncad.com>
Date:   Mon Feb 17 12:03:57 2014 +0100

    [BW]: [Fix] Added local version of FindBoost.cmake to avoid building problems on Ubuntu which doesn't use patched version of cmake in the distro cmake package. Here is point to cmake bugreport:
    http://public.kitware.com/Bug/view.php?id=14720

Response in http://public.kitware.com/Bug/view.php?id=14720 :

Please try with CMake 2.8.11 or higher, preferably the latest release.

Confirmed to work by followup posters.

I guess it's ok to remove if we put CMake 2.8.11 or higher into build requirements.

@pmconrad

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

Question: do we want to retain the ability to switch EC crypto between slow openssl and the fast secp256k1, or can EC openssl support be removed?

@nathanhourt

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

Question: do we want to retain the ability to switch EC crypto between slow openssl and the fast secp256k1, or can EC openssl support be removed?

If we remove it, do we still need openssl at all? Or can it be dropped as a dependency?

@pmconrad

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

OpenSSL will still be required. (Even the secp256k1-based uses it in some places, due to lack of certain primitives in that library.)
OpenSSL is also used for symmetric encryption and hashing for example.

@nathanhourt

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

Right, OK. So does it buy us anything to drop support for using openssl for EC signatures? If not, I suppose it makes sense to keep it.

@pmconrad

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

It's a simplification. If the code isn't used there's no need to keep (and maintain) it.

@nathanhourt

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2019

Fair point. Go ahead and remove it, then :)

Perfection is achieved, not when there is nothing more to add, but when there is nothing left to take away. [1]

@pmconrad pmconrad added this to In development in Feature Release (3.2.0) Jun 4, 2019

@pmconrad pmconrad self-assigned this Jun 4, 2019

@pmconrad pmconrad removed this from New -Awaiting Core Team Evaluation in Project Backlog Jun 4, 2019

@pmconrad pmconrad modified the milestone: 3.3.0 - Feature Release Jun 4, 2019

@pmconrad pmconrad added this to the 3.2.0 - Feature Release milestone Jun 4, 2019

@pmconrad pmconrad moved this from In development to In testing in Feature Release (3.2.0) Jun 11, 2019

@pmconrad pmconrad added this to To do in Feature Release (3.3.0) via automation Jun 21, 2019

@pmconrad pmconrad removed this from In testing in Feature Release (3.2.0) Jun 21, 2019

@pmconrad pmconrad moved this from To do to In testing in Feature Release (3.3.0) Jun 21, 2019

@pmconrad

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

Future ideas:

  • replace fc::optional with boost::optional or std::optional (C++17)
  • replace fc::thread with boost::fiber - this would be a major undertaking and should be combined with attempts to allow parallel read access
@abitmore

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

Please remove fc::uint160_t, because it's not really a traditional integer, for example, it doesn't have an operator+().

@pmconrad

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Merged cleanups with #1789 . Still potential for more, so please keep this open.

@pmconrad pmconrad removed their assignment Aug 14, 2019

@abitmore abitmore added this to To do in Feature Release (4.1.0) via automation Aug 14, 2019

@ryanRfox ryanRfox added this to To do in Protocol Upgrade Release (4.0.0) via automation Aug 15, 2019

@ryanRfox ryanRfox removed this from In testing in Feature Release (3.3.0) Aug 15, 2019

nathanhourt added a commit to bitshares/bitshares-fc that referenced this issue Aug 29, 2019

@ryanRfox ryanRfox moved this from To do to In development in Protocol Upgrade Release (4.0.0) Sep 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.