Conversation
MattHalpinParity
added
T0-node
|
@cheme Could you take another look if you have time? |
cheme
left a comment
cheme
left a comment
There was a problem hiding this comment.
looked at recent change, did not see things wrong (most my comment are related to old code I think).
substrate/client/db/src/lib.rs
Outdated
| &self, | ||
| transaction: &mut Transaction<DbHash>, | ||
| id: BlockId<Block>, | ||
| hash: Block::Hash, |
There was a problem hiding this comment.
We should use BlockId here. I think would be simpler to have all that is added clean_state = true in a separate function so the current prune_block stays unchanged. But still on separate fn we should try to use block id (even if in code we likely will use as here the hash function).
substrate/client/db/src/lib.rs
Outdated
| transaction: &mut Transaction<DbHash>, | ||
| displaced: &DisplacedLeavesAfterFinalization<Block>, | ||
| clean_body: bool, | ||
| clean_state: bool, |
There was a problem hiding this comment.
both boolean looks exclusive, I should have put all that correspond to clean_state = true in a separate function I think.
(it would make things easier to follow)
|
|
||
| let changeset = trie.commit(); | ||
| let root = changeset.root_hash(); | ||
| changeset.apply_to(&mut mdb); |
There was a problem hiding this comment.
not sure but I think apply_to return changeset.root_hash so can maybe remove line 114 and 116 here.
| pub fn record_all_keys<L: TrieConfiguration, DB>( | ||
| db: &DB, | ||
| pub fn record_all_keys<L: TrieConfiguration>( | ||
| db: &dyn trie_db::node_db::NodeDB<L::Hash, trie_db::DBValue, L::Location>, |
There was a problem hiding this comment.
is there an type alias for trie_db::node_db::NodeDB<L::Hash, trie_db::DBValue, L::Location>, ?
(not sure but maybe)
| use polkadot_primitives::UpgradeGoAhead; | ||
| use sp_runtime::traits::HashingFor; | ||
| use sp_trie::PrefixedMemoryDB; | ||
| use sp_trie::MemoryDB; |
There was a problem hiding this comment.
Note that here I reduced the testing surface, but makes things closer to real world usage.
Not sure anymore if this should switch.
| None => old_state.clone(), | ||
| }; | ||
| let mut new_state = | ||
| operation.old_state.clone_in_mem().expect("Backend is MemoryDB; qed"); |
There was a problem hiding this comment.
Likely possible to avoid the expect here (new_state as old_state .clone and no commit as in master).
Maybe expect is better, but I am not sure anymore so safest may be to use the existing strategy (even if it may be wrong).
| ) { | ||
| (client.as_block_import(), None, ()) | ||
| ) -> (Self::BlockImport, Option<BoxJustificationImport<Block>>, Self::PeerData) { | ||
| (client, None, ()) |
There was a problem hiding this comment.
Not too sure here, I most likely remove this trait from paritytech/substrate#14612 when switching PrefixedMemorydb to an explicit changeset type.
Still would be good to double check if it may be an issue to remove it (makes code simpler I think as the intention of the mentioned PR cc @bkchr).
Reading this PR again, I see maybe something with an async trait impl (maybe related to something not in polkadot-sdk codebase?).
| let revert_up_to_hash = | ||
| HeaderBackend::hash(&*client, revert_up_to_number)?.ok_or(ClientError::Backend( | ||
| format!("Unexpected hash lookup failure for block number: {}", revert_up_to_number), | ||
| ))?; |
There was a problem hiding this comment.
Same, wonder if master code could be use here
| recorder.reset(); | ||
| self.proof_recorder_root.set(self.root.get()); | ||
| } | ||
| let storage_db = Arc::new(StorageDb::<Hasher> { db, _phantom: Default::default() }); |
There was a problem hiding this comment.
Note that there is a refactor of client/db/bench.
| }; | ||
|
|
||
| struct DbAdapter(parity_db::Db); | ||
| struct DbAdapter(parity_db::Db, bool); |
There was a problem hiding this comment.
Here I should have used a struct with named field:
struct DbAdapter {
db: parity::Db,
is_multi: bool,
}
but actually there should be a way to check if db is multi tree from parity db (likely an utility function that checks state column format: not sure if it is doable with current paritydb code), and no need for this additional bool
7 participants
This PR enable multi tree storage of paritydb.
It currently depends on paritytech/subtrie#2
cc\ @MattHalpinParity @arkpar
A new option is offered to client: paritydbmulti that should probably be consider experimental (it is not default). For polkadot parachain db it is not used (only impact state storage).
Code is based on https://github.com/paritytech/polkadot-sdk/tree/arkpar/new-trie . I am currently not really happy with the way I did merge the changes from #1462 into it (I kept the WithDatabase version, but I am not sure there is not a better way to run it (basically new code with 'wrap' did not run well with Box change, so after a few attempt I just made the new RwLock of this branch not present under no_std).
Right now I did not move a lot of test to support multitree, neither did I test a lot, so I am opening the branch in draft.