Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

discuss: trie mask issue #613

Draft
wants to merge 60 commits into
base: develop
Choose a base branch
from
Draft

discuss: trie mask issue #613

wants to merge 60 commits into from

Conversation

0xaatif
Copy link
Contributor

@0xaatif 0xaatif commented Sep 9, 2024

Relates #583

I'm a bit confused by mpt_trie::trie_subsets::create_trie_subset.
AIUI any subset should NOT change the state root of a trie - it just hides subtries behind a hash indirection.

Note that #583 calls hiding subtries "masking", mpt_trie calls it "subsetting". The StateMpt::mask function is a thin (and I think low risk) wrapper around the mpt_trie functionality:

fn mask(&mut self, addresses: impl IntoIterator<Item = TrieKey>) -> anyhow::Result<()> {
let inner = mpt_trie::trie_subsets::create_trie_subset(
self.typed.as_hashed_partial_trie(),
addresses.into_iter().map(TrieKey::into_nibbles),
)?;
self.typed = TypedMpt {
inner,
_ty: PhantomData,
};
Ok(())
}

Here we branch on masking a particular subtrie:

if !precompiled_addresses.contains(&addr.compat()) {
// TODO(0xaatif): this looks like an optimization,
// but if it's omitted, the tests fail...
if std::env::var_os("SKIP_MASK_PRECOMPILED").is_some_and(|it| it == "true") {
} else {
state_mask.insert(TrieKey::from_address(addr));
}
}

I've added an SKIP_MASK_PRECOMPILED for the sake of this discussion.

If we DO mask those addresses:

$ git rev-parse --short HEAD
cfe6d42d
$ cargo test --quiet --package trace_decoder --test consistent-with-header -- --quiet
loading test vectors...done

running 14 tests
..............
test result: ok. 14 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 13.20s

Everything is ok.
If we DO NOT mask those addresses:

$ SKIP_MASK_PRECOMPILED=true cargo test --quiet --package trace_decoder --test consistent-with-header -- --quiet
loading test vectors...done
...
test result: FAILED. 4 passed; 10 failed; 0 ignored; 0 measured; 0 filtered out; finished in 12.46s

Eek! Let's see more detail about one of the failures

$ SKIP_MASK_PRECOMPILED=true cargo test --quiet --package trace_decoder --test consistent-with-header -- --exact b4_dev@1
loading test vectors...done
...
Assertion failed at trace_decoder/tests/consistent-with-header.rs:34:17:
  check!( pairs().position(|(before, after)| {
                        before.trie_roots_after.state_root != after.tries.state_trie.hash()
                    }) == None )
with expansion:
  Some(1) == None

Assertion failed at trace_decoder/tests/consistent-with-header.rs:50:17:
  check!( gen_inputs
                        .last()
                        .map(|it| it.trie_roots_after.state_root.compat()) == Some(header.state_root) )
with diff:
  Some(
<     0xb2415bb7cb3c0dea9520a546500dc54cbe444afadd85e68bd688e3a96bf69196,
>     0x038e7e3c67c9166523c4b2bae746f5d7cf72480af4294bb900f252faf05d04e9,
  )
...

I'll try and work on a smaller test case :)

0xaatif and others added 30 commits September 2, 2024 14:15
@github-actions github-actions bot added the crate: trace_decoder Anything related to the trace_decoder crate. label Sep 9, 2024
@0xaatif 0xaatif marked this pull request as draft September 9, 2024 15:36
@Nashtare
Copy link
Collaborator

I'm not sure I understand the replication to begin with..

if !precompiled_addresses.contains(&addr.compat()) {
// TODO(0xaatif): this looks like an optimization,
// but if it's omitted, the tests fail...
if std::env::var_os("SKIP_MASK_PRECOMPILED").is_some_and(|it| it == "true") {
} else {
state_mask.insert(TrieKey::from_address(addr));
}
}

We enter the if condition if we do NOT have a precompile address at hand, so your SKIP_MASK_PRECOMPILED flag is irrelevant here. What #488 aimed at doing was, in the case we do have a precompile, to check if it effectively is present in the initial trie witness (i.e. not hashed out), and mark it (or mask with your terminology? I'm not sure) as touched if there is a path down the trie for it.

@0xaatif 0xaatif force-pushed the 0xaatif/trace-decoder-backend-rewrite4 branch 2 times, most recently from f56b00d to af9251a Compare September 11, 2024 11:03
Base automatically changed from 0xaatif/trace-decoder-backend-rewrite4 to develop September 12, 2024 16:42
@BGluth
Copy link
Contributor

BGluth commented Sep 19, 2024

Yeah creating a trie subset should always produce a trie with an identical hash, so that's a bug.

Maybe subset isn't the best name here. It's true that the trie has only subset of the original trie's node and it's a different trie, but it's a bit confusing as well since the trie is the same except for hash nodes.

I would just run this through trie diff and see where they diverge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: trace_decoder Anything related to the trace_decoder crate.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants