Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consensus upgrade to fix problems associated with replacing deferred transactions #6103

Closed
arhag opened this issue Oct 22, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@arhag
Copy link
Contributor

commented Oct 22, 2018

Background

The send_deferred intrinsic includes a bool field replace_existing which if true allows the new deferred transaction to replace any existing pending one with the same sender and sender_id (rather than failing on detecting duplicates as would be the case if replace_existing was false). This same functionality can be replicated by first calling cancel_deferred with the sender_id that will be used with the soon-to-be-sent deferred transaction and then calling send_deferred with replace_existing set to false.

There is a bug with the implementation of send_deferred which led to a subjective mitigation (#4288) to temporarily disable the ability to replace an existing deferred transaction.

The bug involved two distinct oversights with how deferred transactions were scheduled in apply_context::schedule_deferred_transaction. The first oversight was not refunding the RAM for the existing to-be-replaced deferred transaction to the original payer. The second oversight was not replacing the trx_id field of the modified generated_transaction_object to the transaction ID of the new deferred transaction.

The first oversight means that some accounts affected by this bug would currently be charged more RAM than they should. It is possible to correct the RAM usage of all accounts to the correct amount (and in fact this consensus upgrade proposes to do so).

The second oversight can lead to retiring deferred transactions in the block with a transaction ID different than the ID of the actual deferred transaction that was retired. In fact, the deferred transaction could be retired with an ID equivalent to a previously retired ID in the blockchain (this is something that was actually observed in a live EOSIO blockchain). There is nothing that can be done to remove these incorrect and perhaps duplicate IDs from the blockchain once the blocks in which they were retired have become irreversible. The best that can be done in this respect is to correct the logic so that the correct transaction IDs are used going forward when retiring deferred transactions in a block.

Consensus upgrade feature

The goal of the consensus upgrade feature described in this document is to fix the behavior of replacing a deferred transaction so that the subjective mitigation from #4288 can be removed, but also to correct the RAM usage of all accounts that may have been affected by the bug.

Correcting the RAM usage requires nodeos to track the correction to RAM usage so that the deltas can be added to the RAM usage fields of all affected accounts at the moment of the consensus upgrade feature activation. Tracking the corrections to RAM usage requires nodeos to replay the blockchain from genesis. (An alternative approach could be for the RAM usage field to instead represent the correct amount from the beginning and to use the tracked corrections to dynamically compute the incorrect old values for the purposes of maintaining the same old behavior for the resource_limits_manager::get_account_ram_usage and resource_limits_manager::verify_account_ram_usage functions prior to activation of the consensus upgrade feature. This allows the consensus upgrade feature activation to occur instantaneously without having to iterate through many accounts at the moment of activation.)

A new consensus protocol upgrade feature will be added to trigger the changes described in this consensus upgrade proposal. The actual digest for the feature understood at the blockchain level is to be determined. For the purposes of this proposal the codename REPLACE_DEFERRED will be use to stand-in for whatever the feature identifier will actually end up being.

Correcting RAM usage

The resource_limits_manager would add a new Chainbase index (with lookup by account) that stores a RAM correction accumulator. Any account that doesn't have an object in this index by the time of REPLACE_DEFERRED activation does not need any correction to their RAM usage. Otherwise, the amount in the accumulator is subtracted from the RAM usage of the account at the moment of REPLACE_DEFERRED activation.

To save memory in the Chainbase shared_memory.bin file, the index could be deleted (requires new functionality in Chainbase) after REPLACE_DEFERRED activation.

A new chain_snapshot_header version is necessary (e.g. version 2) to handle the changes of this consensus upgrade feature. Version 2 of the snapshot may include a section for the new index (it actually must include the section if REPLACE_DEFERRED has not yet been activated as of the snapshot and must not include the section if REPLACE_DEFERRED has been activated as of the snapshot). The new version of nodeos supporting the REPLACE_DEFERRED consensus upgrade feature should not be able to read a snapshot with a chain_snapshot_header version of 1 (this forces the necessary replay from genesis to occur). The new version of nodeos should support version 2 of the snapshot when reading or writing a snapshot.

Changes to apply_context::scheduled_deferred_transaction

If REPLACE_DEFERRED has not been activated:

  • Keep subjective disabling of replacing deferred transactions.
  • Add RAM usage of the to-be-replaced deferred transaction to the RAM correction accumulator of the payer of the to-be-replaced deferred transaction.
  • Add RAM usage of the new deferred transaction as it currently does.
  • Keep the same gtx.trx_id as before when modifying the generated_transaction_object.

If REPLACE_DEFERRED has been activated:

  • Avoid subjective disabling of replacing deferred transactions.
  • Subtract RAM usage of the to-be-replaced deferred transaction.
  • Add RAM usage of the new deferred transaction as it currently does.
  • When modifying the generated_transaction_object, replace gtx.trx_id with trx.id() (which is the ID of the new deferred transaction).
@arhag

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

This issue depends on #6429. It will also be setup by default to be a protocol feature requiring pre-activation, thus it also depends on #6431.

@taokayan

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

I guess it's better to handle this issue on day one of hard-fork as well, at least we can create a multi-index table to keep track of the refunding RAM, since a full hard-replay is required. Otherwise it will probably end up with an extra hard-replay.

@arhag arhag added CONSENSUS in progress and removed HARDFORK labels Mar 22, 2019

@arhag arhag referenced this issue Mar 26, 2019

Merged

Implement REPLACE_DEFERRED protocol feature #6997

1 of 3 tasks complete

@arhag arhag removed the in progress label Mar 28, 2019

@arhag

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

Resolved by #6997.

@arhag arhag closed this Mar 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.