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

Improvements to irreversible native block handling in block_conversion_plugin #442

Open
arhag opened this issue Apr 10, 2023 · 1 comment
Milestone

Comments

@arhag
Copy link
Member

arhag commented Apr 10, 2023

The block_conversion_plugin currently attempts to maintain the last irreversible native block in the native_blocks list. As long as that invariant is satisfied, the logic within the handler of the native_blocks channel should work correctly. There are some places where we could be more defensive in checks after mutating native_blocks to ensure that invariant is maintained.

However, I think the code would be cleaner if we changed the invariant to not include irreversible blocks (including the last one) in native_blocks. Instead what is really required to keep as state (within the block_conversion_plugin_impl) is a last irreversible block ID (actually make it an optional and call it native_lib_id) and also the block number of the last irreversible EVM block (call it evm_lib_num).

This approach also allows us to remove the need for the hack in load_head where a fake native_block is created in which only its id, block_num, and timestamp are set to reconstructed values; and in the case of timestamp is set potentially incorrectly (though in a way that is benign to how it is used in block_conversion_plugin) since it loses the sub-second resolution of the original native block.

Note that load_head would also need to be modified to not push any block to native_blocks. It would not necessarily need to set native_lib_id and evm_lib_num member variables either; it can leave them as their defaults of an std::nullopt for native_lib_id and 0 for evm_lib_num which would be appropriate when trying to synchronize a fresh node for the first time to a network. Both values would be set to the correct value automatically as soon as the information to set that appropriately (i.e. an irreversible native block) becomes available. native_lib_id is only needed when a new block cannot build off any of the blocks in native_blocks; in that case it serves as a check that the new block's previous ID matches *native_lib_id to ensure proper linkage.

But what if in that scenario in which the new block cannot build off any of the blocks in native_blocks, native_lib_id also had no value? That would mean the block_conversion_plugin has not yet encountered a native block that it knows is irreversible. This wouldn't be a problem if syncing irreversible native block prior to the block in which the init action was called. Prior to the genesis timestamp, native_lib_id would not be needed; note there would be no need to add blocks to native_blocks if they are prior to the genesis_timestamp. And native_lib_id would be set to the ID of an irreversible native block prior to reaching the block in which init was called. But there could be a problem in the case in which the node starts up syncing from a later block (as would normally be the case after the network is bootstrapped). The block_conversion_plugin needs to somehow know the block number of the more recent EVM block that it knows is irreversible so that it could recover from its mixHash the block ID of the (necessarily irreversible) native block from which it was generated; that recovered block ID is then set as the native_lib_id and also tells the node from which starting point to ask SHiP for additional blocks. This could be a potential solution to eosnetworkfoundation/eos-evm-node#39; but I will not go into it further here since this issue is not meant to fix that problem.

The end of the native_blocks channel handler could then have code that looks something like the following to properly purge the irreversible native and EVM blocks from native_blocks and evm_blocks respectively:

auto new_native_lib_num = new_block->lib;
if (!native_lib_id || utils::to_block_num(*native_lib_id) < new_native_lib_num) {
   while (!native_blocks.empty() && native_blocks.front().block_num <= new_native_lib_num) {
      native_lib_id = native_blocks.front().id;
      evm_lib_num = timestamp_to_evm_block_num(native_blocks.front().timestamp) - 1;
      native_blocks.pop_front();
   }
}
while (!evm_blocks.empty() && evm_blocks.front().header.number <= evm_lib_num) {
   evm_blocks.pop_front();
}
if (evm_blocks.empty()) {
   // Log error to SILK_CRIT
   throw std::runtime_error("Invariant failure: evm_blocks should never be empty");
}

An additional optimization would be to avoid adding *new_block to native_blocks in the first place if it was already known to be irreversible (e.g. when syncing from an old block); it would still be important to set native_lib_id and evm_lib_num separately from *new_block in that case though. That way native_blocks would remain empty during most of that sync and only be added once the sync catches up to the most recent reversible blocks.

@arhag arhag added this to the Post-launch milestone Apr 10, 2023
@elmato
Copy link
Contributor

elmato commented Mar 6, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

No branches or pull requests

2 participants