-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: Implement eth68 announcement metrics, track entries by TxType #6786
Conversation
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.
nice work! this is very useful. perhaps you want to implement a nice panel and add to dashboard we ship in docker when done? I can make an issue for it.
crates/net/network/src/metrics.rs
Outdated
impl AnnouncedTxSizeMetrics { | ||
/// Update metrics during announcement validation, by examining each announcement entry based on | ||
/// TxType | ||
pub(crate) fn update_on_valid_announcement(&self, announcement: &ValidAnnouncementData) { |
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.
instead of &ValidAnnouncementData
as param, can we pass a new type TxTypesCounter
?
struct TxTypesCounter {
legacy: usize,
eip2930: usize,
eip1559: usize,
eip4844: usize,
}
and then have a new function that updates TxTypesCounter
called from here
reth/crates/net/network/src/transactions/validation.rs
Lines 129 to 142 in 6cf9e91
if let Some(strict_min_encoded_tx_length) = self.strict_min_encoded_tx_length(tx_type) { | |
if size < strict_min_encoded_tx_length { | |
debug!(target: "net::eth-wire", | |
ty=ty, | |
size=size, | |
hash=%hash, | |
strict_min_encoded_tx_length=strict_min_encoded_tx_length, | |
network=%Self, | |
"invalid tx size in eth68 announcement" | |
); | |
return ValidationOutcome::Ignore | |
} | |
} |
?
a new instance of TxTypesCounter
is then made for each eth68 announcement
Co-authored-by: Emilia Hane <[email protected]>
Co-authored-by: Emilia Hane <[email protected]>
@@ -208,7 +219,7 @@ impl FilterAnnouncement for EthAnnouncementFilter { | |||
fn filter_valid_entries_68( | |||
&self, | |||
msg: NewPooledTransactionHashes68, | |||
) -> (FilterOutcome, ValidAnnouncementData) | |||
) -> (FilterOutcome, ValidAnnouncementData, TxTypesCounter) |
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.
@emhane Where would you like to put an instance of TxTypesCounter
? I am a little confused about TxTypesCounter
, let me know if this is not you want.
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.
np. only the file with metrics and the validation file should have to change. create a new instance of TxTypesCounter
here
pass it as parameter to should_fetch
fn should_fetch(&self, ty: u8, hash: &TxHash, size: usize, ty_counter: &mut TxTypesCounter) -> ValidationOutcome;
update it here
update metrics before this line
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.
Thank you for your detailed instructions. Based on your guidance, it seems I should incorporate AnnouncedTxTypesMetrics
as a field within EthAnnouncementFilter
, rather than placing it within TransactionFetcher
. PTAL again.
perfect, thanks! |
ty: u8, | ||
hash: &TxHash, | ||
size: usize, | ||
tx_types_counter: &mut TxTypesCounter, |
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.
hmm, should the caller be responsible for updating this?
the update can be derived from the return value?
…aradigmxyz#6786) Co-authored-by: Emilia Hane <[email protected]>
Close #6524
Add
Histogram
to track entries number by TxType when validating announcement.