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

Update to latest kvdb-*: no default column, DBValue is Vec#11312

Merged
dvdplm merged 15 commits into
masterfrom
dp/chore/kvdb-no-default-column
Dec 20, 2019
Merged

Update to latest kvdb-*: no default column, DBValue is Vec#11312
dvdplm merged 15 commits into
masterfrom
dp/chore/kvdb-no-default-column

Conversation

@dvdplm
Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm commented Dec 6, 2019 •
edited
Loading

This PR started as an update in parity-ethereum to use the new kvdb API that does not expose the legacy "default" column anymore. In code terms, we no longer specify columns with an Option<u32>, but use a u32 instead.

The new kvdb-* versions also changes the type of DBValue from an ElasticArray128<u8> to be simply Vec<u8> and this has caused some significant scope-drift of this PR as it was necessary to make both of these rather noisy changes at once.

Notes for reviewers:

  1. Database columns no longer use Option<u32>; anything that was Some<123> is now 123 and None is no longer an allowed value. Smaller and less complex API.
  2. kvdb::DBValue is now a Vec<u8> which is kind of nice in different ways: a little less cloning, a little nicer code here and there, a few less dependencies on kvdb, less unsafe code.
  3. As a consequence of 1), it was necessary to add a migration for secret-store (the only component of parity-ethereum that used the "default" column, i.e. None) and provide a means to run that migration before we landed this PR. This migration code, from PR #11322, is removed here and lives on only in the 2.6.7 release. Users with secret-store databases are expected to run the migration using that version and then proceed with further upgrades. @grbIzl will revive the migration code as part of his on-going work to extract secret-store from the main source tree.
  4. The DBValue from kvdb is mirrored in trie-db, which is unfortunate. This means that changing the type in one requires changing the other. This explains the need to upgrade those crates as well.

It bears repeating for posterity:

Note: After this PR lands, users of secretstore must use parity-ethereum v2.6.7 to migrate their data and then upgrade to more recent versions. This is unfortunately necessary as the new version of kvdb-rocksdb does not expose a way to access the "default" column on databases and so legacy data must be moved from the "default" column to column 0.

@dvdplm
Only use kvdb "column families"
714c0b9 
This PR contains the changes necessary to use the `kvdb-*` crates from paritytech/parity-common#278 (so a synchronized merge is required) which drops support for the old-style rocksdb "default" column to get a smaller and less complex API.

As it stands this PR is working correctly except for secret-store; we need to migrate it to use a new column family.
@dvdplm dvdplm self-assigned this Dec 6, 2019
@dvdplm dvdplm added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-io 💾 Interaction with filesystem/databases. labels Dec 6, 2019
ordian
ordian reviewed Dec 6, 2019
View reviewed changes
Comment thread parity/db/rocksdb/helpers.rs Outdated
ordian
ordian reviewed Dec 6, 2019
View reviewed changes
Comment thread parity/db/rocksdb/helpers.rs Outdated
dvdplm added 7 commits December 9, 2019 15:57
@dvdplm
Merge branch 'master' into dp/chore/kvdb-no-default-column
bf1bfea 
* master:
  Istanbul activation on xDai (#11299)
  Istanbul activation on POA Core (#11298)
  Adds support for ipc socket permissions (#11273)
  Add check for deserialising hex values over U256 limit (#11309)
  validate-chainspecs: check istanbul eips are in the foundation spec (#11305)
@dvdplm
Fix secretstore build
c1528d2
@dvdplm
Fix secretstore build: include ethkey when building with the "account…
1a1173a 
…s" feature
@dvdplm
typos
9cb5bfc
@dvdplm
Remove SecretStore migration code and invite users to use 2.6 to migr…
c674b09 
…ate their data instead

Merge branch 'master' into dp/chore/kvdb-no-default-column

* master:
  tx-q: enable basic verification of local transactions (#11332)
  remove null signatures (#11335)
  ethcore/res: activate agharta on classic 9573000 (#11331)
  [secretstore] migrate to version 4 (#11322)
  Enable EIP-2384 for ice age hard fork (#11281)
  Fix atomicity violation in network-devp2p (#11277)
@dvdplm
Restore state test commit
41ae23c
@dvdplm
Override all of parity-common from git
341eb2d
@dvdplm dvdplm marked this pull request as ready for review December 15, 2019 14:16
@dvdplm dvdplm requested a review from ordian December 15, 2019 14:16
@dvdplm dvdplm added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Dec 15, 2019
grbIzl
grbIzl previously approved these changes Dec 18, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@grbIzl grbIzl left a comment

Choose a reason for hiding this comment

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

Looks good

niklasad1
niklasad1 reviewed Dec 18, 2019
View reviewed changes
Comment thread ethcore/db/src/db.rs Outdated
@ordian
Copy link
Copy Markdown
Member

ordian commented Dec 18, 2019

(Looks good, but it should be put on ice till we release a new version of kvdb & co)

niklasad1
niklasad1 reviewed Dec 18, 2019
View reviewed changes
Comment thread ethcore/db/src/db.rs Outdated
niklasad1
niklasad1 reviewed Dec 18, 2019
View reviewed changes
Comment thread ethcore/db/src/db.rs Outdated
niklasad1
niklasad1 reviewed Dec 18, 2019
View reviewed changes
Comment thread ethcore/db/src/db.rs Outdated
niklasad1
niklasad1 reviewed Dec 18, 2019
View reviewed changes
Comment thread ethcore/db/src/db.rs
niklasad1
niklasad1 reviewed Dec 18, 2019
View reviewed changes
Comment thread parity/db/rocksdb/helpers.rs
@niklasad1
Copy link
Copy Markdown
Collaborator

niklasad1 commented Dec 18, 2019

This is unfortunately necessary as the new version of kvdb-rocksdb does not expose a way to access the "default" column on databases and so legacy data must be moved from the "default" column to column 0.

default means Option::None? (which is changed to 0?)
Then column 0 → 1?

niklasad1
niklasad1 reviewed Dec 18, 2019
View reviewed changes
Comment thread util/migration-rocksdb/src/lib.rs Outdated
niklasad1
niklasad1 reviewed Dec 18, 2019
View reviewed changes
Comment thread util/migration-rocksdb/src/lib.rs
@dvdplm
Copy link
Copy Markdown
Collaborator Author

dvdplm commented Dec 18, 2019

default means Option::None? (which is changed to 0?)
Then column 0 → 1?

"default" columns does mean Option::None but it was not changed to 0 here (or elsewhere). In other words, before this PR one could have a database with zero columns and only use the "default" column. That is so confusing and awkward that we're getting rid of the default column from the kvdb API. This PR is what that looks like in eth.

@dvdplm @niklasad1
Update ethcore/db/src/db.rs
32e8685 
Co-Authored-By: Niklas Adolfsson <niklasadolfsson1@gmail.com>
@niklasad1
Copy link
Copy Markdown
Collaborator

niklasad1 commented Dec 18, 2019

"default" columns does mean Option::None but it was not changed to 0 here (or elsewhere). In other words, before this PR one could have a database with zero columns and only use the "default" column. That is so confusing and awkward that we're getting rid of the default column from the kvdb API. This PR is what that looks like in eth.

All right, then it makes sense

@dvdplm
Address review grumbles
a47efd8
@dvdplm
Merge branch 'dp/chore/kvdb-no-default-column' of github.com:parityte…
f7d9a77 
…ch/parity-ethereum into dp/chore/kvdb-no-default-column

* 'dp/chore/kvdb-no-default-column' of github.com:paritytech/parity-ethereum:
  Update ethcore/db/src/db.rs
@dvdplm
Review grumbles
d496b03
@dvdplm dvdplm requested a review from niklasad1 December 19, 2019 11:10
niklasad1
niklasad1 previously approved these changes Dec 19, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

good work David, looks good but don't forget to remove patches and update Cargo.lock before merging

@dvdplm
Merge branch 'master' into dp/chore/kvdb-no-default-column
ad0bf92 
* master:
  Add Nat PMP method to P2P module (#11210)
  Add randomness contract support to AuthorityRound. (#10946)
  ethcore/res: activate ecip-1061 on kotti and mordor (#11338)
@dvdplm dvdplm requested review from grbIzl and niklasad1 December 19, 2019 23:29
@dvdplm dvdplm changed the title Only use kvdb "column families" Update to latest kvdb-*: no default column, DBValue is Vec Dec 19, 2019
@dvdplm dvdplm mentioned this pull request Dec 19, 2019
Closed
@dvdplm dvdplm dismissed stale reviews from niklasad1 and grbIzl December 19, 2019 23:33

stale

ordian
ordian approved these changes Dec 19, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Looks much nicer!

Comment thread Cargo.lock
source = "registry+https://github.com/rust-lang/crates.io-index"
dependencies = [
"parity-scale-codec 1.0.5 (registry+https://github.com/rust-lang/crates.io-index)",
"parity-scale-codec 1.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
Copy link
Copy Markdown
Member

@ordian ordian Dec 19, 2019

Choose a reason for hiding this comment

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

I've noticed we're using this even though we probably shouldn't (this comes as part of default features of primitive-types), but I guess it's not substantial.

Copy link
Copy Markdown
Collaborator Author

@dvdplm dvdplm Dec 20, 2019

Choose a reason for hiding this comment

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

I've seen it too, it's been in there for a while now. :/

Copy link
Copy Markdown
Collaborator

@niklasad1 niklasad1 Dec 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

codec has been included in the default features for a while now,

paritytech/parity-common@7989816#diff-7b2119e2b4c3e0074b53b88c21461b62

EDIT: I guess would could fix it ethereum-types

@dvdplm dvdplm merged commit b9f9d11 into master Dec 20, 2019
@niklasad1 niklasad1 deleted the dp/chore/kvdb-no-default-column branch December 20, 2019 12:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@grbIzl grbIzl Awaiting requested review from grbIzl

2 more reviewers

@ordian ordian ordian approved these changes

@niklasad1 niklasad1 niklasad1 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dvdplm dvdplm

Labels

A0-pleasereview 🤓 Pull request needs code review. M4-io 💾 Interaction with filesystem/databases.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dvdplm @ordian @niklasad1 @grbIzl