-
Notifications
You must be signed in to change notification settings - Fork 3.8k
More transaction context changes for resource payer feature, and adding exceptions for resource payer limits #10337
Conversation
…ng exceptions for resource payer limits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may just be a work-in-progress PR, but it is consensus breaking.
Is it the extensions check? I guess rather than taking those out, we need to check to see if the feature has been activated? |
Can you please specify which part is consensus breaking? And also, any suggestions how to implement without breaking the consensus? Thanks. |
@@ -52,6 +52,8 @@ namespace eosio { namespace chain { | |||
uint64_t max_memory_bytes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be initialized to 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
"resource payer account '${n}' has insufficient cpu resources for this transaction", | ||
("n", name(a)) | ||
("cpu_used_in_window",cpu_used_in_window) | ||
("max_user_use_in_window",max_user_use_in_window) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the specified but unused argument replacements in the string description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
libraries/chain/resource_limits.cpp
Outdated
} else { | ||
EOS_ASSERT( cpu_used_in_window <= max_user_use_in_window, | ||
tx_cpu_usage_exceeded, | ||
"authorizing account '${n}' has insufficient cpu resources for this transaction", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the specified but unused argument replacements in the string description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
"resource payer account '${n}' has insufficient net resources for this transaction", | ||
("n", name(a)) | ||
("net_used_in_window",net_used_in_window) | ||
("max_user_use_in_window",max_user_use_in_window) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the specified but unused argument replacements in the string description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
"authorizing account '${n}' has insufficient net resources for this transaction", | ||
("n", name(a)) | ||
("net_used_in_window",net_used_in_window) | ||
("max_user_use_in_window",max_user_use_in_window) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the specified but unused argument replacements in the string description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -239,9 +258,6 @@ namespace eosio { namespace chain { | |||
bool skip_recording ) | |||
{ | |||
const transaction& trx = packed_trx.get_transaction(); | |||
if( trx.transaction_extensions.size() > 0 ) { | |||
disallow_transaction_extensions( "no transaction extensions supported yet for input transactions" ); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can't just be removed. It has to remain when protocol feature for resource payer is not enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added it back conditionally.
@@ -339,13 +355,15 @@ namespace eosio { namespace chain { | |||
// NOTE: net_limit may possibly not be objective anymore due to net greylisting, but it should still be no greater than the truly objective net_limit | |||
net_limit = static_cast<uint64_t>(account_net_limit); | |||
net_limit_due_to_block = false; | |||
use_resource_payer_net_limit = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right? I thought the idea was that resource payer account would be in play if specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just used to determine which net limit is the most restrictive. The payer account remains the same regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using resource payer limits when account (and other) limits are lower, will create a backdoor for abusing limits. But, I'm not sure what exactly the requirement is. I'll apply resource payer limits only, if that is what the requirement is. Need advice.
} | ||
|
||
// Possibly lower objective_duration_limit to what the billed accounts can pay | ||
if( account_cpu_limit <= objective_duration_limit.count() ) { | ||
// NOTE: objective_duration_limit may possibly not be objective anymore due to cpu greylisting, but it should still be no greater than the truly objective objective_duration_limit | ||
objective_duration_limit = fc::microseconds(account_cpu_limit); | ||
billing_timer_exception_code = tx_cpu_usage_exceeded::code_value; | ||
use_resource_payer_cpu_limit = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right? I thought the idea was that resource payer account would be in play if specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using resource payer limits when account (and other) limits are lower, will create a backdoor for abusing limits. But, I'm not sure what exactly the requirement is. I'll apply resource payer limits only, if that is what the requirement is. Need advice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The payer account will still be the resource payer, though the more restrictive of the limits, the account limit of resource payer is applied, instead of the limit in the resource payer extension.
} else if( deadline_exception_code == resource_payer_cpu_exceeded::code_value ) { | ||
EOS_THROW( resource_payer_cpu_exceeded, | ||
"transaction was unable to complete within resource payer CPU limit ${billing_timer}us", | ||
("now", now)("deadline", _deadline)("start", start)("billing_timer", now - pseudo_start) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add additional specified args to the description string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
} FC_LOG_AND_RETHROW() | ||
|
||
BOOST_FIXTURE_TEST_CASE(resource_payer_net_validation, resource_limits_fixture) try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will need tests for when protocol feature is disabled that it will assert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated tests to cover cases before and after enabling protocol feature.
@@ -339,13 +355,15 @@ namespace eosio { namespace chain { | |||
// NOTE: net_limit may possibly not be objective anymore due to net greylisting, but it should still be no greater than the truly objective net_limit | |||
net_limit = static_cast<uint64_t>(account_net_limit); | |||
net_limit_due_to_block = false; | |||
use_resource_payer_net_limit = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just used to determine which net limit is the most restrictive. The payer account remains the same regardless.
Change Description
More transaction context changes for resource payer feature. Also, adding exceptions for resource payer limits, and updated resource limits manager.
Change Type
Select ONE:
Testing Changes
Select ANY that apply:
Consensus Changes
API Changes
Documentation Additions