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
Handle null blocks from Lotus #5294
base: master
Are you sure you want to change the base?
Conversation
483cea4
to
641a235
Compare
e2b503b
to
94b9513
Compare
94b9513
to
31cd36d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see changes made to make sure we don't introduce additional overhead for existing chains.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that is right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right.
No description provided.