-
Notifications
You must be signed in to change notification settings - Fork 1
trie/metrics: Adding state depth metrics tracking using witness #1
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
base: state-size-metrics
Are you sure you want to change the base?
Conversation
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
This reverts commit 87e1d82. Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
avgAccessDepthInBlock = metrics.NewRegisteredGauge("trie/access/depth/avg", nil) | ||
minAccessDepthInBlock = metrics.NewRegisteredGauge("trie/access/depth/min", nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally, add every access depth and let the consumer do its own filtering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're going to want to separate account trie and storage tries
return nil, err | ||
} | ||
|
||
// If witness was generated, update metrics regarding the access paths. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note for later: this is creating a dependency on the witness creation, so maybe it would be a good idea to create a specific object that is passed to ProcessBlock
. But let's see that for when we want to merge it in master.
Witness() map[string]struct{} | ||
|
||
// WitnessPaths returns a set of paths for all trie nodes. For future reference, | ||
// witness can be deprecated and used as a replacement to witness. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// witness can be deprecated and used as a replacement to witness. | |
// witness can be deprecated and these paths can be used as a replacement to witness. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with this comment, because the witness will be spec'd by something exterior and this external consumer will not be interested in the paths because of size reasons. But we shall see.
State: make(map[string]struct{}), | ||
chain: chain, | ||
Paths: make(map[string]struct{}), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
silly comment, but why the empty line here?
|
||
// Witness returns a set containing all trie nodes that have been accessed. | ||
func (t *VerkleTrie) WitnessPaths() map[string]struct{} { | ||
panic("not implemented") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good start. My main issue with this, is that it connects the metrics collection with the witness, when I am hoping this can be done at the tracer level so that we leave the witness alone. Food for thought.
return witnessStates | ||
} | ||
|
||
func (t *Trie) WitnessPaths() map[string]struct{} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should merge Witness
and WitnessPaths
as a single function, returning accessed trie nodes in key-value pairs. e.g., map[nodepath][nodeblob]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of doing this but the reason I haven't is because it seems like we have some deserialization going on when receiving witness from the beacon client.
defer w.lock.Unlock() | ||
|
||
maps.Copy(w.State, nodes) | ||
if paths != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's problematic. The node paths are aggregated without distinguishing the owner.
e.g., the nodes in storage trie and account trie may have shared path, but they belong to different trie.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha this makes sense!
Headers []*types.Header // Past headers in reverse order (0=parent, 1=parent's-parent, etc). First *must* be set. | ||
Codes map[string]struct{} // Set of bytecodes ran or accessed | ||
State map[string]struct{} // Set of MPT state trie nodes (account and storage together) | ||
Paths map[string]struct{} // Set of MPT trie paths (i.e. all accessed nodes, not just the ones in state) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to distinguish account trie and storage tries somehow.
71dc038
to
b582c39
Compare
b582c39
to
ca82606
Compare
e2036eb
to
f7332e9
Compare
This PR tracks accesses and updates on the state trie through the witness api and tracks with in our metrics