Fixed withdrawal accounts not being in the state trie #18
Conversation
| @@ -350,12 +354,26 @@ impl ProcessedBlockTrace { | |||
| let txn_idx_of_dummy_entry = | |||
There was a problem hiding this comment.
Nit: I think this is actually never used and could be removed at some point.
| // To avoid double hashing the addrs, but I don't know if the extra `Vec` | ||
| // allocation is worth it. |
There was a problem hiding this comment.
I guess this is fine as is for now (we're definitely not in a hot spot anyway here). But there are a few allocations across the lib that could be alleviated I think, so worth keeping track of, I'll open a ticket.
| fn create_dummy_proof_trie_inputs( | ||
| final_trie_state: &PartialTrieState, | ||
| state_trie: HashedPartialTrie, | ||
| ) -> TrieInputs { |
There was a problem hiding this comment.
The namings here aren't straightforward. We need a massive documentation effort anyway across the decoder (proof-gen documentation is somehow acceptable now). But to not increase complexity once we merge this branch fix to main I'd suggest documenting the newest changes for clarity.
| @@ -350,12 +354,26 @@ impl ProcessedBlockTrace { | |||
| let txn_idx_of_dummy_entry = | |||
| txn_ir.last().unwrap().txn_number_before.low_u64() as usize + 1; | |||
There was a problem hiding this comment.
Why are we guaranteed that txn_ir is never empty? And why wouldn't txn_ir.last()?.txn_number_before.low_u64() work?
| // To avoid double hashing the addrs, but I don't know if the extra `Vec` | ||
| // allocation is worth it. |
There was a problem hiding this comment.
This comment should probably be removed.
There was a problem hiding this comment.
As there are a few places where the allocations may be removed, I think it could be preferable to keep this "for now", and remove it during the. Allocations PR, whether we find a concrete answer to this section or not. Or at least turn it as a todo
|
|
||
| impl TxnMetaState { | ||
| fn txn_bytes(&self) -> Vec<u8> { | ||
| match self.txn_bytes.as_ref() { |
There was a problem hiding this comment.
Is the as_ref() really needed here?
There was a problem hiding this comment.
Yeah in this case we need it if we want to have &self instead of self. We could also do &self.txn_bytes.
|
Hey since I accidentally merged this PR a bit early, I'll open up a second PR later today for some cleanup (including the comments here). |
Just thought I would get your eyes on this one while I test it. Now includes the accounts involved with withdrawals in the dummy IR.