-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat: Adds ChainSegment
filtering
#230
base: master
Are you sure you want to change the base?
Conversation
742bafe
to
361d1fb
Compare
361d1fb
to
3fe647c
Compare
I think we should refer to it as 4444s not pre-merge. As 4444s is a window which grows over time. Calling it pre-merge now just creates more work down the line. Other then that I think we can merge this now and iterate on it if needed. |
I think 4444's audit should work like this.
Filtering 4444's blocks from the current random audit I don't think achieves this goal. 4444s is more important then the latest and random audit. So I think it should be its own audit type not a child of random. |
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.
Can you help me understand how the audit's happening? IIUC, glados has a "random" audit, which "randomly" selects content keys from its database and audits them.
Then, 4444s is filtering "random" content keys (less than MERGE_BLOCK_NUMBER
).
The "random" strategy on glados-ethdevops is consistently around 60%. Which leads me to believe that glados-audit
hasn't "audited" the entire chains worth of content keys, and it's only randomly selecting content keys that have been added to the db while glados-audit
has been running in "latest" mode (since it's last reboot).
So then it seems that "pre-merge" wouldn't have any content keys to select from?
Anyways, that's my understanding. I could be wrong. If glados-audit
is run on backfill from 0
-MERGE_BLOCK_NUMBER
, then I think this strategy would work. But i'm not sure how devops is configured. Furthermore, if I run glados locally, then I don't want to wait for glados-audit
to run that backfill. So, imo a strategy more like what @KolbyML laid out seems to be worth implementing
content_alias, | ||
) | ||
.join(JoinType::InnerJoin, aliased_content_metadata_relation) | ||
.filter(execution_metadata::Column::BlockNumber.lt(15_537_392)) |
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 would set this block value to be a const
... nitpick I also think you're off by a few blocks source
Agreed that this needs to change eventually, but isn't important for the time being. But calling it "pre-merge" internally ("4444s" externally) might make sense for the time being, since it makes it fairly obvious that the strategy is for only "pre-merge" blocks and will need to be updated once 4444s is stabilized |
@KolbyML @njgheorghita Good questions. Some context I missed in the description is that I tested by loading tons of pre-4444 keys into the DB via I agree this is sub-optimal operational experience. I'm updating to allow |
ChainSegment
filtering
Adds audit filtering by
ChainSegment
, with one segment defined:PreMerge
(orAll
).Adds stat calculation logic for pre merge, as well as for pre merge headers, bodies, and receipts.
Adds migration to add these stats to stat history table.
Adds these stats, labeled as "4444s" on the stats history graph.
Adds a "4444s" filtering selection to the audit dashboard.