Skip to content

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

Merged
sergio-mena merged 2 commits intocometbft:v0.37.xfrom
yihuang:v0.37.x
Sep 5, 2023
Merged

feat: make handshake cancelable (backport #857)#1013
sergio-mena merged 2 commits intocometbft:v0.37.xfrom
yihuang:v0.37.x

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

// 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
Collaborator

@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
Collaborator

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
Collaborator

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
@yihuang yihuang requested a review from sergio-mena September 5, 2023 06:23
@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
@yihuang yihuang deleted the v0.37.x branch September 5, 2023 13:47
g3ndo9kar9 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.

2 participants