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
3 of 17 tasks
pmconrad opened this issue Feb 13, 2019 · 13 comments
Open
3 of 17 tasks

Clean up FC #1584

pmconrad opened this issue Feb 13, 2019 · 13 comments
Labels
1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2d Developing Status indicating currently designing and developing a solution 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 4a Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists 6 Performance Impacts flag identifying system/user efficiency, performance, etc. 6 Security Impact flag identifying system/user security code cleanup

Comments

@pmconrad
Copy link
Contributor

pmconrad 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
@nathanielhourt
Copy link
Contributor

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?

@pmconrad pmconrad added code cleanup 1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2b Gathering Requirements Status indicating currently refining User Stories and defining Requirements 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 4a Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists 6 Performance Impacts flag identifying system/user efficiency, performance, etc. 6 Security Impact flag identifying system/user security labels Feb 13, 2019
@pmconrad pmconrad added this to New -Awaiting Core Team Evaluation in Project Backlog via automation Feb 13, 2019
@jmjatlanta
Copy link
Contributor

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
Copy link
Contributor Author

pmconrad 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
Copy link
Contributor Author

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?

@nathanielhourt
Copy link
Contributor

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
Copy link
Contributor Author

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.

@nathanielhourt
Copy link
Contributor

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
Copy link
Contributor Author

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

@nathanielhourt
Copy link
Contributor

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 added 2d Developing Status indicating currently designing and developing a solution and removed 2b Gathering Requirements Status indicating currently refining User Stories and defining Requirements labels 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
@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
nathanielhourt 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
@abitmore
Copy link
Member

abitmore commented Sep 18, 2019

Since we no longer support boost 1.57 nor lower versions, we can cleanup code like this:

#if BOOST_VERSION >= 105600
...
#else
...
#endif

@abitmore abitmore removed this from In development in Protocol Upgrade Release (4.0.0) Sep 18, 2019
@abitmore abitmore added this to To do in Feature Release (5.1.0) via automation Sep 13, 2020
@abitmore abitmore removed this from To do in Feature Release (5.1.0) Jan 6, 2021
@abitmore abitmore added this to To do in Feature Release (5.2.0) via automation Jan 6, 2021
@abitmore abitmore moved this from To do to In development in Feature Release (5.2.0) Mar 29, 2021
@abitmore abitmore removed this from In development in Feature Release (5.2.0) Apr 13, 2021
@abitmore abitmore added this to To do in Feature Release (6.1.0) via automation Apr 13, 2021
@abitmore abitmore removed this from To do in Feature Release (6.1.0) Aug 8, 2022
@abitmore abitmore added this to To do in Feature Release (7.1.0) via automation Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2d Developing Status indicating currently designing and developing a solution 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 4a Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists 6 Performance Impacts flag identifying system/user efficiency, performance, etc. 6 Security Impact flag identifying system/user security code cleanup
Development

No branches or pull requests

4 participants