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 old hardfork checks #1553

Closed
oxarbitrage opened this issue Feb 2, 2019 · 8 comments
Closed

remove old hardfork checks #1553

oxarbitrage opened this issue Feb 2, 2019 · 8 comments
Assignees
Labels
4a Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists code cleanup

Comments

@oxarbitrage
Copy link
Member

There are certain checks in the codebase that are safeguards added before a consensus upgrade is triggered and that are not needed after that date pass.

This checks can only be removed at the next hardfork as they are part of the consensus code.

This ticket is to identify and remove the most of this checks we can for the next consensus release.

Some of this checks are easily identfied in the code with Todo comments just as:
https://github.com/bitshares/bitshares-core/blob/master/libraries/chain/market_evaluator.cpp#L160-L163

@jmjatlanta jmjatlanta self-assigned this Feb 4, 2019
@ryanRfox ryanRfox removed the hardfork label Feb 12, 2019
@ryanRfox ryanRfox added the 4a Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists label Feb 12, 2019
@jmjatlanta
Copy link
Contributor

Question 1: In the process of reviewing the code, I have found 4 HARDFORK #defines (so far) that are only #defines. There is no code related to them. I can only assume there used to be, and it was removed when it was no longer needed. Can I remove such xxx.hf files from the repository?

@jmjatlanta
Copy link
Contributor

I am researching each one, collecting information. The summary is being collected here: https://docs.google.com/spreadsheets/d/1l5H3WpkxVQpyI4HzfLMaqfNl1TYRrFBBcEGtXpB7VUE

I realize Google Docs isn't the best place to post such information, but once the data becomes more static, I will post the results here for posterity.

@pmconrad
Copy link
Contributor

Yes, unused hardfork constants can be removed.

@jmjatlanta
Copy link
Contributor

jmjatlanta commented Feb 22, 2019

I am coming to the end of my research. One is confusing me. HARDFORK_CORE_583_TIME. Comments say it can be removed, but it seems dangerous to do so.

https://github.com/bitshares/bitshares-core/blob/319d7b96a4d29902db80631022bb021ca79ac950/libraries/chain/market_evaluator.cpp#L302:L334

My synopsis: That GRAPHENE_ASSERT is in the log, therefore it was executed at least once. Changing the behavior to the "else" below includes an extra check, which (in my mind) could cause a change in consensus. Therefore I believe the "if" should remain. Am I missing something?

Update: I am running both the "if" and "else" logic to see if the results would change should the "if" be removed.

@pmconrad
Copy link
Contributor

Hm. Operations that fail on this assertion shouldn't make it into the blockchain.

The only exception that comes to mind is if the operation is contained in a proposal. Can you check? I agree that this cannot be removed if the assertion is triggered.
The comment may have overlooked the possibility of proposals. (Or maybe the comment means that the comment should be removed, not the if. :-)

@abitmore
Copy link
Member

I admit that I overlooked the possibility of proposals. If the data has been included in the chain, we should keep the code, and perhaps update the comment explaining why we kept it.

@jmjatlanta
Copy link
Contributor

Reopening

@jmjatlanta
Copy link
Contributor

Fixed in part by #1718. Outstanding issues moved to #1743

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4a Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists code cleanup
Projects
None yet
Development

No branches or pull requests

5 participants