Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Update ref to parity-common and update seek behaviour #9257

Merged
merged 2 commits into from
Aug 1, 2018

Conversation

ngotchac
Copy link
Contributor

This PR: paritytech/parity-common#13 updates the behavior of seek in TrieDBIterators.
This PR updates the reference to parity-common and updates the behavior of seek which places the iterator on the given element (if existing) or the closest next one.

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 31, 2018
@ngotchac ngotchac requested a review from dvdplm July 31, 2018 12:56
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

LGTM. There'll be a follow-up PR that removes the branch = … after PR #13 is merged yes?

@ngotchac
Copy link
Contributor Author

@dvdplm That's right!

@@ -1692,6 +1692,9 @@ impl BlockChainClient for Client {
if let Some(after) = after {
if let Err(e) = iter.seek(after) {
trace!(target: "fatdb", "list_accounts: Couldn't seek the DB: {:?}", e);
} else {
// Position the iterator after the `after` element
iter.next();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain how this works? Why calling next after seek returned an Err would position the iterator after the after?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the else branch is actually when there has been no errors

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, right, thanks for clarifying that. So we assume that after is present in the db, otherwise this is not correct?

Copy link
Contributor Author

@ngotchac ngotchac Jul 31, 2018

Choose a reason for hiding this comment

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

So in this context, after is either None or an address that is actually in DB.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could be simplified by writing:

if let Err(e) = iter.next(after) {
  trace!("...");
}

Cause Err is no used later on anyway

@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 31, 2018
Cargo.toml Outdated
@@ -33,7 +33,7 @@ fdlimit = "0.1"
ctrlc = { git = "https://github.com/paritytech/rust-ctrlc.git" }
jsonrpc-core = { git = "https://github.com/paritytech/jsonrpc.git", branch = "parity-1.11" }
ethcore = { path = "ethcore", features = ["parity"] }
parity-bytes = { git = "https://github.com/paritytech/parity-common" }
parity-bytes = { git = "https://github.com/paritytech/parity-common", branch = "ng-fix-triedb-seek" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just an idea. To get "atomic" PRs, we can make parity-common repo a submodule of this repo. A submodule is pinned to a commit, so we can avoid always manually changing branches. (And it also makes browsing all ethereum-related code easier!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if we just specify a commit hash for parity-common? Should be easier to implement, and should be safe.

Copy link
Collaborator

@debris debris left a comment

Choose a reason for hiding this comment

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

lgtm, please just fix remove branch = from .toml files :)

@@ -1692,6 +1692,9 @@ impl BlockChainClient for Client {
if let Some(after) = after {
if let Err(e) = iter.seek(after) {
trace!(target: "fatdb", "list_accounts: Couldn't seek the DB: {:?}", e);
} else {
// Position the iterator after the `after` element
iter.next();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could be simplified by writing:

if let Err(e) = iter.next(after) {
  trace!("...");
}

Cause Err is no used later on anyway

@debris debris added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Aug 1, 2018
@ngotchac
Copy link
Contributor Author

ngotchac commented Aug 1, 2018

Hm, the error is used in the trace log, and next doesn't give an error

@debris
Copy link
Collaborator

debris commented Aug 1, 2018

ah, you are totally right! I confused seek with peek 🤦‍♂️

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Aug 1, 2018
@ngotchac ngotchac merged commit c224980 into master Aug 1, 2018
@ngotchac ngotchac deleted the ng-update-seek branch August 1, 2018 16:03
ordian added a commit to ordian/parity that referenced this pull request Aug 2, 2018
…rp_sync_on_light_client

* 'master' of https://github.com/paritytech/parity:
  Propagate transactions for next 4 blocks. (openethereum#9265)
  decode block rlp less often (openethereum#9252)
  Fix eternalities tests can_create (missing parameter) (openethereum#9270)
  Update ref to `parity-common` and update `seek` behaviour (openethereum#9257)
  Comply EIP-86 with the new definition (openethereum#9140)
  Check if synced when using eth_getWork (openethereum#9193) (openethereum#9210)
@5chdn 5chdn added this to the 2.1 milestone Aug 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants