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

feat: make handshake cancelable (backport #857) #1013

Merged
merged 2 commits into from
Sep 5, 2023

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Jun 21, 2023

it'll make the handshake work with graceful shutdown(see: cosmos/cosmos-sdk#16202)

handshake could be a long running process if there are many local blocks to replay, for example we use it to do profiling.


  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

it'll make the handshake work with graceful shutdown(see: cosmos/cosmos-sdk#16202)

handshake could be a long running process if there are many local blocks to replay, for example we use it to do profiling.

Hope we can backport this to 0.34.x.

---

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments
@yihuang yihuang requested a review from a team as a code owner June 21, 2023 08:25
@@ -265,7 +271,7 @@ func (h *Handshaker) Handshake(proxyApp proxy.AppConns) error {
}

// Replay blocks up to the latest in the blockstore.
_, err = h.ReplayBlocks(h.initialState, appHash, blockHeight, proxyApp)
appHash, err = h.ReplayBlocksWithContext(ctx, h.initialState, appHash, blockHeight, proxyApp)
Copy link
Contributor

@sergio-mena sergio-mena Jun 21, 2023

Choose a reason for hiding this comment

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

I know in future branches the original here re-writes appHash, but not here. I'd leave it as is unless we understand why we are making that change.

Copy link
Contributor

Choose a reason for hiding this comment

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

@yihuang any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's kind of bug fix, previously the log don't output latest hash, but I'm ok that we change that in a separate PR

Copy link
Contributor Author

@yihuang yihuang Sep 5, 2023

Choose a reason for hiding this comment

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

hi @sergio-mena , do you want me to revert this line?

This makes the log outputs the latest hash, and it only affects the log content.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @yihuang sorry for delay on coming back to this (constant flow of kinda-more-urgent things)

I took a look at this again, and when comparing this with the backport on v0.34.x (#1012) I realise this change is present there as well, so I'm OK with it. Approving...

@sergio-mena sergio-mena self-assigned this Jun 21, 2023
@github-actions
Copy link

github-actions bot commented Jul 3, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale For use by stalebot label Jul 3, 2023
@sergio-mena sergio-mena removed the stale For use by stalebot label Jul 3, 2023
@sergio-mena sergio-mena added the wip Work in progress label Jul 10, 2023
@sergio-mena sergio-mena removed the wip Work in progress label Sep 5, 2023
@sergio-mena sergio-mena merged commit 790d57e into cometbft:v0.37.x Sep 5, 2023
19 checks passed
@yihuang yihuang deleted the v0.37.x branch September 5, 2023 13:47
nivasan1 pushed a commit to skip-mev/cometbft that referenced this pull request Jan 16, 2024
it'll make the handshake work with graceful shutdown(see: cosmos/cosmos-sdk#16202)

handshake could be a long running process if there are many local blocks to replay, for example we use it to do profiling.

Hope we can backport this to 0.34.x.

---

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants