Agreement: Add warning when decoding proposal of an unsupported consensus version

[ghost]

Summary

When the agreement receives a proposal from a consensus version it cannot yet support, the node disconnects from the sender and logs the fact that it was unable to decode the proposal without providing guidance on the consensus version issue. This PR adds a warning message to the log specifying that the agreement cannot handle the consensus version's proposals

[tsachiherman]

you approach would work correctly for future cases, but not for past cases, since the decoder is still going to fail.
I would suggest a slightly more conservative approach of limiting the change to the demux, and testing against the latest round supported by the ledger.

[ghost]

you approach would work correctly for future cases, but not for past cases, since the decoder is still going to fail. I would suggest a slightly more conservative approach of limiting the change to the demux, and testing against the latest round supported by the ledger.

you approach would work correctly for future cases, but not for past cases, since the decoder is still going to fail. I would suggest a slightly more conservative approach of limiting the change to the demux, and testing against the latest round supported by the ledger.

How do I limit the change to the demux if I need to extract the round number of the provided proposal? And second, when you say testing against the latest round supported by the ledger, do you mean just check if the specified round > max supported round?

The issue I'm after is when the local node is already behind, and cannot move forward since it's not supporting the newer protocols. In this case, using ledger.NextRound, call ledger.ConsensusVersion and then check if we have this version in the config.Consensus map. If we don't, it means that we can't support the next round, and therefore, this message is likely to be unparsable for that reason. ( there is a fail case in this logic, but it won't make any functional harm. The node is out of date anyway )

[codecov-commenter]

Codecov Report

Merging #3730 (9513bd9) into master (d3dc437) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #3730      +/-   ##
==========================================
- Coverage   49.68%   49.66%   -0.03%     
==========================================
  Files         392      392              
  Lines       68588    68595       +7     
==========================================
- Hits        34076    34065      -11     
- Misses      30766    30779      +13     
- Partials     3746     3751       +5     

Impacted Files	Coverage Δ
agreement/demux.go	89.65% <0.00%> (-3.21%)	⬇️
crypto/merkletrie/trie.go	66.42% <0.00%> (-2.19%)	⬇️
crypto/merkletrie/node.go	91.62% <0.00%> (-1.87%)	⬇️
catchup/peerSelector.go	98.95% <0.00%> (-1.05%)	⬇️
cmd/tealdbg/debugger.go	71.42% <0.00%> (-0.99%)	⬇️
data/transactions/verify/txn.go	44.15% <0.00%> (-0.87%)	⬇️
network/requestTracker.go	70.25% <0.00%> (-0.87%)	⬇️
ledger/acctupdates.go	68.42% <0.00%> (-0.67%)	⬇️
catchup/service.go	69.38% <0.00%> (+0.74%)	⬆️
network/wsPeer.go	69.16% <0.00%> (+1.11%)	⬆️
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3dc437...9513bd9. Read the comment docs.

[tsachiherman]

The logic here looks correct. Two small tweaks:

1. if you're writing this new log message, then the other log message is no longer needed.
2. could you change the log message to something like
   d.log.Warnf("received proposal message was ignored. The node binary doesn't support the next network consensus (%v) and would no longer be able to process agreement messages", cv)

[tsachiherman]

looks good to me. I'd rather having these two identical cases merged, but logically this looks correct.

[ghost]

looks good to me. I'd rather having these two identical cases merged, but logically this looks correct.

Whats a better way of writing this? I thought about

// check protocol version
					cv, err := d.ledger.ConsensusVersion(d.ledger.NextRound())
					if err == nil {
						if _, ok := config.Consensus[cv]; !ok {
							d.log.Warnf("received proposal message was ignored. The node binary doesn't support the next network consensus (%v) and would no longer be able to process agreement messages", cv)
                                                          net.Disconnect(raw.MessageHandle)
					                d.UpdateEventsQueue(eventQueueTokenizing[tag], 0)
					                continue
						}
					}
					net.Disconnect(raw.MessageHandle)
					d.UpdateEventsQueue(eventQueueTokenizing[tag], 0)
					continue

[tsachiherman]

I was thinking about something like this, just to keep the current flow as is :

					warnMsg := fmt.Printf("disconnecting from peer: error decoding message tagged %v: %v", tag, err)
					// check protocol version
 					cv, err := d.ledger.ConsensusVersion(d.ledger.NextRound())
 					if err == nil {
 						if _, ok := config.Consensus[cv]; !ok {
 							warnMsg  = fmt.Printf("received proposal message was ignored. The node binary doesn't support the next network consensus (%v) and would no longer be able to process agreement messages", cv)
 						}
 					}
                                        d.log.Warn(warnMsg)