Handle null blocks from Lotus #5294
Conversation
incrypto32
force-pushed
the
handle-null-blocks
branch
from
483cea4
to
641a235
Compare
incrypto32
force-pushed
the
handle-null-blocks
branch
from
e2b503b
to
94b9513
Compare
incrypto32
force-pushed
the
handle-null-blocks
branch
from
94b9513
to
31cd36d
Compare
| block_hash | ||
| .ok_or_else(|| anyhow!("Ethereum node is missing block #{}", block_ptr.number)) | ||
| .map(|block_hash| block_hash == block_ptr.hash_as_h256()) | ||
| Ok(canonical_block == block_ptr) |
There was a problem hiding this comment.
This check will always be true for chains that are not Lotus, right? And this check requires that we make an RPC call, i.e., introduces overhead for chains that don't need it. I would much prefer an approach where Lotus gets its own EthereumAdapter, something like
struct LotusAdapter {
inner: chain::ethereum::EthereumAdapter
}where the impl of trait EthereumAdapter just forwards calls to inner when that's appropriate and has its own Lotus-specific impl like for this method when needed. That way, existing chains don't pay a price for the additional checks that Lotus requires.
There was a problem hiding this comment.
This check will always be true for chains that are not Lotus, right?
I dont understand this, this check can be false on other chains as well i believe, and even before the PR it still needed an RPC call the difference now is that for Lotus it might need more than one call, but for other chains it will still need the one call which was present even before this PR.
Am i missing something?
| /// missing blocks in the chain store. | ||
| /// `block_hash` and offset=1 means its parent. If `root` is passed, short-circuit upon finding | ||
| /// a child of `root`. Returns None if unable to complete due to missing blocks in the chain | ||
| /// store. |
There was a problem hiding this comment.
I don't understand what 'short-circuiting' here means. Does that mean that when root is passed that the search stops if we either get to the offset-th ancestor of block_ptr or find a block whose parent is root?
There was a problem hiding this comment.
Yes that is right
There was a problem hiding this comment.
I looked at it again, and I think I misread the changes the first time around. You are right, it's not adding an RPC call, just changed how a call is handled. So looks good to me.
One minor nit is that the method name nearest_block_hash_to_number sounds like it's translating a hash to a number when it is doing the reverse, maybe calling it nearest_block_hash_for_number would be clearer.
store/postgres/src/chain_store.rs
Outdated
| from ancestors a | ||
| where a.block_offset = $2;"; | ||
| inner join ethereum_blocks b on a.block_hash = b.hash |
There was a problem hiding this comment.
This join is probably not a big deal, but couldn't it be avoided if the recursive CTE also selected the block number, i.e. became with recursive ancestors (block_hash, block_number, block_offset) .. ?
There was a problem hiding this comment.
recursive CTE also selected the block number
Not sure if im understanding this right., but we cannot do this because we have null block right?
We cannot simply do the below because there can be null blocks?
select b.parent_hash, b.block_number - 1, a.block_offset + 1
No description provided.