proposal: remove ValidatorAddress from Vote

[ebuchman]

It's useful from a debugging standpoint to have the address in the vote but we don't sign it and its redundant since we include the ValidatorIndex, so it's really just 20-bytes in every vote that we could do without.

Before we remove it we should make sure we can actually measure the performance benefit we'll get. We should also first consider the relationship between validator set, vote set, and consensus state in the code and see if it can be improved.

[ValarDragon]

Its actually at least 21 bytes, b/c protobuf field number.
Assuming 1 round blocks:

21 bytes per vote * 2 votes per validator per block * 100 validators = 4kb extra per block. This definitely makes sense to me to remove prelaunch. Until we get state syncing, we require many archive nodes on the network.

At 10 second block times (significantly longer then our testnet block times to drive the point) we have 259200 blocks at 1 month, which means that this change would save us slightly over 1GB on every archive node at the 1 month mark. (And even more presuming faster block times) This make sense as a thing for us to change, as it definitely feels like it buys us more time on implementing state syncing imo.

Before we remove it we should make sure we can actually measure the performance benefit we'll get.

Do you mean amino encoding the struct w/ and w/o the address field and measuring the size difference? Or do you want benchmarks on changes to sync times.

[ebuchman]

we require many archive nodes on the network.

These votes aren't stored in the blockchain. They're just part of the real time bandwidth cost, not the long term disk cost. So this shouldn't actually affect when we need to implement state-syncing by.

Yes we write them to the WAL, but we already rotate that. And the commit is currently being refactored to just be Signature and Timestamp (#2179)

Do you mean amino encoding the struct w/ and w/o the address field and measuring the size difference? Or do you want benchmarks on changes to sync times.

It's strictly about the real-time consensus performance time. Ie. how much faster can the live network commit blocks when its votes are 21 bytes smaller

Would you still prioritize this pre-launch?

[ValarDragon]

I forgot that prevotes / precommits aren't included in a block, its just the commit. My bad.

This means that at 1 month, we would expect 1 extra GB between every pair of peers on the gossip network. Given that the average node is set to get 50 peers by default (Perhaps less, since most are inbound), that means 50 GB extra bandwidth per pair of nodes. At 100mbps, that translates to a node taking (at least) an extra 1.1 hours. Figuring out the avg amount of Round trips needed per block in our expected network topology seems super complicated / perhaps impossible to measure, but I don't think 1.1 hours of delay per node over the course of a month should warrant this being a launch priority.

I agree then, post-launch sounds good to me.

[UnitylChaos]

I think we should do this as part of #2179. Without removing the ValidatorAddress we end up with inconsistent situations, where sometimes votes have them and sometimes they don't. Since any time a vote is reconstructed from the commit it won't have it, unless we use the validator set and ValidatorIndex to reconstruct. We could make the effort to always put the address back in, but I think that would be more work and overhead than simply removing them everywhere, since they aren't actually being used for anything critical. The only area where removing them has complications is in printing votes, but I think it would be reasonable for votes to print without ValidatorAddress and just have the index to reference.

[ebuchman]

I think we should do this as part of #2179.

I don't think we should include this in #2179. We want to try to minimize the number of changes happening at once, even though they're related.

Since any time a vote is reconstructed from the commit it won't have it, unless we use the validator set and ValidatorIndex to reconstruct.

Can we maybe try to minimize the number of places we need to do this conversion?

The only area where removing them has complications is in printing votes, but I think it would be reasonable for votes to print without ValidatorAddress and just have the index to reference.

I don't think that's reasonable. Addresses are too useful a visual crutch for looking at logs, especially as validator set order may be changing rapidly. The whole point of including the addresses in the votes is to ease the debugging/visualization experience.

[ValarDragon]

Why can't we include the validator address as an unexported variable? If we're auto generating from .proto files, then we could make a wrapper struct.

[ebuchman]

Why can't we include the validator address as an unexported variable? If we're auto generating from .proto files, then we could make a wrapper struct.

For what benefit?

[ValarDragon]

So that we can print the address in all debug logs in the same manner, but we don't send this field over the wire.

[ebuchman]

So that we can print the address in all debug logs in the same manner, but we don't send this field over the wire.

But what good is printing the field if we're not even going to receive it?!

Here is the flow:

• receive vote over network
• log vote (including the validator address)

[ValarDragon]

Maybe I'm misunderstanding. The flow I'm proposing is:

Receive vote
Inject address into unexported field
Log, and do everything else as we do currently

Seems like this minimizes code changes, while enabling us to get what we want.

[ebuchman]

I don't think it's worth the trouble. We're not really benefitting by removing the address, just causing more complexity by trying to inject it back in.

[ebuchman]

Closing this for now. It will be tracked in the ADR that gets written for #2179 (comment)