commit_sig absent blockID fails validation (if validator address is present)

[greg-szabo]

Bug description

ADR-025 decision states:

Absent Votes: we include absent votes explicitly with no Signature or Timestamp but with the ValidatorAddress.

But the current Rust implementation of commit_sig defines a validate function that fails with error if the BlockID is "FlagAbsent" and a validator address is present.

Although the neccessity of the validator address is questioned in several places, the current decision is to include it in CommitSigs. So, the Rust implementation should follow that.

Proposed solution

Rewrite the CommitSig in a simpler form (possibly as an enum) and abstract away the serialization and type validation. (I'm planning to give an example of that soon.)

Short term fix: remove the check in the validate function.

[greg-szabo]

Here's a passing test to show the issue:

#[test]
fn deserialize_commit_sig_absent_vote() {
    let incoming = r#"
    {
        "block_id_flag": 1,
        "validator_address": "",
        "timestamp": "1970-01-01T00:00:00Z",
        "signature": null
    }
    "#;

    let result: CommitSig = serde_json::from_str(&incoming).unwrap();

    assert_eq!(result.block_id_flag, tendermint::block::commit_sig::BlockIDFlag::BlockIDFlagAbsent);
    assert_eq!(result.validator_address, None);
    assert_eq!(result.timestamp, tendermint::time::Time::unix_epoch());
    assert_eq!(result.signature, None);
}

Validator address should not be empty and timestamp should be null or omitted.

[liamsi]

But the current Rust implementation of commit_sig defines a validate function that fails with error if the BlockID is "FlagAbsent" and a validator address is present.

Although the neccessity of the validator address is questioned in several places, the current decision is to include it in CommitSigs. So, the Rust implementation should follow that.

Note that while the ADR clearly states the val addr should be included as you say, the go implementation currently does this:
https://github.com/tendermint/tendermint/blob/e4427b72928e5a8066f75cdc98e3d84a038cc17b/types/block.go#L539-L548

So either a decisions was made to deviate from the ADR on purpose and the ADR wasn't updated or the go implementation behaves accidentally as it does (which the rust impl followed).

[greg-szabo]

Seems like a good point to raise on the next Tendermint call.

[greg-szabo]

Closing in favor of #260.