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

fix(blocksync)!: don't block in blocksync if our voting power is blocking the chain #3406

Merged
merged 14 commits into from
Jul 4, 2024

Conversation

sergio-mena
Copy link
Contributor

@sergio-mena sergio-mena commented Jul 3, 2024

Partially addresses #3415

The a node has no peers, blocksync gets stuck without switching to consesnus, because it needs info from other peers to have an idea of maximum height.

However, there is an edge case (mainly when testing) where a validator might have >2/3 of the voting power and other validators are not started. In this case, we know we are blocking the chain, so we don't need to stay in blockchain if the only condition is that we don't have peers.

Moreover, in order to block a chain, 1/3 of the voting power is enough, so the reasoning of this fix is the following:

  • I am a node and I am starting... shall I run blocksync?
  • Well, looks like I have 1/3 of the voting power (or more) at my current height... so there's no way the chain could advance in my absence... so I don't need to blocksync"

Explanation of commits:

  • Commit 1: e2e testbed reproducing the issue
  • Commit 2: commit with a trivial change to trigger e2e tests. Check the error: ❌ next to the commit hash (3fb1057)
  • Commit 3: Tentative fix. Although there is a ❌ next to the commit hash (16a46ea), if you click on it, you'll see that e2e are passing now.
  • Commit 4: revert commit2
  • Commit 5: Move the check for "local node is blocking the chain" outside the pool, as suggested by @cason
  • Commit 6: Fixed unit tests

All further commits: addressing other comments and tidying up the code


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
  • Title follows the Conventional Commits spec

@sergio-mena sergio-mena self-assigned this Jul 3, 2024
@sergio-mena sergio-mena force-pushed the sergio/blocksync-stalled-no-peers branch from 69ccf42 to 0160866 Compare July 3, 2024 11:54
Copy link
Collaborator

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

Not sure about this workaround.

We should now whether we should run block sync outside the protocol. But, ok, it works. But by changing the block Reactor constructor, we breaking a lot of code.

internal/blocksync/pool.go Outdated Show resolved Hide resolved
internal/blocksync/reactor.go Outdated Show resolved Hide resolved
internal/blocksync/reactor.go Outdated Show resolved Hide resolved
node/setup.go Outdated Show resolved Hide resolved
node/setup.go Outdated Show resolved Hide resolved
internal/blocksync/reactor.go Outdated Show resolved Hide resolved
@sergio-mena sergio-mena marked this pull request as ready for review July 3, 2024 17:55
@sergio-mena sergio-mena requested review from a team as code owners July 3, 2024 17:55
@sergio-mena sergio-mena added bug Something isn't working block-sync labels Jul 3, 2024
Copy link
Collaborator

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

I would approve, but the >=1/3 vs >2/3 question remains open.

See associated comment (line 515).

internal/blocksync/reactor.go Outdated Show resolved Hide resolved
internal/blocksync/reactor_test.go Outdated Show resolved Hide resolved
internal/blocksync/reactor.go Outdated Show resolved Hide resolved
internal/blocksync/reactor.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Great job!

@adizere
Copy link
Member

adizere commented Jul 4, 2024

Is this safe to backport to 0.38/v1 (post-rc1 v1)? Or better not to because it's too significant behavior change? Not trying to imply here we should backport; in favor of limiting backporting actually.

@ValarDragon
Copy link
Collaborator

I think it's definitely safe to backport, it can't really affect mainnets as you need one Val w/ over 1/3 to do anything. (And it only helps users right now if it's on the 38 line)

@cason
Copy link
Collaborator

cason commented Jul 4, 2024

Is this safe to backport to 0.38/v1 (post-rc1 v1)?

We need to find a solution for v0.37.x too...

@sergio-mena
Copy link
Contributor Author

Is this safe to backport to 0.38/v1 (post-rc1 v1)?

To me, it's a bug, so unless there is big risk identified I'd backport it. Besides, this is clearly holding teams back, which are on v0.38.x/v0.37.x. Please reply if you don't agree.

@sergio-mena sergio-mena added this pull request to the merge queue Jul 4, 2024
@sergio-mena sergio-mena added backport-to-v0.37.x Tell Mergify to backport the PR to v0.37.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x backport-to-v1.x Tell Mergify to backport the PR to v1.x labels Jul 4, 2024
Merged via the queue into main with commit bd95579 Jul 4, 2024
43 checks passed
@sergio-mena sergio-mena deleted the sergio/blocksync-stalled-no-peers branch July 4, 2024 08:54
mergify bot pushed a commit that referenced this pull request Jul 4, 2024
…king the chain (#3406)

Partially addresses #3415

The a node has no peers, blocksync gets stuck without switching to
consesnus, because it needs info from other peers to have an idea of
maximum height.

However, there is an edge case (mainly when testing) where a validator
might have >2/3 of the voting power and other validators are not
started. In this case, we know we are blocking the chain, so we don't
need to stay in blockchain if the only condition is that we don't have
peers.

Moreover, in order to block a chain, 1/3 of the voting power is enough,
so the reasoning of this fix is the following:

* _I am a node and I am starting... shall I run blocksync?_
* _Well, looks like I have 1/3 of the voting power (or more) at my
current height... so there's no way the chain could advance in my
absence... so I don't need to blocksync"_

Explanation of commits:

* Commit 1: `e2e` testbed reproducing the issue
* Commit 2: commit with a trivial change to trigger `e2e` tests. Check
the error: ❌ next to the commit hash (3fb1057)
* Commit 3: Tentative fix. Although there is a ❌ next to the commit hash
(16a46ea), if you click on it, you'll see that `e2e` are passing now.
* Commit 4: revert commit2
* Commit 5: Move the check for "local node is blocking the chain"
outside the pool, as suggested by @cason
* Commit 6: Fixed unit tests

All further commits: addressing other comments and tidying up the code

---

#### PR checklist

- [x] Tests written/updated
- [x] 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~
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Daniel <[email protected]>
(cherry picked from commit bd95579)

# Conflicts:
#	internal/blocksync/reactor.go
mergify bot pushed a commit that referenced this pull request Jul 4, 2024
…king the chain (#3406)

Partially addresses #3415

The a node has no peers, blocksync gets stuck without switching to
consesnus, because it needs info from other peers to have an idea of
maximum height.

However, there is an edge case (mainly when testing) where a validator
might have >2/3 of the voting power and other validators are not
started. In this case, we know we are blocking the chain, so we don't
need to stay in blockchain if the only condition is that we don't have
peers.

Moreover, in order to block a chain, 1/3 of the voting power is enough,
so the reasoning of this fix is the following:

* _I am a node and I am starting... shall I run blocksync?_
* _Well, looks like I have 1/3 of the voting power (or more) at my
current height... so there's no way the chain could advance in my
absence... so I don't need to blocksync"_

Explanation of commits:

* Commit 1: `e2e` testbed reproducing the issue
* Commit 2: commit with a trivial change to trigger `e2e` tests. Check
the error: ❌ next to the commit hash (3fb1057)
* Commit 3: Tentative fix. Although there is a ❌ next to the commit hash
(16a46ea), if you click on it, you'll see that `e2e` are passing now.
* Commit 4: revert commit2
* Commit 5: Move the check for "local node is blocking the chain"
outside the pool, as suggested by @cason
* Commit 6: Fixed unit tests

All further commits: addressing other comments and tidying up the code

---

#### PR checklist

- [x] Tests written/updated
- [x] 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~
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Daniel <[email protected]>
(cherry picked from commit bd95579)

# Conflicts:
#	.changelog/v0.38.3/bug-fixes/3406-blocksync-dont-stall-if-blocking-chain.md
#	blocksync/reactor.go
#	blocksync/reactor_test.go
#	node/node.go
mergify bot pushed a commit that referenced this pull request Jul 4, 2024
…king the chain (#3406)

Partially addresses #3415

The a node has no peers, blocksync gets stuck without switching to
consesnus, because it needs info from other peers to have an idea of
maximum height.

However, there is an edge case (mainly when testing) where a validator
might have >2/3 of the voting power and other validators are not
started. In this case, we know we are blocking the chain, so we don't
need to stay in blockchain if the only condition is that we don't have
peers.

Moreover, in order to block a chain, 1/3 of the voting power is enough,
so the reasoning of this fix is the following:

* _I am a node and I am starting... shall I run blocksync?_
* _Well, looks like I have 1/3 of the voting power (or more) at my
current height... so there's no way the chain could advance in my
absence... so I don't need to blocksync"_

Explanation of commits:

* Commit 1: `e2e` testbed reproducing the issue
* Commit 2: commit with a trivial change to trigger `e2e` tests. Check
the error: ❌ next to the commit hash (3fb1057)
* Commit 3: Tentative fix. Although there is a ❌ next to the commit hash
(16a46ea), if you click on it, you'll see that `e2e` are passing now.
* Commit 4: revert commit2
* Commit 5: Move the check for "local node is blocking the chain"
outside the pool, as suggested by @cason
* Commit 6: Fixed unit tests

All further commits: addressing other comments and tidying up the code

---

#### PR checklist

- [x] Tests written/updated
- [x] 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~
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Daniel <[email protected]>
(cherry picked from commit bd95579)

# Conflicts:
#	blocksync/reactor_test.go
#	internal/blocksync/reactor.go
#	node/node.go
#	node/setup.go
sergio-mena added a commit that referenced this pull request Jul 4, 2024
sergio-mena added a commit that referenced this pull request Jul 4, 2024
…king the chain (#3406)

Partially addresses #3415

The a node has no peers, blocksync gets stuck without switching to
consesnus, because it needs info from other peers to have an idea of
maximum height.

However, there is an edge case (mainly when testing) where a validator
might have >2/3 of the voting power and other validators are not
started. In this case, we know we are blocking the chain, so we don't
need to stay in blockchain if the only condition is that we don't have
peers.

Moreover, in order to block a chain, 1/3 of the voting power is enough,
so the reasoning of this fix is the following:

* _I am a node and I am starting... shall I run blocksync?_
* _Well, looks like I have 1/3 of the voting power (or more) at my
current height... so there's no way the chain could advance in my
absence... so I don't need to blocksync"_

Explanation of commits:

* Commit 1: `e2e` testbed reproducing the issue
* Commit 2: commit with a trivial change to trigger `e2e` tests. Check
the error: ❌ next to the commit hash (3fb1057)
* Commit 3: Tentative fix. Although there is a ❌ next to the commit hash
(16a46ea), if you click on it, you'll see that `e2e` are passing now.
* Commit 4: revert commit2
* Commit 5: Move the check for "local node is blocking the chain"
outside the pool, as suggested by @cason
* Commit 6: Fixed unit tests

All further commits: addressing other comments and tidying up the code

---

- [x] Tests written/updated
- [x] 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~
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Daniel <[email protected]>
melekes pushed a commit that referenced this pull request Jul 5, 2024
…king the chain (backport #3406) (#3420)

Partially addresses #3415

The a node has no peers, blocksync gets stuck without switching to
consesnus, because it needs info from other peers to have an idea of
maximum height.

However, there is an edge case (mainly when testing) where a validator
might have >2/3 of the voting power and other validators are not
started. In this case, we know we are blocking the chain, so we don't
need to stay in blockchain if the only condition is that we don't have
peers.

Moreover, in order to block a chain, 1/3 of the voting power is enough,
so the reasoning of this fix is the following:

* _I am a node and I am starting... shall I run blocksync?_
* _Well, looks like I have 1/3 of the voting power (or more) at my
current height... so there's no way the chain could advance in my
absence... so I don't need to blocksync"_

Explanation of commits:

* Commit 1: `e2e` testbed reproducing the issue
* Commit 2: commit with a trivial change to trigger `e2e` tests. Check
the error: ❌ next to the commit hash (3fb1057)
* Commit 3: Tentative fix. Although there is a ❌ next to the commit hash
(16a46ea), if you click on it, you'll see that `e2e` are passing now.
* Commit 4: revert commit2
* Commit 5: Move the check for "local node is blocking the chain"
outside the pool, as suggested by @cason
* Commit 6: Fixed unit tests

All further commits: addressing other comments and tidying up the code

---

#### PR checklist

- [x] Tests written/updated
- [x] 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~
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #3406 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Sergio Mena <[email protected]>
Co-authored-by: Daniel <[email protected]>
sergio-mena added a commit that referenced this pull request Jul 5, 2024
sergio-mena added a commit that referenced this pull request Jul 5, 2024
…king the chain (#3406)

Partially addresses #3415

The a node has no peers, blocksync gets stuck without switching to
consesnus, because it needs info from other peers to have an idea of
maximum height.

However, there is an edge case (mainly when testing) where a validator
might have >2/3 of the voting power and other validators are not
started. In this case, we know we are blocking the chain, so we don't
need to stay in blockchain if the only condition is that we don't have
peers.

Moreover, in order to block a chain, 1/3 of the voting power is enough,
so the reasoning of this fix is the following:

* _I am a node and I am starting... shall I run blocksync?_
* _Well, looks like I have 1/3 of the voting power (or more) at my
current height... so there's no way the chain could advance in my
absence... so I don't need to blocksync"_

Explanation of commits:

* Commit 1: `e2e` testbed reproducing the issue
* Commit 2: commit with a trivial change to trigger `e2e` tests. Check
the error: ❌ next to the commit hash (3fb1057)
* Commit 3: Tentative fix. Although there is a ❌ next to the commit hash
(16a46ea), if you click on it, you'll see that `e2e` are passing now.
* Commit 4: revert commit2
* Commit 5: Move the check for "local node is blocking the chain"
outside the pool, as suggested by @cason
* Commit 6: Fixed unit tests

All further commits: addressing other comments and tidying up the code

---

- [x] Tests written/updated
- [x] 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~
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Daniel <[email protected]>
sergio-mena added a commit that referenced this pull request Jul 5, 2024
sergio-mena added a commit that referenced this pull request Jul 5, 2024
…king the chain (#3406)

Partially addresses #3415

The a node has no peers, blocksync gets stuck without switching to
consesnus, because it needs info from other peers to have an idea of
maximum height.

However, there is an edge case (mainly when testing) where a validator
might have >2/3 of the voting power and other validators are not
started. In this case, we know we are blocking the chain, so we don't
need to stay in blockchain if the only condition is that we don't have
peers.

Moreover, in order to block a chain, 1/3 of the voting power is enough,
so the reasoning of this fix is the following:

* _I am a node and I am starting... shall I run blocksync?_
* _Well, looks like I have 1/3 of the voting power (or more) at my
current height... so there's no way the chain could advance in my
absence... so I don't need to blocksync"_

Explanation of commits:

* Commit 1: `e2e` testbed reproducing the issue
* Commit 2: commit with a trivial change to trigger `e2e` tests. Check
the error: ❌ next to the commit hash (3fb1057)
* Commit 3: Tentative fix. Although there is a ❌ next to the commit hash
(16a46ea), if you click on it, you'll see that `e2e` are passing now.
* Commit 4: revert commit2
* Commit 5: Move the check for "local node is blocking the chain"
outside the pool, as suggested by @cason
* Commit 6: Fixed unit tests

All further commits: addressing other comments and tidying up the code

---

- [x] Tests written/updated
- [x] 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~
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Daniel <[email protected]>
sergio-mena added a commit that referenced this pull request Jul 5, 2024
…ing the chain (backport #3406) (#3421)

Partially addresses #3415

The a node has no peers, blocksync gets stuck without switching to
consesnus, because it needs info from other peers to have an idea of
maximum height.

However, there is an edge case (mainly when testing) where a validator
might have >2/3 of the voting power and other validators are not
started. In this case, we know we are blocking the chain, so we don't
need to stay in blockchain if the only condition is that we don't have
peers.

Moreover, in order to block a chain, 1/3 of the voting power is enough,
so the reasoning of this fix is the following:

* _I am a node and I am starting... shall I run blocksync?_
* _Well, looks like I have 1/3 of the voting power (or more) at my
current height... so there's no way the chain could advance in my
absence... so I don't need to blocksync"_

Explanation of commits:

* Commit 1: `e2e` testbed reproducing the issue
* Commit 2: commit with a trivial change to trigger `e2e` tests. Check
the error: ❌ next to the commit hash (3fb1057)
* Commit 3: Tentative fix. Although there is a ❌ next to the commit hash
(16a46ea), if you click on it, you'll see that `e2e` are passing now.
* Commit 4: revert commit2
* Commit 5: Move the check for "local node is blocking the chain"
outside the pool, as suggested by @cason
* Commit 6: Fixed unit tests

All further commits: addressing other comments and tidying up the code

---

#### PR checklist

- [x] Tests written/updated
- [x] 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~
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #3406 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Sergio Mena <[email protected]>
Co-authored-by: Daniel <[email protected]>
sergio-mena added a commit that referenced this pull request Jul 5, 2024
…ing the chain (backport #3406) (#3422)

Partially addresses #3415

The a node has no peers, blocksync gets stuck without switching to
consesnus, because it needs info from other peers to have an idea of
maximum height.

However, there is an edge case (mainly when testing) where a validator
might have >2/3 of the voting power and other validators are not
started. In this case, we know we are blocking the chain, so we don't
need to stay in blockchain if the only condition is that we don't have
peers.

Moreover, in order to block a chain, 1/3 of the voting power is enough,
so the reasoning of this fix is the following:

* _I am a node and I am starting... shall I run blocksync?_
* _Well, looks like I have 1/3 of the voting power (or more) at my
current height... so there's no way the chain could advance in my
absence... so I don't need to blocksync"_

Explanation of commits:

* Commit 1: `e2e` testbed reproducing the issue
* Commit 2: commit with a trivial change to trigger `e2e` tests. Check
the error: ❌ next to the commit hash (3fb1057)
* Commit 3: Tentative fix. Although there is a ❌ next to the commit hash
(16a46ea), if you click on it, you'll see that `e2e` are passing now.
* Commit 4: revert commit2
* Commit 5: Move the check for "local node is blocking the chain"
outside the pool, as suggested by @cason
* Commit 6: Fixed unit tests

All further commits: addressing other comments and tidying up the code

---

#### PR checklist

- [x] Tests written/updated
- [x] 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~
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #3406 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Sergio Mena <[email protected]>
Co-authored-by: Daniel <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Sep 11, 2024
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](https://github.com/informalsystems/unclog) to manage our
changelog)~
- ~[ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments~

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Anton Kaliaev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-v0.37.x Tell Mergify to backport the PR to v0.37.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x backport-to-v1.x Tell Mergify to backport the PR to v1.x block-sync bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants