PBTS: Proposal's reception time should be persisted to the WAL

[cason]

Originally: tendermint/tendermint#8954

For implementing PBTS mechanism, in particular for registering the reception time of Proposal message, the consensus reactor registers the ReceiveTime of every message in the msgInfo instance delivered to the consensus protocol.

Messages received and accepted by the consensus protocol are written to the WAL, allowing nodes to recover from crashes. However, the msgInfo proto, that is written to the WAL, does not include the ReceiveTime field.

As a result, when replaying messages from the WAL their ReceiveTime is nil (zero time), which leads nodes to reject a replayed Proposal message upon recovery, even though the same Proposal message was accepted during regular execution. This leads recovering validator nodes to equivocate.

@sergio-mena has observed this behavior in e2e tests and reported it in detail in tendermint/tendermint#8739. He also proposed a workaround to avoid tests from failing (tendermint/tendermint#8774), by disabling the PBTS timely verification of the Proposal's timestamp when in replay mode. The problem of this approach is that it can also lead to equivocating behavior: if the Proposal's timestamp is rejected by PBTS during regular execution, then upon recovery the same Proposal can be accepted, as the timely verification is disabled.

The straightforward solution for this issue is to write the ReceiveTime field of msgInfo struct to the WAL during regular operation, and to read the same field from the WAL when replaying messages, so that the protocol processes Proposal messages using the same temporal information employed before, during regular operation.

The downside of this solution is to increase the size and the amount of the information written to the WAL, with a potential performance impact. The byte-size of a timestamp is typically 13 bytes, but the main point here is that the ReceiveTime is not relevant for most of the received and broadcast messages. In fact, this field is only employed for Proposal messages. For all the other messages, having it set to zero (nil) during recovery does not constitute a problem.

DoD

• Implement solution (WAL should persist the proposal receive time for proposals made)
  • feat(pbts): Adds timestamp to the msgInfo written into the WAL #2388
• Test if the solution works using a manuale2e test
  • See result: feat(pbts): Adds timestamp to the msgInfo written into the WAL #2388 (comment)
• Optional: test if the solution works by introducing a new test unit
  • This means that the test should fail without the fix, and pass with the fix
  • The current test units infra renders it very hard to test the recovery of a node in consensus' terms

[cason]

The behaviour of the TestWALCrash consensus unit test reflects this issue, see: #2320 (comment)

[lasarojc]

We could add the timestamp as optional to protobuf and only fill it for proposal messages. This should reduce the impact of the field on the WAL.

[sergio-mena]

Another thought: as we are not activating PBTS upon upgrade, but via the "enable height" in the future, we don't need to consider backwards compatibility of the proposal format in the WAL (I'm happy we adopted that way of activating PBTS).

We need to consider, tho, that non-PBTS heights may be missing the optional field also in the proposal (which I don't think it is a problem, as BFT Time does not need it).

[lasarojc]

PR #2388 improves test TestWALEncoderDecoder, which fails if applied to feature/pbts as follows (excerpt):

        	Error Trace:	/Users/lasaro/code/informal/cometbft/internal/consensus/wal_test.go:135
        	Error:      	Not equal: 
        	            	expected: consensus.msgInfo{Msg:(*consensus.ProposalMessage)(0x140000e2310), PeerID:"Nobody", ReceiveTime:time.Date(2024, time.February, 21, 12, 24, 11, 230093000, time.UTC)}
        	            	actual  : consensus.msgInfo{Msg:(*consensus.ProposalMessage)(0x140000e2378), PeerID:"Nobody", ReceiveTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC)}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -35,4 +35,4 @@
        	            	  ReceiveTime: (time.Time) {
        	            	-  wall: (uint64) 230093000,
        	            	-  ext: (int64) 63844115051,
        	            	+  wall: (uint64) 0,
        	            	+  ext: (int64) 0,
        	            	   loc: (*time.Location)(<nil>)
        	Test:       	TestWALEncoderDecoder

The test passes on #2388

[cason]

We need to consider, tho, that non-PBTS heights may be missing the optional field also in the proposal (which I don't think it is a problem, as BFT Time does not need it).

If we can indeed make this field option, we are good here. For BFT Time this field is irrelevant, for PBTS, if it was not saved is because we were not using PBTS or we are an incorrect node.

[cason]

Is this solved?

[sergio-mena]

@cason Yes, the description of #2388 says it closes this issue, and it's merged already. I think it wasn't closed automatically because the PR is not on main. Closing it

[sergio-mena]

Closed by #2388