You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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_CRITthrowstd::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.
The text was updated successfully, but these errors were encountered:
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 mutatingnative_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 theblock_conversion_plugin_impl
) is a last irreversible block ID (actually make it an optional and call itnative_lib_id
) and also the block number of the last irreversible EVM block (call itevm_lib_num
).This approach also allows us to remove the need for the hack in
load_head
where a fakenative_block
is created in which only itsid
,block_num
, andtimestamp
are set to reconstructed values; and in the case oftimestamp
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 tonative_blocks
. It would not necessarily need to setnative_lib_id
andevm_lib_num
member variables either; it can leave them as their defaults of anstd::nullopt
fornative_lib_id
and 0 forevm_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 innative_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 theinit
action was called. Prior to the genesis timestamp,native_lib_id
would not be needed; note there would be no need to add blocks tonative_blocks
if they are prior to thegenesis_timestamp
. Andnative_lib_id
would be set to the ID of an irreversible native block prior to reaching the block in whichinit
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 thenative_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 fromnative_blocks
andevm_blocks
respectively:An additional optimization would be to avoid adding
*new_block
tonative_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 setnative_lib_id
andevm_lib_num
separately from*new_block
in that case though. That waynative_blocks
would remain empty during most of that sync and only be added once the sync catches up to the most recent reversible blocks.The text was updated successfully, but these errors were encountered: