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

chainbase uniqueness violation fixes #7266

Merged
merged 6 commits into from
May 6, 2019

Conversation

arhag
Copy link
Contributor

@arhag arhag commented May 4, 2019

Change Description

This PR updates the chain code to reflect the new expected chainbase requirements reflected in EOSIO/chainbase#44 (it also updated the chainbase submodule to reflect the code code changes from that PR).

The best way to satisfy those requirements (and other ones we care about like deterministic behavior) in the EOSIO code is to satisfy the following restrictions when dealing with chainbase objects:

  • The chainbase object includes a field called id which has the type id_type.
  • The multi-index must include an ordered_unique index tagged with by_id that is based on the id field as the sole key.
  • No other types of indices other than ordered_unique are allowed. If an index is desired that does not enforce uniqueness, then use a composite key that ends with the id field.
  • When creating a chainbase object, the constructor lambda should never mutate the id field.
  • When modifying a chainbase object, the modifier lambda should never mutate any fields in the restricted field set. (new requirement)

A field of a chainbase object is considered to be in the restricted field set if it is involved in the derivation of the key used for one of the indices in the chainbase multi-index unless its only involvement is through being included in composite_keys that end with the id field. The id field is always included in the restricted field set.

So if the multi-index includes an index like the following

    ordered_unique< tag<by_sender_id>,
       composite_key< generated_transaction_object,
          BOOST_MULTI_INDEX_MEMBER( generated_transaction_object, account_name, sender),
          BOOST_MULTI_INDEX_MEMBER( generated_transaction_object, uint128_t, sender_id)
       >
    >

both sender and sender_id fields are part of the restricted field set.

On the other hand, an index like the following

   ordered_unique< tag<by_expiration>,
      composite_key< generated_transaction_object,
         BOOST_MULTI_INDEX_MEMBER( generated_transaction_object, time_point, expiration),
         BOOST_MULTI_INDEX_MEMBER( generated_transaction_object, generated_transaction_object::id_type, id)
      >
   >

would not by itself require the expiration field to be part of the restricted field set.

In order to satisfy this new requirement, it was necessary to change the by_permission_name of the permission_link_index to switch from a composite key of the tuple (account, required_permission, code, message_type) to a composite key of the tuple (account, required_permission, id). This change is acceptable because only the first two fields of the composite key were used for lookups; the remaining fields were only there to add uniqueness to the index which is better handled by the addition of the id field. However, this change to permission_link_index definition means that a nodeos incorporating these changes will not be compatible with the shared_memory.bin state file generated by v1.8.0-rc1 nodeos.

Satisfying the new requirement also requires doing something about the handling of the trx_id field of generated_transaction_object because of the by_trx_id index of generated_transaction_multi_index. Instead of modifying the index definition, the easier solution to satisfying the new requirement was to avoid mutating the restricted trx_id field in a chainbase modifier lambda. This means in order to satisfy the requirements of the replace existing deferred transaction mechanism available to contracts, it was necessary to replace the modification of generated_transaction_object with a removal followed by a creation.

This PR also adds a new unit test, api_tests/more_deferred_transaction_tests, which tests out a few of the various ways that replacing a deferred transaction could have been used to trigger the various chainbase failures due to uniqueness violations. This includes the modify and create pattern (case 3 discussed in EOSIO/chainbase#32) which is resolved due to swap of undo processing order in EOSIO/chainbase#44, and the swap unique keys via modifies pattern (case 4 discussed in EOSIO/chainbase#32) which is avoided by following the restrictions above. The deferred_test test contract has been updated to include two new actions to fulfill the testing needs of the new unit test.

Older versions of nodeos prior to v1.8.0-rc1 are protected from the schemes demonstrated in the new unit test causing nodeos failure partly because of the subjective mitigation introduced in v1.0.6 that disabled deferred transaction replacement for reasons unrelated to this issue, but also because even prior to the introduction of that subjective mitigation, there was a bug since v1.0.0 that led to the trx_id field remaining stale during deferred transaction replacement which neutralized the critical component of carrying out the scheme demonstrated in the new unit test. Even v1.8.0-rc1 is not vulnerable to this issue until the REPLACE_DEFERRED protocol feature, which among other things fixes the stale trx_id bug and disables the subjective mitigation preventing deferred transaction replacement, is activated. Coincidentally, even with REPLACE_DEFERRED activated, it no longer becomes possible to exploit the uniqueness constraint issue if the NO_DUPLICATE_DEFERRED_ID is also activated.

This PR also removed the unused producer_object.hpp file.

Special thanks to @raymondnuaa for first reporting this issue in chainbase through the PR EOSIO/chainbase#32 and describing various scenarios of relevance in the PR description.

Consensus Changes

  • Consensus Changes

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

arhag added 3 commits May 3, 2019 21:22
…t violate the new (and now explicitly stated) requirements imposed on chainbase objects that should be respected in this code base; following those new requirements will automatically satisfy the critical preconditions on the chainbase modify function
…k that certain deferred transaction replacement schemes do not cause chainbase failure any more

Modified the deferred_test test contract to support the new unit test.
@arhag arhag marked this pull request as ready for review May 4, 2019 04:58
@arhag arhag merged commit 5525345 into release/1.8.x May 6, 2019
@arhag arhag deleted the chainbase-uniqueness-violation-fixes branch May 6, 2019 15:04
@arhag arhag mentioned this pull request May 6, 2019
3 tasks
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.

2 participants