Skip to content

refactor(node): simplify 'node is blocking chain' logic#3885

Merged
sergio-mena merged 21 commits intomainfrom
sergio/3415-simplify-blocksync-node-blocks
Sep 11, 2024
Merged

refactor(node): simplify 'node is blocking chain' logic#3885
sergio-mena merged 21 commits intomainfrom
sergio/3415-simplify-blocksync-node-blocks

Conversation

@sergio-mena
Copy link
Collaborator

Contributes to #3415

This is mainly refactoring to simplify onlyValidatorIsUs and localNodeBlocksTheChain (since the latter implies the former).
It is a follow-up of #3406 (this is the part of #3406 that doesn't need to be backported)


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

@sergio-mena sergio-mena requested a review from a team as a code owner August 28, 2024 09:01
@sergio-mena sergio-mena requested a review from a team August 28, 2024 09:01
@github-actions

This comment was marked as resolved.

@sergio-mena sergio-mena changed the title Simplify 'node is blocking chain' logic refactor(node): simplify 'node is blocking chain' logic Aug 28, 2024
@sergio-mena sergio-mena self-assigned this Aug 28, 2024
Copy link

@cason cason left a comment

Choose a reason for hiding this comment

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

Legit.

node/setup.go Outdated
config.Mempool,
mp,
waitSync,
true,
Copy link

Choose a reason for hiding this comment

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

I assume this is always true because block sync is mandatory?

Copy link
Collaborator Author

@sergio-mena sergio-mena Sep 11, 2024

Choose a reason for hiding this comment

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

Yes. I just (mindlessly) went from the value we would get in the old code if blocksync is set to true, and then simplified

Copy link
Collaborator Author

@sergio-mena sergio-mena Sep 11, 2024

Choose a reason for hiding this comment

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

Actually it was so mindless that it was wrong 🤦.
Luckily, a couple of unit tests failed, which allowed me to spot the problem and validate the fix.

@cason
Copy link

cason commented Sep 2, 2024

There is a test error that might be related to the changes:
2024/09/02 07:58:15 failed to unmarshal response: error in json rpc client, with http response metadata: (Status: 200 OK, Protocol HTTP/1.1) : RPC error -32603 - Internal error: endpoint is closed while node is catching up

@sergio-mena
Copy link
Collaborator Author

Unit tests really saved me here (rather than the usual e2e)

@sergio-mena sergio-mena added this pull request to the merge queue Sep 11, 2024
Merged via the queue into main with commit 580b923 Sep 11, 2024
@sergio-mena sergio-mena deleted the sergio/3415-simplify-blocksync-node-blocks branch September 11, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants