feat: fallback preimage recovery w/o debug_dbGet #30
Conversation
crates/primitives/src/storage.rs
Outdated
|
|
||
| /// Custom database access methods implemented by RSP storage backends. | ||
| pub trait ExtDatabaseRef { | ||
| /// The database error type. | ||
| type Error; | ||
|
|
||
| /// Gets the preimage of a trie node given its Keccak hash. | ||
| fn trie_node_ref(&self, hash: B256) -> Result<Bytes, Self::Error>; | ||
| fn trie_node_ref(&self, hash: B256, context: PreimageContext) -> Result<Bytes, Self::Error>; |
There was a problem hiding this comment.
Does passing this around affect the number of cycles in the client program? I feel like since client program cycles are super critical, we should make sure it doesn't
There was a problem hiding this comment.
I was thinking about the same thing but thought it shouldn't matter since it's just passing 2 references. But I can do a quick benchmark on a before/after for this. Will update here. Do note that since the code is not yet deterministic it might not be 100% accurate.
There was a problem hiding this comment.
Hmmm to my surprise the cycle count change turns out to be not really negligible. I was expected it to be buried in the noise of the non-determinism. Turns out for the block tested 20600000 (avg out of 10 runs each):
- Before:
762,727,324cycles - After:
765,003,639cycles
We got some 0.5% increase from this. This is quite surprising as this is really just passing 2 references, and I was half expecting the compiler to optimize it away since it's unused.
0.5% isn't huge but for something that can be avoided altogether it's not ideal. Will do a bit refactor here to add a feature that makes this host-only. Was hoping that it would be negligible so we don't have to over-engineer it.
There was a problem hiding this comment.
@puma314 Just pushed a new commit that uses Rust feature to make sure none of the changes affect client other than adding a unit type parameter (which I'm kina sure gets optimized away since it's zero-sized).
Ran the test against the same block and I'm now getting 762,685,729 which is actually even lower than before this change, though I'm pretty sure this comes from the noise of non-determinism.
In any case, we're now no longer sacrificing client efficiency for this feature.
Enables running RSP against a node that does not serve
debug_dbGet. Changes README to not require but only recommend agethnode runningstate.scheme=hash.