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

enforce resource limits #1979

Merged
merged 31 commits into from
Mar 30, 2018

Conversation

wanderingbort
Copy link
Contributor

@wanderingbort wanderingbort commented Mar 30, 2018

This resolves #353 (eventually)

I wanted to start the PR process early while I finish up some of the interfacing and billing details

commits have more details about individual changes along the way

Still todo:

  • full-integration testing (only basic functional tests are present)
  • add an objective limit set by the system contract on the max cpu usage any transaction can use
  • add network cost set by system contract per read/write lock in a transaction
  • add base network cost set by system contract per transaction
  • add pre-committed cpu usage from context free actions
  • add discount set by system contract on all cpu usage associated with context free actions
  • add discount set by system contract on net usage from prunable data (signatures and context free data) enforced when checking the commitment to the packed size

@moskvanaft @zorba80 - I had to disable the call to set_resource_limits from the system contract because the values used in the test cases to exercise things like refunds and voting violate some basic principles of the resource system like:

  • its impossible to completely unstake all the ram for an account... where would we store the account itself
  • Unstaking all of the cpu_resources from an account leaves 0 capacity for delivering the refund action... maybe system floats some minimum stake to all accounts at all time to make stuff like this work out?
  • at all times, the account needs to maintain some stake, on occasion the voting tests would unstake too much and then not be able to proceed.

b1bart added 21 commits March 1, 2018 14:15
… processing to reflect the per-account pending
…vel" concepts like transactions from the basic resource interface to allow for future things that are outside the limited contexts offered by those types to affect resource consumption.

add producer voted configuration (unexposed so far) for most of the magic numbers that were part of the system.
 - Database row usage
 - Accounts
 - Permissions
 - Permission Links
 - Code
 - ABI
 - Deferred Transactions

Fixed a few merge-issues from the prior merge
…s use

 - fixed accounting for actions in the transaction cpu usage
 - fixed names on some globals and made them configurable as part of chain_config
 - fixed more test cases, removed now unecessary misc_test for producer median voting (it's in contract code now)
 - updated overhead for per-row after spot review
 - restored linear decay of the averaging when there are gaps in the incoming data.  This is used to allow account usage to decay during missed blocks as it did previously
 - added comments to the calling code that explain the semantics of ordinal selection
 - added accessors for budgeting and tests
 - fixed math to behave like the old algorithm while maintaining the new continuity across window size changes
… to match the rest of the system and imply its multiplier. Additionally changed to a VLQ encoded unsigned int.

- change name and type of commitment to the cpu usage for the context_free portion of a transaction to match the rest of the system and imply its multiplier.  Additionally changed to a VLQ encoded unsigned int.
- exposed VLQ encoded unsigned int and ZigZag encoded signed integer types to the ABI built-ins as "varuint32" and "varint32" respectively
- Fixed the unit tests that broke as a result of the above
… accounted for

- add test cases for ram and block limits
… are failing because they are actually assigning resources to people
…s are straightened out... reverted tests to the minimal change to make them work
@wanderingbort wanderingbort added this to the RC1 milestone Mar 30, 2018
@@ -300,12 +308,13 @@ transaction_trace chain_controller::_push_transaction(const packed_transaction&

time_point_sec execute_after = head_block_time();
execute_after += time_point_sec(delay);
deferred_transaction dtrx(context.get_next_sender_id(), config::system_account_name, execute_after, trx);
//TODO: !!! WHO GETS BILLED TO STORE THE DELAYED TX?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

who gets billed for storing auto-boxed transactions? the authorizer with the highest delay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bill to highest delay

@@ -1449,6 +1474,28 @@ void chain_controller::update_signing_producer(const producer_object& signing_pr
} );
}

void chain_controller::update_permission_usage( const transaction_metadata& meta ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was refactored out of the old update_usage method and made independent.

@@ -29,7 +29,6 @@ namespace eosio { namespace chain {
shared_vector<char> abi;

void set_abi( const eosio::chain::contracts::abi_def& a ) {
// Added resize(0) here to avoid bug in boost vector container
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 should restore this comment... not sure why I took it out

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean.. am I the only one who is seriously bothered by this? @PaulCalabrese did we ever ask boost folks about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are not the only one... but I can confirm that taking it out -OR- moving it all into the create call causes tests to fail in that same odd bug. I had tried to at least consolidate the abi and the code "hack" into the same logical place but it seems there is some corruption that can occur between create/constructor and future calls.

@@ -94,7 +95,8 @@ namespace eosio { namespace chain { namespace contracts {
table_id t_id;
uint64_t primary_key;
SecondaryKey secondary_key;
account_name payer;
account_name payer = 0;
uint16_t billed_overhead = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Billed overhead should be removed, it is not in use

 - added CFA cpu usage discount
 - added max transaction net and cpu usage
 - refactored trace data structures to resolve cyclic dependencies in header files
 - refactored accounting code in chain controller for a little more clarity
 - refactored lock processing and put them in the associated block/transaction traces
… less than the setcode penalty... the smartest contract is the one that does not have to exist

- fixed incorrect usage of flat_set::erase
@@ -262,7 +262,7 @@ namespace eosiosystem {
act.owner = del.from;
transaction out( now() + refund_delay + refund_expiration_time );
out.actions.emplace_back( permission_level{ del.from, N(active) }, self, N(refund), act );
out.send( del.from, now() + refund_delay );
out.send( del.from, 0, now() + refund_delay );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove implicit self pay in send deferred

@@ -28,35 +28,36 @@ namespace eosio {
:expiration(exp),region(r)
{}

void send(uint64_t sender_id, time delay_until = 0) const {
void send(uint64_t sender_id, account_name payer = account_name(0), time delay_until = 0) const {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

change to current_receiver()

@@ -222,6 +225,10 @@ void apply_context::execute_deferred( deferred_transaction&& trx ) {
FC_ASSERT( trx.execute_after < trx.expiration, "transaction expires before it can execute" );
FC_ASSERT( !trx.actions.empty(), "transaction must have at least one action");

if (trx.payer != receiver) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

or sender is privileged.

if( (delta > 0) && payer != account_name(receiver) ) {
require_authorization( payer );
if( (delta > 0) ) {
if (payer != account_name(receiver)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add or is privileged

@@ -405,7 +417,8 @@ int apply_context::db_store_i64( uint64_t code, uint64_t scope, uint64_t table,
++t.count;
});

update_db_usage( payer, buffer_size + 200 );
int64_t billable_size = (int64_t)(buffer_size + sizeof(key_value_object) + config::overhead_per_row_ram_bytes);
update_db_usage( payer, billable_size, "New Row ${id} in (${c},${s},${t})", _V("id", obj.primary_key)("c",receiver)("s",scope)("t",table));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for RC2 we need to worry about overhead for the variant construction

@@ -57,6 +57,10 @@ struct shared_authority {
for( const auto& a : accounts ) { auth.accounts.emplace_back( a ); }
return auth;
}

size_t get_billable_size() const {
return keys.size() * sizeof(key_weight) + accounts.size() * sizeof(permission_level_weight);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

account for compilers having different sizes for these and the database structures.

});
}

void resource_limits_manager::add_transaction_usage(const vector<account_name>& accounts, uint64_t cpu_usage, uint64_t net_usage, uint32_t time_slot ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new invariant, bill unique authorizers equally ... don't double up

* @param cpu_weight - the weight for determining share of compute capacity
*/
void set_resource_limits( account_name account, int64_t ram_bytes, int64_t net_weight, int64_t cpu_weight) {
EOS_ASSERT(ram_bytes >= -1 && ram_bytes <= INT64_MAX, wasm_execution_error, "invalid value for ram resource limit expected [-1,INT64_MAX]");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

INT64_MAX is not an effective test on an int64_t

account_name actual_payer = payer;
if (actual_payer != account_name(0)) {
const auto* paying_account = context.db.find<account_object, by_name>(payer);
EOS_ASSERT(paying_account, tx_unknown_argument, "The account for the payer: ${a}, does not exist!", ("a", payer));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove this in favor of failing when trying to bill

@@ -300,12 +308,13 @@ transaction_trace chain_controller::_push_transaction(const packed_transaction&

time_point_sec execute_after = head_block_time();
execute_after += time_point_sec(delay);
deferred_transaction dtrx(context.get_next_sender_id(), config::system_account_name, execute_after, trx);
//TODO: !!! WHO GETS BILLED TO STORE THE DELAYED TX?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bill to highest delay

@bytemaster
Copy link
Contributor

I went over this with bart, looks good, outstanding issues can be addressed in future pull request.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DAWN-472 ⁃ Bandwidth Rate Limiting & Storage Usage Limits
5 participants