remove UniCatchupReq message type #82
Conversation
|
IMO we should assert that the messages are ignored to explicitly maintain history/legacy/whatever you want to call it |
There was a problem hiding this comment.
I agree with @aphelionz here, but on the other hand - how valuable such a test is to you?
I would say it would be one of the least interesting tests - so the effort to adapt to the change - is it really worth it?
If not, I would vote to keep a simple comment about that test so we have it documented in our codebase if anybody ever raises a question.
| @@ -89,7 +89,6 @@ The fuzz tests aim to buttress the message conformance tests with extra verifica | |||
| | PingReplyTag | WS data (Tag: pj) | ✅ | `C009`, `R003` | | |||
| | ProposalPayloadTag | WS data (Tag: PP) | ✅ | `C007`, `C013`, `R003`, `R004` | | |||
| | StateProofSigTag | WS data (Tag: SP) | ❌ | `R003` | | |||
| | UniCatchupReqTag | WS data (Tag: UC) | ✅ | `C010`, `R003` | | |||
There was a problem hiding this comment.
This is what I would maybe leave here (related to my general comment)
| UniCatchupReqTag (deprecated) | WS data (Tag: UC) | ☑️ | Test removed |
There was a problem hiding this comment.
And maybe we could update the commit message to (we use the conventional commits specification here):
chore: remove UniCatchupReq message type
in any case, LGTM!
Requests using the UniCatchupReq message type were removed in algorand/go-algorand#1916 (Feb 2021), and there have been several consensus releases since then, so clients effectively no longer use this message. We recently removed the handler for this tag from the codebase in algorand/go-algorand#4517 (Feb 2023).
This removes this message type from the Ziggurat tests too. (Unless you think it would be better to assert that the UniCatchupReqTag messages were ignored ...?)