Skip to content
New issue

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

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

Already on GitHub? # to your account

Etcm 139 incomplete data errors during fast sync #748

Merged
merged 2 commits into from
Oct 29, 2020

Conversation

kapke
Copy link
Contributor

@kapke kapke commented Oct 20, 2020

Description

Report custom eth error "100 missing data" when getting data from account

Proposed Solution

implementation of https://eth.wiki/json-rpc/json-rpc-error-codes-improvement-proposal (at least in account-touching methods)

Important Changes Introduced

Reorganized a bit json serialization. Due to nasty error I run into at some point (json4s and its formats <3) I wanted to plug Encoder-based serialization into rpc servers, due to multiple issues I stopped that and got into working state

Testing

Call methods like eth_getBalance during fast-sync - it should return error with additional information that state node is missing

@kapke kapke added the BREAKS API Affects services that interacts with APIs label Oct 20, 2020
@kapke kapke requested review from mmrozek and lemastero October 20, 2020 15:11
@kapke kapke self-assigned this Oct 20, 2020
@kapke kapke removed the BREAKS API Affects services that interacts with APIs label Oct 20, 2020
Copy link
Contributor

@mmrozek mmrozek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments only

.getOrElse(Account.empty(blockchainConfig.accountStartNonce))
)
}
}.recover { case _: MissingNodeException =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we could get MissingNodeException?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly during fast-sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could happen during regular one if state node is lost along the way, but it's much less probable while during fast-sync it is sure that such error happens.

private def withAccount[T](address: Address, blockParam: BlockParam)(f: Account => T): ServiceResponse[T] =
Future {
resolveBlock(blockParam).map { case ResolvedBlock(block, _) =>
f(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: I would like to name this function toResponse or sth similar and reformat the code to

val account = blockchain
            .getAccount(address, block.header.number)
            .getOrElse(Account.empty(blockchainConfig.accountStartNonce))
toResponse(account)

it is a little bit more readable

@kapke kapke force-pushed the etcm-139-incomplete-data-during-fast-sync branch 2 times, most recently from 3aff6e9 to e8201e1 Compare October 28, 2020 09:29
Copy link
Contributor

@lemastero lemastero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@kapke kapke force-pushed the etcm-139-incomplete-data-during-fast-sync branch from e8201e1 to b3165d2 Compare October 28, 2020 14:49
@kapke kapke force-pushed the etcm-139-incomplete-data-during-fast-sync branch 2 times, most recently from 126018c to ba8f975 Compare October 29, 2020 09:14
@kapke kapke force-pushed the etcm-139-incomplete-data-during-fast-sync branch from ba8f975 to 75c74ce Compare October 29, 2020 09:22
@kapke kapke merged commit c02157d into develop Oct 29, 2020
@kapke kapke deleted the etcm-139-incomplete-data-during-fast-sync branch October 29, 2020 10:06
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants