Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Rel 2.0.x: Subjective CPU billing cleos enhancement and adding subjective_cpu_bill to /v1/chain/get_account result #10250

Merged
merged 8 commits into from
Apr 17, 2021

Conversation

linhuang-blockone
Copy link
Contributor

Change Description

This PR adds subjective_cpu_bill to /v1/chain/get_account result and adds a new entity subjective cpu bandwidth to cleos get_account command. An example output looks like

permissions:
     owner     1:    1 EOS8Dq1KosJ9PMn1vKQK3TbiihgfUiDBUsz471xaCE6eYUssPB1KY
        active     1:    1 EOS8Dq1KosJ9PMn1vKQK3TbiihgfUiDBUsz471xaCE6eYUssPB1KY
memory:
     quota:       unlimited  used:      2.66 KiB

net bandwidth:
     used:               unlimited
     available:          unlimited
     limit:              unlimited

cpu bandwidth:
     used:               unlimited
     available:          unlimited
     limit:              unlimited

subjective cpu bandwidth:
     used:               186 us

Change Type

Select ONE:

  • Documentation
  • Stability bug fix
  • Other
  • Other - special case

Testing Changes

Select ANY that apply:

  • New Tests
  • Existing Tests
  • Test Framework
  • CI System
  • Other

Manual testing.

Consensus Changes

  • Consensus Changes

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

A new subjective cpu bandwidth: is added to cleos get_account command to display the used subjective cpu of the account. Please see above example output.

@@ -4,7 +4,7 @@ add_library( chain_plugin
chain_plugin.cpp
${HEADERS} )

target_link_libraries( chain_plugin eosio_chain appbase )
target_include_directories( chain_plugin PUBLIC "${CMAKE_CURRENT_SOURCE_DIR}/include" "${CMAKE_CURRENT_SOURCE_DIR}/../chain_interface/include" "${CMAKE_CURRENT_SOURCE_DIR}/../../libraries/appbase/include")
target_link_libraries( chain_plugin producer_plugin eosio_chain appbase )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no idea that would work:
https://cmake.org/cmake/help/latest/command/target_link_libraries.html#cyclic-dependencies-of-static-libraries

However, I'm not sure if we should do this or use the method providers of plugin_interface. I'm personally not a huge fan of the plugin_interface but that is the current way this is done. Thoughts @larryk85

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will look into how to use plugin_interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know an existing code example? I saw register and subscribe of method providers. But it is immediately clear to me how to use it.

plugins/chain_plugin/chain_plugin.cpp Outdated Show resolved Hide resolved
@@ -160,6 +160,8 @@ class read_only {
account_resource_limit cpu_limit;
int64_t ram_usage = 0;

uint32_t subjective_cpu_bill = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be optional so that old cleos will work with new nodeos. Please test with new cleos <-> old nodeos and old cleos <-> new nodeos.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Will change and test different versions of cleos.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested with different versions of nodeos and cleos. cleos worked in all combinations.

Copy link
Contributor

@arhag arhag Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think this would make more sense as a field in its own structure similar to net_limit and cpu_limit. This is basically the used field in account_resource_limit. And eventually (maybe not now?) we will want the max and available too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I will do that.

if( res.subjective_cpu_bill.valid() ) {
subjective_cpu_bill = *res.subjective_cpu_bill;
}
std::cout << indent << std::left << std::setw(11) << "used:" << std::right << std::setw(18) << to_pretty_time( subjective_cpu_bill ) << "\n";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to not print the section at all if not available. Most of the time it would be true to report 0 as it would not have subjective billing. However, we have released versions that have subjective billing but will not return a subjective bill amount.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will not log only when it is not valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_subjective_bill in subjective_billing.hpp always returns a value (returning 0 when subjective billing is disabled or not available). Looks like we need another API to tell if subject billing amount is present. Do that in 2.1.x?

Copy link
Contributor

@heifner heifner Apr 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a difference from a nodeos not having the code of this PR vs having subject billing disabled. If nodeos has this PR code then 0 is the correct response to print as that indicates either there is no subjective billing on the node or the account has no subjective billing applied to it. Either way there is no subjective billing for that account.

As for an indication if a node has subjective billing enabled or not at all, that should be a different api call. That api call if added would need to indicate if it is enable for API calls or P2P calls or both. In the future that api call might also provide information on any configurable values of subjective billing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explanation.

@linhuang-blockone linhuang-blockone merged commit e4ee6d4 into release/2.0.x Apr 17, 2021
@kj4ezj kj4ezj deleted the subjective_cpu_cleos_enhancement branch July 19, 2021 17:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants