Build correctly, with the latest ethportal-api

[carver]

(I think) I need the latest ethportal-api to parse the new query trace. I tried to force cargo to use the latest, but then glados-web wouldn't build for me locally.

To start, this PR is making sure I don't just have a local build problem. After confirming that, I'll switch over to making glados build again.

[carver]

We should do this more often. Looks like this hasn't been run since ethereum/trin@63275a2, which 3 months ago. Looks like quite a few incompatibilities have shown up since then.

[carver]

I know there's a lot to look at at once, but I can't really split it into smaller PRs. I tried to make the commits conceptually self-contained, if you want to look at smaller chunks at a time.

[morph-dev]

In general, looks good to me.

I think all my comments (except one) are small suggestion on how type conversion could have been done differently. Not necessarily because I think it's a better way, but because I wanted to provide alternative.

The only bigger comment is related to state audit and not strictly related to this PR. Therefore, while nice to have, it can be ignored and added in future improvements.

[morph-dev]

nit: I think this should work, but I don't think this is the best way of doing things (because it can panic if something is wrong). Instead, I would:

1. Have let content_key = StateContentKey::AccountTrieNode(current_content_key.clone()); at the beginning of loop (line 164) and reuse it later (line 166).

2. replace this and following line (196) with:

let trie_node = match StateContentValue::decode(&content_key, &content_value)
    .map_err(|err| anyhow!("Error decoding content value: {err}"))
    .and_then(|content_value| match content_value {
        StateContentValue::TrieNode(trie_node) => Ok(trie_node.node),
        _ => Err(anyhow!("Wrong content value type, expected: TrieNode")),
    })
    .and_then(|encoded_trie_node| Ok(encoded_trie_node.as_trie_node()?))
{
    Ok(trie_node) => trie_node,
    Err(err) => return Err((err, current_content_key)),
};

[carver]

Ok, I agree that we should get rid of the panic. Even though the .expect("Trie node received from the portal network should be decoded as a trie node") was not introduced by this PR, I'm happy to fix that broken window along the way.

The shortest version I came up with is:

A)

let encoded_trie_node: EncodedTrieNode = content_value.to_vec().into();        
let trie_node = encoded_trie_node.as_trie_node().map_err(|err| {        
    (        
        anyhow!("Error decoding node while walking trie: {err:?}"),        
        current_content_key.clone(),        
    )        
})?; 

Versus the touched-up version from your comment:

B)

let full_key = StateContentKey::AccountTrieNode(current_content_key.clone());        
let trie_node = match StateContentValue::decode(&full_key, &content_value)        
    .map_err(|err| anyhow!("Error decoding content value: {err}"))        
    .and_then(|content_value| match content_value {        
        StateContentValue::TrieNode(trie_node) => Ok(trie_node.node),        
        other => Err(anyhow!("Wrong content value type, expected: TrieNode, got: {other:?}")),        
    })        
    .and_then(|encoded_trie_node| Ok(encoded_trie_node.as_trie_node()?))        
{        
    Ok(trie_node) => trie_node,        
    Err(err) => return Err((err, current_content_key)),        
}; 

The most notable tradeoffs I see are:
A

• Shorter code
• It assumes the type of the state content value is an account trie node

B

• Longer code
• More generalized, maybe laying the groundwork for auditing things like the bytecode

I can imagine a rationale for B could be laying some groundwork toward supporting bytecode and contract storage content values. Though I think in this PR which is focused on getting glados working with the latest ethportal-api, it feels a bit tangential. It would probably make sense to make broader changes in process_trie_nodes(), etc, if we're really going to take a stab at generalizing.

In the context it's in, A seems like it's leaving the code better than I found it, without getting too invested in generalizing to handling other content values. So I guess I'm leaning toward A right now.

I guess you were saying as much here:

The only bigger comment is related to state audit and not strictly related to this PR. Therefore, while nice to have, it can be ignored and added in future improvements.

So I'll just go with that and merge when everything is rebased and green.

[morph-dev]

nit: you don't have to convert it to RawContentKey.

Instead, you can use HistoryContentKey::from_hex(&content_key_hex) below (instead of HistoryContentKey::try_from(content_key_raw.clone())).

Same for state and beacon.

[carver]

Yeah, the only downside of this is that we want to understand the difference between a key that isn't valid hex-bytes, and a key that doesn't match any valid content key pattern. Plus, we use the raw bytes when querying the database anyway, so we still have to manually decode the hex. I still think your approach is better anyway, by avoiding a few clones.

I think I see a reasonable path to accommodate all the needs. It's a little slower in the "invalid hex" path, but that seems okay.

Edit: I don't know, I'm still working on it. I don't like it yet.

[carver]

I think there are more optimized approaches, but I didn't like how any of my attempts went. So I just went with the most naive change that seems fine.

It does some things that make me cringe a tiny bit, like decoding the hex value for the DB, then using the original hex value, which is getting decoded over and over again for each key check. But it's not a big deal, and I think it's time to just call it a distraction and move on.