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.

Sign up for GitHub

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

Jump to bottom

[node] Add "connections.block_proposal_max_age_secs" and ignore block proposals more than the configured amount #5552

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open

[node] Add "connections.block_proposal_max_age_secs" and ignore block proposals more than the configured amount #5552

wants to merge 16 commits into from

Conversation

jferrant
Copy link
Collaborator

Closes #5439

@jferrant jferrant marked this pull request as ready for review December 13, 2024 18:54
@jferrant jferrant requested review from a team as code owners December 13, 2024 18:54
@aldur aldur added this to the 3.1.0.0.3 milestone Dec 16, 2024
obycode
obycode previously approved these changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

LGTM! Just one minor comment.

stackslib/src/net/api/postblock_proposal.rs Outdated Show resolved Hide resolved
jcnelson
jcnelson previously approved these changes Dec 16, 2024
View reviewed changes
@jferrant jferrant dismissed stale reviews from jcnelson and obycode via 86dc10f December 16, 2024 20:16
@jferrant jferrant requested review from obycode and jcnelson December 16, 2024 20:16
obycode
obycode previously approved these changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

👍

hstove
hstove requested changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

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

I think you forgot to add block_proposal_max_age_secs to ConnectionOptionsFile (with related code to pass into ConnectionOptions).

(This is such an easy thing to forget, it'd be great if we had more of a serde_toml approach or something to make this cleaner and less footgunable)

@jferrant
Copy link
Collaborator Author

block_proposal_max_age_secs

Ugh I totally did. Good catch. Thank you!

@jferrant jferrant requested a review from a team as a code owner December 16, 2024 20:37
@jferrant jferrant requested review from hstove and obycode December 16, 2024 20:37
jcnelson
jcnelson previously approved these changes Dec 16, 2024
View reviewed changes
@obycode
Copy link
Contributor

obycode commented Dec 16, 2024

I think you forgot to add block_proposal_max_age_secs to ConnectionOptionsFile (with related code to pass into ConnectionOptions).

(This is such an easy thing to forget, it'd be great if we had more of a serde_toml approach or something to make this cleaner and less footgunable)

💯 Totally agree!

obycode
obycode previously approved these changes Dec 16, 2024
View reviewed changes
hstove
hstove previously approved these changes Dec 16, 2024
View reviewed changes
@jferrant jferrant dismissed stale reviews from hstove, obycode, and jcnelson via 5f40e2b December 17, 2024 00:46
@hstove
Copy link
Contributor

hstove commented Dec 17, 2024

Hmm, tests::signer::v0::idle_tenure_extend_active_mining is failing

@jferrant
Copy link
Collaborator Author

Hmm, tests::signer::v0::idle_tenure_extend_active_mining is failing

Both idle_tenure_extend_active_mining and miner_recovers_when_broadcast_block_delay_across_tenures_occurs pass locally, but this PR is the first time i have seen these fail which is stressing me out.

@jferrant
Accept all proposals to the proposal endpoint in idle_tenure_extend_a…
2d5e219 
…ctive_mining and miner_recovers_when_broadcast_block_delay_across_tenures_occurs

Signed-off-by: Jacinta Ferrant <[email protected]>
@jferrant jferrant force-pushed the feat/ignore-old-proposals-node branch from c5fe688 to 2d5e219 Compare December 17, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@obycode obycode obycode approved these changes

@jcnelson jcnelson Awaiting requested review from jcnelson

@hstove hstove Awaiting requested review from hstove

At least 2 approving reviews are required to merge this pull request.

Assignees

@jferrant jferrant

Labels
None yet
Projects
Stacks Core Eng
Status: Status: 💻 In Progress
Milestone
3.1.0.0.3
Development

Successfully merging this pull request may close these issues.
5 participants
@jferrant @obycode @hstove @jcnelson @aldur