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

LastCommitRound can be -1 for heights > 1 #2737

Closed
melekes opened this issue Oct 31, 2018 · 2 comments · Fixed by #4970
Closed

LastCommitRound can be -1 for heights > 1 #2737

melekes opened this issue Oct 31, 2018 · 2 comments · Fixed by #4970
Labels
C:consensus Component: Consensus T:bug Type Bug (Confirmed)
Milestone
Wishlist

Comments

@melekes
Copy link
Contributor

melekes commented Oct 31, 2018 •
edited

p2p tests started failing when we added a check here for NewRoundStepMessage

func (m *NewRoundStepMessage) ValidateBasic() error {
...
	if (m.Height == 1 && m.LastCommitRound != -1) ||
		(m.Height > 1 && m.LastCommitRound < 0) {
		return errors.New("Invalid LastCommitRound (for 1st block: -1, for others: >= 0)")
	}

which is not supposed to happen

Logs:

2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_2[1256]: D[31106-10-31|15:27:31.494] Read PacketMsg                               module=p2p peer=0x9278f0 conn=MConn{172
.57.0.101:26656} packet="PacketMsg{20:C96A6FA808201803 T:1}"
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_2[1256]: D[31106-10-31|15:27:31.495] Received bytes                               module=p2p peer=0x9278f0 chID=32 msgByt
es=C96A6FA808201803
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_2[1256]: D[31106-10-31|15:27:31.495] Receive                                      module=consensus src="Peer{MConn{172.57
.0.101:26656} 55854e179f195424fbf969016e1a2ab47e1a0aea out}" chId=32 msg="[NewRoundStep H:32 R:0 S:RoundStepPropose LCR:0]"
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_3[1256]: D[31106-10-31|15:27:31.494] Read PacketMsg                               module=p2p peer=0x9278f0 conn=MConn{172
.57.0.101:26656} packet="PacketMsg{20:C96A6FA808201803 T:1}"
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_1[1256]: D[31106-10-31|15:27:31.496] Read PacketMsg                               module=p2p peer=0x9278f0 conn=MConn{172
.57.0.103:53290} packet="PacketMsg{20:C96A6FA808201803 T:1}"
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_1[1256]: D[31106-10-31|15:27:31.496] Received bytes                               module=p2p peer=0x9278f0 chID=32 msgByt
es=C96A6FA808201803
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_1[1256]: D[31106-10-31|15:27:31.496] Receive                                      module=consensus src="Peer{MConn{172.57
.0.103:53290} 6e0080aeae8ecfe813ddc358cbc02881af5c76d1 in}" chId=32 msg="[NewRoundStep H:32 R:0 S:RoundStepPropose LCR:0]"
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_3[1256]: D[31106-10-31|15:27:31.497] Received bytes                               module=p2p peer=0x9278f0 chID=32 msgByt
es=C96A6FA808201803
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_3[1256]: D[31106-10-31|15:27:31.497] Receive                                      module=consensus src="Peer{MConn{172.57
.0.101:26656} 55854e179f195424fbf969016e1a2ab47e1a0aea out}" chId=32 msg="[NewRoundStep H:32 R:0 S:RoundStepPropose LCR:0]"
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_2[1256]: D[31106-10-31|15:27:31.538] Flush                                        module=p2p peer=0x9278f0 conn=MConn{172
.57.0.101:26656}
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_1[1256]: D[31106-10-31|15:27:31.540] Read PacketMsg                               module=p2p peer=0x9278f0 conn=MConn{172
.57.0.102:58218} packet="PacketMsg{0:723A31CD T:1}"
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_1[1256]: D[31106-10-31|15:27:31.540] Received bytes                               module=p2p peer=0x9278f0 chID=0 msgByte
s=723A31CD
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_1[1256]: D[31106-10-31|15:27:31.540] Received message                             module=p2p src="Peer{MConn{172.57.0.102
:58218} 8a7f687c6e8ed894d860a1571285943cb50166e8 in}" chId=0 msg=[pexRequest]
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_1[1256]: D[31106-10-31|15:27:31.540] Send                                         module=p2p peer=0x9278f0 channel=0 conn
=MConn{172.57.0.102:58218} msgBytes=1CBB676B0A400A2836653030383061656165386563666538313364646333353863626330323838316166356337366431121000000000000000000000FFFFAC39006718A0
D0010A400A2838613766363837633665386564383934643836306131353731323835393433636235303136366538121000000000000000000000FFFFAC39006618A0D0010A400A286530323765343732343734326539
3632623833343231353533336666636338633531386534333366121000000000000000000000FFFFAC39006818A0D001
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_4[1256]: D[31106-10-31|15:27:31.547] Flush                                        module=p2p peer=0x9278f0 conn=MConn{172
.57.0.102:42800}
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_2[1256]: D[31106-10-31|15:27:31.548] Read PacketMsg                               module=p2p peer=0x9278f0 conn=MConn{172
.57.0.104:26656} packet="PacketMsg{40:5A433AB90801 T:1}"
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_2[1256]: D[31106-10-31|15:27:31.548] Received bytes                               module=p2p peer=0x9278f0 chID=64 msgByt
es=5A433AB90801
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_2[1256]: D[31106-10-31|15:27:31.548] Receive                                      module=blockchain src="Peer{MConn{172.5
7.0.104:26656} e027e4724742e962b834215533ffcc8c518e433f out}" chID=64 msg="[bcStatusResponseMessage 1]"
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_2[1256]: D[31106-10-31|15:27:31.549] Read PacketMsg                               module=p2p peer=0x9278f0 conn=MConn{172
.57.0.104:26656} packet="PacketMsg{20:C96A6FA808021804204C28FFFFFFFFFFFFFFFFFF01 T:1}"
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_2[1256]: D[31106-10-31|15:27:31.549] Received bytes                               module=p2p peer=0x9278f0 chID=32 msgByt
es=C96A6FA808021804204C28FFFFFFFFFFFFFFFFFF01
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_2[1256]: E[31106-10-31|15:27:31.549] Peer sent us invalid msg                     module=consensus peer="Peer{MConn{172.5
7.0.104:26656} e027e4724742e962b834215533ffcc8c518e433f out}" msg="[NewRoundStep H:2 R:0 S:RoundStepPrevote LCR:-1]" err="Invalid LastCommitRound (for 1st block: -1, for ot
hers: >= 0)"
2018-10-31T15:27:31+00:00 172.17.0.1 local_testnet_2[1256]: E[31106-10-31|15:27:31.549] Stopping peer for error                      module=p2p peer="Peer{MConn{172.57.0.10
4:26656} e027e4724742e962b834215533ffcc8c518e433f out}" err="Invalid LastCommitRound (for 1st block: -1, for others: >= 0)"

How to reproduce:

  1. use the snippet above
  2. run p2p tests
melekes added a commit that referenced this issue Oct 31, 2018
@melekes
relax LastCommitRound check
24f4e1e 
Refs #2737
@ebuchman ebuchman added T:bug Type Bug (Confirmed) C:consensus Component: Consensus labels Oct 31, 2018
@ebuchman ebuchman added this to the v1.0 milestone Oct 31, 2018
ebuchman pushed a commit that referenced this issue Nov 1, 2018
@melekes @ebuchman
validate reactor messages (#2711)
fb91ef7 
* validate reactor messages

Refs #2683

* validate blockchain messages

Refs #2683

* validate evidence messages

Refs #2683

* todo

* check ProposalPOL and signature sizes

* add a changelog entry

* check addr is valid when we add it to the addrbook

* validate incoming netAddr (not just nil check!)

* fixes after Bucky's review

* check timestamps

* beef up block#ValidateBasic

* move some checks into bcBlockResponseMessage

* update Gopkg.lock

Fix

```
grouped write of manifest, lock and vendor: failed to export github.com/tendermint/go-amino: fatal: failed to unpack tree object 6dcc6ddc143e116455c94b25c1004c99e0d0ca12
```

by running `dep ensure -update`

* bump year since now we check it

* generate test/p2p/data on the fly using tendermint testnet

* allow sync chains older than 1 year

* use full path when creating a testnet

* move testnet gen to test/docker/Dockerfile

* relax LastCommitRound check

Refs #2737

* fix conflicts after merge

* add small comment

* some ValidateBasic updates

* fixes

* AppHash length is not fixed
@cuonglm
Copy link
Contributor

cuonglm commented Dec 21, 2019

@melekes how to run p2p tests locally?

$ make test_p2p
docker rm -f rsyslog || true
rsyslog
rm -rf test/logs || true
mkdir test/logs
cd test/
docker run -d -v "logs:/var/log/" -p 127.0.0.1:5514:514/udp --name rsyslog voxxit/rsyslog
cfe8aefd52ca371b4258643e887cb360c7ba03450855508bb2fd784ba32f7b0d
cd ..
# requires 'tester' the image from above
bash test/p2p/test.sh tester
Error response from daemon: No such container: local_testnet_1
Error: No such container: local_testnet_1
Error response from daemon: No such container: local_testnet_2
Error: No such container: local_testnet_2
Error response from daemon: No such container: local_testnet_3
Error: No such container: local_testnet_3
Error response from daemon: No such container: local_testnet_4
Error: No such container: local_testnet_4
Error: No such network: local_testnet
Unable to find image 'tester:latest' locally
docker: Error response from daemon: pull access denied for tester, repository does not exist or may require 'docker login': denied: requested access to the resource is denied.
See 'docker run --help'.
make: *** [test_p2p] Error 125

@melekes
Copy link
Contributor Author

melekes commented Dec 21, 2019

You need to build docker image for tests:

make build_docker_test_image

make tools
make install

make test_p2p

@cuonglm cuonglm mentioned this issue Dec 21, 2019
Closed
5 tasks
@melekes melekes mentioned this issue Jun 5, 2020
Merged
melekes added a commit that referenced this issue Jun 10, 2020
@melekes @cuonglm
consensus: stricter on LastCommitRound check (#4970)
65909a1 
LastCommitRound should always be >= 0 for heights > 1.

In State.updateToState, last precommit is computed  only when round
greater than -1 and has votes. But "LastCommit" is always updated
regardless of the condition. If there's no last precommit, "LastCommit"
is set to (*types.VoteSet)(nil). That's why "LastCommit" can be -1 for
heights > 1.

To fix it, only update State.RoundState.LastCommit when there is last
precommit.

Fixes #2737

Co-authored-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C:consensus Component: Consensus T:bug Type Bug (Confirmed)
Projects
None yet
Milestone
Wishlist
3 participants
@melekes @ebuchman @cuonglm