Introduce Rlp::at_with_offset method.#269
Conversation
|
Could you add this description as the method docs and submit a separate PR for new version release with changelog updates (https://github.com/paritytech/parity-common/blob/master/CONTRIBUTING.md#releasing-a-new-version), thanks! |
dvdplm
left a comment
dvdplm
left a comment
There was a problem hiding this comment.
paritytech/trie#34 seems like a pretty major improvement, congrats; would love to see it used in eth asap.
| Ok(rlp) | ||
| } | ||
|
|
||
| pub fn at_with_offset<'view>(&'view self, index: usize) |
There was a problem hiding this comment.
What is the purpose of adding this as a new method rather than changing the return value of at()? Just to reduce the breakage in parity-ethereum? If yes then I think I'd prefer to do a breaking release of rlp and do the grunt work.
There was a problem hiding this comment.
Well, most of the time you don't need the offset, so even if designing the API from scratch I think having two separate methods probably makes sense. At the moment this is only used for rlp-encoded trie nodes.
But yeah, probably the main reason is backwards compatibility. Just doesn't seem like there's a good reason to break the current API, IMO.
There was a problem hiding this comment.
most of the time you don't need the offset,
In your eth branch it's used 4 times and only for RlpNodeCodec so I guess you're right. at() is used much more extensively, which kind of surprised me: I'd have hoped it was used much less.
jimpo
force-pushed
the
jimpo/at-with-offset
branch
from
3fcb53a to
46b7272
Compare
* master: Compile triehash for no_std (#280) [kvdb-rocksdb] Use "pinned" gets to avoid allocations (#274) [kvdb-rocksdb] Release 0.2 (#273) [kvdb-rocksdb] switch to upstream (#257) travis: try to fix wasmpack chrome test on macOS (#263) Use 2018 edition for rustfmt (#266) [fixed-hash]: re-export `alloc_` (#268) kvdb-web: async-awaitify (#259) kvdb-rocksdb: configurable memory budget per column (#256) Bump rlp crate version. (#270) Introduce Rlp::at_with_offset method. (#269) Make fixed-hash test structs public (#267) Migrate primitive types to 2018 edition (#262) upgrade tiny-keccak to 2.0 (#260)
3 participants
This is needed to update parity-ethereum's Patricia Trie to version 0.16.0 of the
trie-dbcrate.NodeCodecintrie-dbwas changed in paritytech/trie#34 so that encoded nodes can be decoded into aNodePlanwhich specifies the slice indices of data contained within the encoded node instead of just the slices themselves. This allows for a cleaner and faster trie iteration interface.Since parity-ethereum uses an rlp-based codec for trie nodes unlike Substrate and the reference trie, a method to get a list item with its slice offset is required. I have written a prototype implementation here (note that the grossness of having multiple rlp versions in the same crate would go away).