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

Implement REPLACE_DEFERRED protocol feature #6997

Merged

Conversation

arhag
Copy link
Contributor

@arhag arhag commented Mar 26, 2019

Change Description

Resolves #6103.

This PR adds support for the REPLACE_DEFERRED protocol feature which fixes two issues that occurred under the original protocol rules when replacing a deferred transaction:

  • The transaction ID associated with the new deferred transaction remained as the one of the original deferred transaction.
  • The RAM cost for storing the old deferred transaction was not refunded to the payer.

The activation of this protocol feature will also instantaneously correct the RAM usage of any accounts affected by this bug earlier.

Finally, the subjective mitigation that prevented replacing deferred transactions will automatically be removed after activation of REPLACE_DEFERRED.

This PR introduces a new unit test protocol_feature_tests/replace_deferred_test to test the behavior of this new protocol feature. The deferred_test test contract was augmented to enable functionality to replace deferred transactions which was needed by the new unit test.

The changes in this PR also enable a section of an existing test api_tests/deferred_transaction_tests which required the replacing deferred transaction functionality to be available.

The implementation for REPLACE_DEFERRED required an addition of an index for the new account_ram_correction_object into the state database. This meant it had an impact on the chain snapshot version. Other prior changes to the expected data structures in chain snapshots which were merged into the protocol-feature-foundations branch were not accompanied with a bump in the chain snapshot version because of the anticipation that further changes would be required (for example with the changes associated with the REPLACE_DEFERRED protocol feature).

Now this changes the current supported version for chain snapshots to 2 and also increases the minimum supported version to 2, to ensure that the new snapshot version is not compatible with the original one (since replay from genesis is required to support this protocol feature). Also, one small addition is made to the protocol_state_object (the addition of the uint32_t num_supported_key_types field which is initialized to 2) because it will be useful for a future anticipated protocol feature and I don't want that future protocol feature to force another version bump for chain snapshots.

Consensus Changes

  • Consensus Changes

Makes the REPLACE_DEFERRED protocol feature available. See above description and #6103 for details.

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

… the REPLACE_DEFERRED protocol feature.

Updated the deferred_test test contract to make it possible to 
selectively replace existing deferred transaction, which was needed for 
the above unit test. 

Added disable_all_subjective_mitigations configuration option to 
controller to make it possible to write the above unit test.

Finally, enabled the part of api_tests/deferred_transaction_tests that 
deals with replacing deferred transactions.
template<>
void controller_impl::on_activation<builtin_protocol_feature_t::replace_deferred>() {
const auto& indx = db.get_index<account_ram_correction_index, by_id>();
auto itr = indx.begin();
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this line auto itr = indx.begin(); is redundant and "itr" will be hided by another "itr" in the for loop.

Copy link
Contributor

@taokayan taokayan left a comment

Choose a reason for hiding this comment

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

Looks good to me in general

@abourget
Copy link
Contributor

Couldn't we make the snapshot importer smart and fix things as it imports? Breaking with previously taken snapshot will impose much delays in much systems (just say'in :)

Also, are all these consensus-breaking changes going to be pooled into one release?

In any case, it would be very desirable to have backwards-compatible snapshot files.

@arhag
Copy link
Contributor Author

arhag commented Mar 27, 2019

@abourget:

No backwards-compatible snapshot files for the initial release introducing protocol features because we actually want everyone to replay from genesis (or import a v2 snapshot from someone who has replayed from genesis). This will hopefully not be a normal thing required with the introduction of new protocol features. But this particular one, REPLACE_DEFERRED, uses historical information in the blockchain to determine exactly how much to correct the RAM usage of accounts that were affected by the bug.

Many consensus protocol upgrades are going to be pooled into the initial release. Future ones are planned that will not make it into the first release. These will (hopefully) be much easier to deploy assuming they do not require state database data structure changes (and especially if they do not require historical information to implement).

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.

None yet

3 participants