Conversation
drahnr
added
the
A3-in_progress
drahnr
force-pushed
the
bernhard/slashing-w-ocw
branch
3 times, most recently
from
31bf932 to
54da597
Compare
| fn prove(key: (KeyTypeId, D)) -> Option<Self::Proof> { | ||
| //@note must take a context containing the index as paramter | ||
| // rather than depending on the current one since this func might | ||
| // be called at a later point of time |
There was a problem hiding this comment.
Yeah and the ProvingTrie should only be generated if it's equal to the current session
|
|
||
| // @todo requires some internal mutability concept to work properly | ||
| // @todo which is yet to be hashed out | ||
| unsafe impl<L> core::marker::Sync for AlternativeDB<L> where |
There was a problem hiding this comment.
nothing in the struct definition makes me think this unsafe implementation is needed. Generally we avoid unsafe code very strongly, so I'd rather take a performance hit than introduce unsafe code.
There was a problem hiding this comment.
Unfortuantely the trait bound of HashDB requires it to be sync, which can only guaranteed if we introduce internal mutability or only impl HashDB for Arc<Mutex<AlternativeDB>> which I wanted to hold off until there is clarity iff concurrency/reentrancy are a problems.
There was a problem hiding this comment.
I don't see how a Sync bound, in the absence of some other factors, implies interior mutability. I haven't looked at this in some time, but don't the functions of HashDB that require you to mutate pass &mut self? If that is the case, then are you saying that there is a reason we would need interior mutability on the &self methods of HashDB?
| let mapping: BTreeSet<(Vec<u8>, Vec<u8>)> = | ||
| offchain::local_storage_get(StorageKind::PERSISTENT, tracking_key.as_ref()) | ||
| .map(|bytes| { | ||
| <BTreeSet<(Vec<u8>, Vec<u8>)> as Decode>::decode(&mut &bytes[..]).unwrap() |
There was a problem hiding this comment.
we are also strictly against unwraps in the codebase. the rule is "prove or remove" - use expect with a proof that the panic will not occur.
There was a problem hiding this comment.
Mea culpa, what would be the expected behaviour in that case? Should a warning be issued if a entry can not be loaded and the error be discarded and converted to a "Not Found"?
There was a problem hiding this comment.
At this level of the code, it seems that we should be bubbling up a None value from the return of the function. It seems like the users of get_index should have an error handling case where that returns none. Warn & continue sounds reasonable for the use-cases we have.
| } | ||
|
|
||
|
|
||
| fn derive_tracking_index_key(session_index: &[u8]) -> Vec<u8> { |
There was a problem hiding this comment.
could this and other functions with a self receiver be free functions rather than inherent impls? Free function is typically more idiomatic in Rust; inherent impls are usually only used as constructors.
There was a problem hiding this comment.
Could be, the only reason was the move of the const prefixes into the impl, but if both live outside, it'll work - will do
drahnr
force-pushed
the
bernhard/slashing-w-ocw
branch
from
73cb220 to
a037d25
Compare
drahnr
force-pushed
the
bernhard/slashing-w-ocw
branch
from
9537827 to
bde7a20
Compare
|
I think this can be discarded in favour of #6220 |
drahnr
added
A0-please_review
3 participants
Plumbing to allow simplified slashing utilizing data stored by the
offchain_index::setAPI.Related #3722