go/consensus/supplementarysanity: Fix checks for legacy validators

[ptrus]

Before, when upgrading from 22.2.x the supplementary-sanity checks would fail for validator nodes that did not yet re-register with added P2P address.

(Unrelated change) update minNodeDescriptorVersion to 3, because V2 descriptors are automatically migrated to V3 in deserialization, so no need to allow V2 descriptors.

[codecov]

Codecov Report

Merging #5168 (886aea1) into master (364c7f4) will increase coverage by 0.25%.
The diff coverage is 56.09%.

❗ Current head 886aea1 differs from pull request most recent head f790a1f. Consider uploading reports for the commit f790a1f to get more accurate results

@@            Coverage Diff             @@
##           master    #5168      +/-   ##
==========================================
+ Coverage   66.73%   66.98%   +0.25%     
==========================================
  Files         497      499       +2     
  Lines       53300    53361      +61     
==========================================
+ Hits        35570    35745     +175     
+ Misses      13363    13256     -107     
+ Partials     4367     4360       -7     

Impacted Files	Coverage Δ
go/common/node/node.go	73.52% <ø> (+2.45%)	⬆️
go/keymanager/api/api.go	75.86% <0.00%> (-7.00%)	⬇️
go/registry/api/api.go	54.39% <ø> (+1.81%)	⬆️
go/consensus/tendermint/apps/keymanager/genesis.go	56.14% <33.33%> (-2.05%)	⬇️
...nsensus/tendermint/apps/keymanager/transactions.go	47.72% <40.00%> (-1.06%)	⬇️
...onsensus/tendermint/apps/keymanager/state/state.go	67.21% <46.66%> (-6.70%)	⬇️
...o/consensus/tendermint/apps/keymanager/messages.go	56.52% <56.52%> (ø)
...nsus/tendermint/apps/supplementarysanity/checks.go	46.90% <60.00%> (+0.05%)	⬆️
go/keymanager/api/sanity_check.go	65.21% <65.21%> (ø)
go/oasis-node/cmd/genesis/genesis.go	54.36% <100.00%> (+0.26%)	⬆️
... and 35 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[kostko]

Suggested change

	// XXX: Remove this after 22.3.x.
	// XXX: Remove this after 23.0.x.

[kostko]

Suggested change

	// to allow non-migrated nodes (for 22.3.x these are validator nodes with no p2p address).
	// to allow non-migrated nodes (for 23.0.x these are validator nodes with no p2p address).

[peternose]

These changes also force that the public key has to be in the entity's node list for the first MaxNodeExpiration epochs (see 546). Not sure if this is a problem.

[kostko]

Note that this is only for supplementary sanity checks which are disabled by default, but yeah this seems like an unintended consequence as it could cause the sanity checks to incorrectly fail. Maybe it would be better to use the isSanityCheck flag in VerifyRegisterNodeArgs, e.g.:

	switch isGenesis || isSanityCheck {
	case true:
		// Allow legacy descriptor with optional p2p address for validator.

[ptrus]

Changed to use the isSanityCheck solution. Tried to avoid this before, since it's not the "correct fix", but it does make the logic much simpler, and it's also only a temporary special case anyway.