This repository has been archived by the owner on Aug 2, 2022. It is now read-only.
fixes to contract whitelist/blacklist #3921
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also fixed error in transaction::send definition and added helper script for abi generation.
wanderingbort
suggested changes
Jun 7, 2018
libraries/chain/controller.cpp
Outdated
| push_scheduled_transaction( receipt.trx.get<transaction_id_type>(), fc::time_point::maximum(), receipt.cpu_usage_us ); | ||
| } | ||
| const transaction_receipt_header& r = pending->_pending_block_state->block->transactions.back(); | ||
| FC_ASSERT( r == static_cast<const transaction_receipt_header&>(receipt), |
There was a problem hiding this comment.
Can this be made into an EOS_ASSERT with a proper type
| @@ -75,6 +75,7 @@ namespace eosio { namespace chain { | |||
| fc::microseconds delay; | |||
| bool is_input = false; | |||
| bool apply_context_free = true; | |||
| bool cannot_subjectively_fail = false; | |||
There was a problem hiding this comment.
The double negative name is probably more confusing than an alternative like can_subjectively_fail = true/false. I realize that will invert all the usages of it but there were enough "Not cannot" checks that I got concerned
wanderingbort
approved these changes
Jun 7, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #3814.
Whitelisting/blacklisting exceptions are subjective failures. They depend on the particular configuration of the block producer considering transactions for inclusion in a block. A subjective failure should never lead to a block being produced that relies on that subjective decision making (which is not made objective by including it in the block structure as an official part of the protocol, e.g. with the CPU billing amount) because the validators would not in general able to recreate that subjective decision making.
At the moment, there are a couple places where this rule is broken in the current code.
First, since the onblock transaction is implicit which means it is not recorded as successful or not within the block, any subjective failure of the onblock action should not be allowed. However, the contract whitelist/blacklist was not respecting this rule. This means a producer who configured their whitelist/blacklists to effectively ban the
eosiocontract from executing would produce blocks that would be considered objectively invalid by all other validators. This was the issue described in #3814.Second, the whitelist/blacklist exceptions were not treated as subjective failures in
controller. This meant that whitelist/blacklist failures encountered while processing a deferred transaction would have incorrectly elevated the status of the retired transaction tosoft_failorhard_fail. A validator would have expected a different receipt status and again considered that block to be objectively invalid.These bugs are fixed in this PR.
These bugs did not affect correct validation of a chain. All validators would still correctly accept or reject blocks according to the intended protocol on v1.0.1. The problem is with the block producers potentially producing objectively bad blocks due to these bugs. However, if block producers do not use the
contract-whitelistandcontract-blacklistoptions on v1.0.1, they are still safe from producing objectively bad blocks.Since these bugs do not affect correct validation of a chain, the bug fixes in this PR (which do not modify validation logic†) are not consensus-breaking changes.
With the changes in this PR (intended to be included in v1.0.2), the contract whitelist/blacklist features should be safe for block producers to use.
This PR also includes the following changes:
transaction::sendin eosiolib to accept auint128_tsender ID rather than auint64_tone. The signature of the C intrinsic (send_deferred) thattransaction::sendwraps was already correct, so once again this is not a consensus-breaking change.validating_tester.Footnotes:
† There is an immaterial change in the validation logic within this PR. The
transaction_receipt_headers generated during the validation step are now compared against those in the signed block to ensure they are identical. This change is immaterial from the perspective of following the same protocol as before because any differences in the bits of thesetransaction_receipt_headers would have led to a very different block-signing digest used to recover the producer public key. So the block would still fail anyway due to a signature mismatch. The changes in this PR just make the validation fail earlier with a more meaningful error message.