-
Notifications
You must be signed in to change notification settings - Fork 2.7k
avoid key collision on child trie and proof on child trie #2209
Changes from 1 commit
f6c4bb2
878a7ae
a464936
4de73d7
be0e340
7f3a282
4d208db
6d7f355
25bcb4c
9eaef35
37d9536
67c03a2
99dbb5a
3be1802
fec73d0
20d168d
d797bd0
27096aa
3560acc
c3fc432
3e417eb
5af490f
5a8dddd
8f04f00
8051179
8eac118
a65f9f2
d495ef5
197d77a
2ea3c89
99c45ea
3553ab2
47984fe
2bf2d7b
bc7165c
21c3acf
98b2fc3
432cb10
e266dfe
7f64652
999a26e
7bbd681
87f03b7
f18e002
0eaeca0
b57319d
1f848d2
403df51
305b60a
423cfb1
a0ffa31
6e3bed7
7411146
331be51
bc2935c
5a87b6a
f40400b
9bc1ab7
c073b21
d15ca49
0736b96
9e0485d
586b50e
65d7485
95a69b2
d089693
ec69ae0
76ea14d
b2050c8
5a0cbe1
6e84810
7f5694f
5006d73
b85508e
0c14777
10c4f58
453927b
349f9a5
fc034fb
45cfbd6
4f9717e
7cb2d84
c821a08
c3ba830
acf9641
0050457
c36b91b
9d3d9e2
3db4da8
4348d70
134a4bf
5325621
97118e8
0ed7f80
8a6986a
7652de9
94c629e
7675740
90fba8a
d8c58c6
661ba2e
833e9ff
71bda12
08b3062
89f3cd5
834f52a
0445228
75add99
e70edab
b8a0cd4
1514171
a7da811
b96c523
865672d
b6d7705
b45344c
517f95c
79a07de
a027fb0
b92655c
8a875f4
fcd8bdb
3241fc4
ecac03b
10369b9
15a2967
acc94e4
7d96338
ba7bdcb
aacea85
6789641
2b37160
b1183bd
bc5653c
5ecfd19
baf89c8
0f8bff9
5a8576b
045ee32
58e6e41
70555c5
130e5e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
will be handle by full_storage order.
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ | |
|
||
use parity_codec::{Encode, Decode}; | ||
use rstd::prelude::*; | ||
use rstd::ptr; | ||
use crate::storage::well_known_keys::CHILD_STORAGE_KEY_PREFIX; | ||
#[cfg(feature = "std")] | ||
pub use impl_serde::serialize as bytes; | ||
|
@@ -190,19 +191,23 @@ impl ChildTrie { | |
/// | ||
/// This can be quite unsafe for user, so use with care (write new trie information | ||
/// as soon as possible). | ||
pub fn fetch_or_new_pending( | ||
pub fn fetch_or_new( | ||
keyspace_builder: &mut impl KeySpaceGenerator, | ||
parent_fetcher: impl FnOnce(&[u8]) -> Option<Self>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is for incoming multiple level trie ? because otherwise the only function to put here is runtime_io::child_trie no ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really, it is just to keep things generic (but yes it should be runtime_io::child_trie that is call for general srml module). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then maybe I would put a new method in externalities to have fetch_or_new without this argument ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if empty child trie can't exist then we can't do better than that I think |
||
child_trie_update: impl FnOnce(ChildTrie), | ||
parent: &[u8], | ||
) -> Self { | ||
parent_fetcher(parent).unwrap_or_else(|| { | ||
parent_fetcher(parent) | ||
.unwrap_or_else(|| { | ||
let parent = Self::prefix_parent_key(parent); | ||
ChildTrie { | ||
let ct = ChildTrie { | ||
keyspace: keyspace_builder.generate_keyspace(), | ||
root: Default::default(), | ||
parent, | ||
extension: Default::default(), | ||
} | ||
}; | ||
child_trie_update(ct.clone()); | ||
ct | ||
}) | ||
} | ||
/// Get a reference to the child trie information | ||
|
@@ -262,8 +267,77 @@ impl ChildTrie { | |
enc.extend_from_slice(&self.extension[..]); | ||
enc | ||
} | ||
|
||
/// Function to send child trie without relying on | ||
/// contiguous memory. | ||
pub fn unsafe_ptr_child_trie(&self) -> PtrChildTrie { | ||
( | ||
self.keyspace.as_ptr(), | ||
self.keyspace.len() as u32, | ||
self.root.as_ref().map(|r| r.as_ptr()).unwrap_or(ptr::null()), | ||
self.root.as_ref().map(|r| r.len() as u32).unwrap_or(u32::max_value()), | ||
self.parent.as_ptr(), | ||
self.parent.len() as u32, | ||
self.extension.as_ptr(), | ||
self.extension.len() as u32, | ||
) | ||
} | ||
/// Function to rebuild child trie accessed from | ||
pub fn unsafe_from_ptr_child_trie(pct: PtrChildTrieMut) -> Self { | ||
let ( | ||
keyspace, | ||
kl, | ||
root, | ||
rl, | ||
parent, | ||
pl, | ||
extension, | ||
el, | ||
) = pct; | ||
unsafe { | ||
let keyspace = from_raw_parts(keyspace, kl).expect("non optional; qed"); | ||
let root = from_raw_parts(root, rl); | ||
let parent = from_raw_parts(parent, pl).expect("non optional; qed"); | ||
let extension = from_raw_parts(extension, el).expect("non optional; qed"); | ||
ChildTrie { keyspace, root, parent, extension } | ||
} | ||
} | ||
} | ||
|
||
// this is redundant with runtime io without_std TODO EMCH move to some util crate | ||
unsafe fn from_raw_parts(ptr: *mut u8, len: u32) -> Option<Vec<u8>> { | ||
if len == u32::max_value() { | ||
None | ||
} else { | ||
Some(<Vec<u8>>::from_raw_parts(ptr, len as usize, len as usize)) | ||
} | ||
} | ||
|
||
/// Pointers repersentation of ChildTrie | ||
type PtrChildTrie = ( | ||
*const u8, | ||
u32, | ||
*const u8, | ||
u32, | ||
*const u8, | ||
u32, | ||
*const u8, | ||
u32, | ||
); | ||
|
||
/// Mut Pointers repersentation of ChildTrie | ||
type PtrChildTrieMut = ( | ||
*mut u8, | ||
u32, | ||
*mut u8, | ||
u32, | ||
*mut u8, | ||
u32, | ||
*mut u8, | ||
u32, | ||
); | ||
|
||
|
||
impl AsRef<ChildTrie> for ChildTrie { | ||
fn as_ref(&self) -> &ChildTrie { | ||
self | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,6 +108,11 @@ export_api! { | |
/// Get child trie for a given `storage_key` location, or `None` if undefined. | ||
fn child_trie(storage_key: &[u8]) -> Option<ChildTrie>; | ||
|
||
/// Update or create an existing child trie. | ||
/// Return false if it could not be updated (eg direct change | ||
/// of root is not allowed). | ||
fn set_child_trie(ct: ChildTrie) -> bool; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be replaced with a safer semantics? Like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't it the same thing as what we do with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main suggestion here is letting this call generate the keyspace for newly initialized child tries. The function could just then be |
||
|
||
/// Get `key` from child storage, placing the value into `value_out` (as much of it as possible) and return | ||
/// the number of bytes that the entry in storage had beyond the offset or None if the storage entry | ||
/// doesn't exist at all. Note that if the buffer is smaller than the storage entry length, the returned | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,8 +19,9 @@ pub use rstd; | |
pub use rstd::{mem, slice}; | ||
|
||
use core::{intrinsics, panic::PanicInfo}; | ||
use rstd::{vec::Vec, cell::Cell, convert::TryInto}; | ||
use primitives::{offchain, Blake2Hasher, child_trie::{ChildTrie, ChildTrieReadRef}}; | ||
use rstd::{vec::Vec, cell::Cell, convert::TryInto, ptr}; | ||
use primitives::{offchain, Blake2Hasher, | ||
child_trie::{ChildTrie, ChildTrieReadRef}}; | ||
|
||
#[cfg(not(feature = "no_panic_handler"))] | ||
#[panic_handler] | ||
|
@@ -201,6 +202,30 @@ pub mod ext { | |
|
||
/// Set value for key in storage. | ||
fn ext_set_storage(key_data: *const u8, key_len: u32, value_data: *const u8, value_len: u32); | ||
/// Get child trie at a storage location. | ||
fn ext_get_child_trie( | ||
storage_key_data: *const u8, | ||
storage_key_len: u32, | ||
a: *mut *mut u8, | ||
b: *mut u32, | ||
c: *mut *mut u8, | ||
d: *mut u32, | ||
e: *mut *mut u8, | ||
f: *mut u32, | ||
g: *mut *mut u8, | ||
h: *mut u32 | ||
) -> bool; | ||
/// Set child trie return false if there is an attempt to change non empty root. | ||
fn ext_set_child_trie( | ||
a: *const u8, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Use descriptive variable names. |
||
b: u32, | ||
c: *const u8, | ||
d: u32, | ||
e: *const u8, | ||
f: u32, | ||
g: *const u8, | ||
h: u32 | ||
) -> bool; | ||
/// Remove key and value from storage. | ||
fn ext_clear_storage(key_data: *const u8, key_len: u32); | ||
/// Checks if the given key exists in the storage. | ||
|
@@ -584,12 +609,43 @@ impl StorageApi for () { | |
|
||
/// Get child trie at storage key location. | ||
fn child_trie(storage_key: &[u8]) -> Option<ChildTrie> { | ||
let prefixed_key = ChildTrie::prefix_parent_key(storage_key); | ||
let prefixed_key_cat = ChildTrie::parent_key_slice(&prefixed_key); | ||
storage(prefixed_key_cat) | ||
.and_then(|enc_node| ChildTrie::decode_node_with_parent(&enc_node, prefixed_key)) | ||
let mut a = ptr::null_mut(); | ||
let mut b = 0u32; | ||
let mut c = ptr::null_mut(); | ||
let mut d = 0u32; | ||
let mut e = ptr::null_mut(); | ||
let mut f = 0u32; | ||
let mut g = ptr::null_mut(); | ||
let mut h = 0u32; | ||
unsafe { | ||
if ext_get_child_trie.get()( | ||
storage_key.as_ptr(), | ||
storage_key.len() as u32, | ||
&mut a as *mut _, | ||
&mut b, | ||
&mut c as *mut _, | ||
&mut d, | ||
&mut e as *mut _, | ||
&mut f, | ||
&mut g as *mut _, | ||
&mut h, | ||
) { | ||
Some(ChildTrie::unsafe_from_ptr_child_trie((a, b, c, d, e, f, g, h))) | ||
} else { | ||
None | ||
} | ||
} | ||
} | ||
|
||
/// Set child trie. Can fail and return false (eg change of root). | ||
fn set_child_trie(ct: ChildTrie) -> bool { | ||
unsafe { | ||
let p = ct.unsafe_ptr_child_trie(); | ||
ext_set_child_trie.get()(p.0, p.1, p.2, p.3, p.4, p.5, p.6, p.7) | ||
} | ||
} | ||
|
||
|
||
fn child_storage(child_trie: ChildTrieReadRef, key: &[u8]) -> Option<Vec<u8>> { | ||
let mut length: u32 = 0; | ||
let empty_byte: [u8;0] = []; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -238,6 +238,27 @@ where | |
self.overlay.set_child_storage(child_trie, key, value); | ||
} | ||
|
||
fn set_child_trie(&mut self, ct: ChildTrie) -> bool { | ||
let _guard = panic_handler::AbortGuard::new(true); | ||
// do check for backend | ||
let ct = match self.child_trie(ct.parent_trie().as_ref()) { | ||
Some(ct_old) => if | ||
(ct_old.root_initial_value() != ct.root_initial_value() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand this. If the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Root should never be change directly like this. 👍 |
||
&& !ct.is_new()) || | ||
ct_old.keyspace() != ct.keyspace() { | ||
return false; | ||
} else { | ||
ct | ||
}, | ||
None => if ct.is_new() { | ||
ct | ||
} else { | ||
return false; | ||
}, | ||
}; | ||
self.overlay.set_child_trie(ct) | ||
} | ||
|
||
fn kill_child_storage(&mut self, child_trie: &ChildTrie) { | ||
let _guard = panic_handler::AbortGuard::new(true); | ||
|
||
|
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.
in which context ? because you can create multiple ones in case you give wrong entry ?
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.
context is mainly the window of time where you create the child trie at a parent key but it is not updated (calling this function again will create another child trie instance at the same parent key which is not suitable).
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 ok I feel it is better that old new function as old new function would have been worse:
like you could do a new without fetching the current trie and then write on it and you would have two child trie with the same parent.