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

Add timestamp calculation to all block responses #5470

Draft
wants to merge 1 commit into
base: chore/add-timestamp-to-block-response
Choose a base branch
from

Conversation

jferrant
Copy link
Collaborator

First pass. Haven't tested this at all and this was just first thoughts.


sortition_state
.cur_sortition
.burn_header_timestamp
Copy link
Collaborator Author

@jferrant jferrant Nov 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work i don't think...cause the burn header timestamp could be super duper old. Like this could be the second, third,...,Nth tenure extend. I will need to track the current tenure timestamp start better I think/Needs to reset per tenure extend.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can relatively easily check if a block has a TenureChange payload in it (doesn't matter if it's for an extend or new block), and then use that as our "start" for the timer

@jferrant jferrant marked this pull request as draft November 16, 2024 00:32
@@ -190,6 +193,7 @@ impl BlockInfo {
} else {
self.signed_self.get_or_insert(get_epoch_time_secs());
}
self.processed_time = Some(get_epoch_time_secs());
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be updated to use the block validate response process time (which will most likely be a nmb secs rather than a timestamp)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding that now. I'm going to include a u128, validation_time_ms, in an Ok response.

pub struct BlockValidateOk {
    pub signer_signature_hash: Sha512Trunc256Sum,
    pub cost: ExecutionCost,
    pub size: u64,
    pub validation_time_ms: u128,
}

Will open a PR after updating the tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #5474

@@ -135,6 +136,8 @@ pub struct SignerConfig {
pub tenure_last_block_proposal_timeout: Duration,
/// How much time to wait for a block proposal validation response before marking the block invalid
pub block_proposal_validation_timeout: Duration,
/// How much idle tie must pass before allowing a tenure extend
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// How much idle tie must pass before allowing a tenure extend
/// How much idle time must pass before allowing a tenure extend

@@ -190,6 +193,7 @@ impl BlockInfo {
} else {
self.signed_self.get_or_insert(get_epoch_time_secs());
}
self.processed_time = Some(get_epoch_time_secs());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding that now. I'm going to include a u128, validation_time_ms, in an Ok response.

pub struct BlockValidateOk {
    pub signer_signature_hash: Sha512Trunc256Sum,
    pub cost: ExecutionCost,
    pub size: u64,
    pub validation_time_ms: u128,
}

Will open a PR after updating the tests.

@@ -190,6 +193,7 @@ impl BlockInfo {
} else {
self.signed_self.get_or_insert(get_epoch_time_secs());
}
self.processed_time = Some(get_epoch_time_secs());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #5474

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants