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 hardfork code #1718

Merged
merged 16 commits into from Apr 29, 2019

Conversation

Projects
3 participants
@jmjatlanta
Copy link
Contributor

commented Apr 12, 2019

Fixes #1553

There are some hardfork dates that have long passed, and the alternate logic never was used. This PR removes those unnecessary sections.

All hardfork code prior to v3.0.0 was reviewed. The code that remains is believed to be necessary, and was untouched.

Snapshots were done on testnet and mainnet to assure that the results are the same.

jmjatlanta added some commits Feb 22, 2019

@abitmore

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Note: we may probably need to keep some of those in testnet branch.

@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Note: we may probably need to keep some of those in testnet branch.

I started testing by running snapshots against testnet. It was a while ago (mid-march). I will test again. If the snapshot comes out equal, is there something else that must be considered?

@abitmore

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

The code that remains is believed to be necessary, and was untouched.

Please remove the TODO here:

if( d.head_block_time() <= HARDFORK_CORE_583_TIME ) // TODO remove after hard fork core-583

or perhaps better replace it with a comment explaining why. I don't remember if there is another comment (or more) that need to be corrected.

I think this code can be removed:

// TODO: remove this check and the assertion after hf_834
if( next_maintenance_time <= HARDFORK_CORE_834_TIME )
FC_ASSERT( !o.extensions.value.target_collateral_ratio.valid(),
"Can not set target_collateral_ratio in call_order_update_operation before hardfork 834." );

I started testing by running snapshots against testnet. It was a while ago (mid-march). I will test again. If the snapshot comes out equal, is there something else that must be considered?

I don't remember anything else..

@abitmore

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

  • HF 834
  • HF 199
  • HF 583 (can't remove, need to update comments in code)

@abitmore abitmore added this to the 3.1.0 - Feature Release milestone Apr 12, 2019

@abitmore abitmore added this to In development in Feature Release (3.1.0) via automation Apr 12, 2019

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

chain_test failed in travis

jmjatlanta added some commits Apr 16, 2019

@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

I am working on the issues found, and re-running the snapshot comparisons on testnet and mainnet. I will reply here when those modifications and tests are complete.

Note: the commit da6337d "removal of hf834" has an incorrect commit message. Some (not all) HF code was removed. Some has to stay.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

chain_test still failing

@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

I believe I have handled all the previously found issues with this PR. Please review.

Re-ran snapshots for testnet and mainnet with no differences.

Show resolved Hide resolved libraries/chain/account_evaluator.cpp
Show resolved Hide resolved libraries/chain/account_evaluator.cpp
Show resolved Hide resolved libraries/chain/account_evaluator.cpp
Show resolved Hide resolved libraries/chain/asset_evaluator.cpp
Show resolved Hide resolved libraries/chain/asset_evaluator.cpp
if( next_maintenance_time <= HARDFORK_CORE_834_TIME )
FC_ASSERT( !o.extensions.value.target_collateral_ratio.valid(),
"Can not set target_collateral_ratio in call_order_update_operation before hardfork 834." );

_paying_account = &o.funding_account(d);
_debt_asset = &o.delta_debt.asset_id(d);
FC_ASSERT( _debt_asset->is_market_issued(), "Unable to cover ${sym} as it is not a collateralized asset.",

This comment has been minimized.

Copy link
@pmconrad

pmconrad Apr 25, 2019

Contributor

Please update the comment in lines 172-176.

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta May 2, 2019

Author Contributor

Updated comment about softfork 1465 as part of #1743
Update more work needed on this one.

Show resolved Hide resolved libraries/chain/db_market.cpp
@@ -641,25 +641,16 @@ int database::match( const limit_order_object& bid, const call_order_object& ask
// TODO remove when we're sure it's always false

This comment has been minimized.

Copy link
@pmconrad

pmconrad Apr 25, 2019

Contributor

Please update this comment to indicate why we're sure it's not always false.

@@ -56,21 +56,9 @@ struct proposal_operation_hardfork_visitor
|| (v.delta_debt.asset_id( db ).bitasset_data_id

This comment has been minimized.

Copy link
@pmconrad

pmconrad Apr 25, 2019

Contributor

Please replace the TODO comment in line 51

if (block_time < HARDFORK_CORE_188_TIME) {
FC_ASSERT(false, "Not allowed until hardfork 188");
}
}
// hf_588
// issue #588
//

This comment has been minimized.

Copy link
@pmconrad

pmconrad Apr 25, 2019

Contributor

Please update the rest of this comment to indicate why this can't be moved into asset_settle_cancel_operation::validate as suggested.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Apr 25, 2019

Contributor

The HARDFORK_CORE_1468 checks should be removed as well I think.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

As discussed on telegram we'll merge what we have for the upcoming release and create another cleanup PR later that will address the remaining comments.

@jmjatlanta jmjatlanta merged commit 7ad8447 into develop Apr 29, 2019

3 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

Feature Release (3.1.0) automation moved this from In development to Done Apr 29, 2019

@jmjatlanta jmjatlanta deleted the jmj_1553b branch Apr 29, 2019

@jmjatlanta jmjatlanta referenced this pull request Apr 29, 2019

Open

Hardfork cleanup of 3.0.1 and prior #1743

1 of 17 tasks complete
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.