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

Make chain_parameters::current_fees const #1670

Closed
nathanhourt opened this issue Mar 23, 2019 · 5 comments

Comments

Projects
2 participants
@nathanhourt
Copy link
Contributor

commented Mar 23, 2019

Bug Description
Following #1556, the constness of chain_parameters::current_fees does not match that of its parent chain_parameters. In particular, current_fees -- having type shared_ptr<fee_schedule> which becomes const shared_ptr<fee_schedule> if the parent is const -- is never actually const. This was not an issue with smart_ref as references propagate their constness to the referenced object, but shared_ptr does not.

This is almost entirely an architectural purism issue, but not entirely as the fee_helper visitor does not define non-const getters for some fee types, and due to the kludgy hackery that is our fee infrastructure (I hate to say it guys, but it's kinda scary in there -- if you don't believe me, just grep for MAX_FEE_STABILIZATION_ITERATION), it's not easy to properly define a non-const version of these getters.

But in general, failing to properly propagate the constness of a struct to its members is a design flaw, and should be fixed. And through great trial and travail, I have done so (PReq coming soon to a branch near you, and an FC not very far away).

Implemented Solution

  1. Make chain_parameters::current_fees type shared_ptr<const fee_schedule> instead of shared_ptr<fee_schedule>
  2. Fix all the places where FC breaks because of step 1
  3. Add const and non-const getters get_current_fees() to chain_parameters
  4. Replace all external references to current_fees with get_current_fees()

Impacts
Describe which portion(s) of BitShares Core may be impacted by this bug. 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 (Architectural Purity)

CORE TEAM TASK LIST

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

Holding off on submitting a PReq until the FC one clears so I can update the submodule right. In the meantime, my changes are available for review here.

@abitmore

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

Looks good to me, although initial_state.initial_parameters.get_current_fees() = fee_schedule::get_default(); looks a bit strange -- a non-const getter.

@abitmore abitmore added this to To do in Feature Release (3.1.0) via automation Mar 23, 2019

@nathanhourt

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

That's a good point; I'll rename the non-const overload to get_mutable_fees()

nathanhourt added a commit to nathanhourt/bitshares-2 that referenced this issue Mar 25, 2019

Ref bitshares#1670: Rename non-const getter for clarity
Rename non-const `get_current_fees()` to `get_mutable_fees()` to
make its non-constness explicit at the call site

nathanhourt added a commit to nathanhourt/bitshares-2 that referenced this issue Mar 25, 2019

nathanhourt added a commit to nathanhourt/bitshares-2 that referenced this issue Mar 25, 2019

Ref bitshares#1670: Rename non-const getter for clarity
Rename non-const `get_current_fees()` to `get_mutable_fees()` to
make its non-constness explicit at the call site
@nathanhourt

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

PReq #1678 submitted

nathanhourt added a commit to nathanhourt/bitshares-2 that referenced this issue Apr 9, 2019

nathanhourt added a commit to nathanhourt/bitshares-2 that referenced this issue Apr 9, 2019

Ref bitshares#1670: Rename non-const getter for clarity
Rename non-const `get_current_fees()` to `get_mutable_fees()` to
make its non-constness explicit at the call site

nathanhourt added a commit to nathanhourt/bitshares-2 that referenced this issue Apr 9, 2019

pmconrad added a commit that referenced this issue Apr 10, 2019

Merge pull request #1678 from nathanhourt/issue-1670
PReq for #1670: Make chain_parameters::current_fees const
@abitmore

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Merged #1678. Closing.

@abitmore abitmore closed this Apr 10, 2019

Feature Release (3.1.0) automation moved this from To do to Done Apr 10, 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.