Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove validator address fields from vote extensions once we switch to ABCI++ #635

Open
Tracked by #2023
james-chf opened this issue Oct 18, 2022 · 1 comment
Open
Tracked by #2023
Labels
ABCI++ This issue should be done when we switch to ABCI++ (Tendermint v0.38) ethereum-bridge ledger post-mainnet Don't worry about this yet.

Comments

@james-chf
Copy link
Contributor

james-chf commented Oct 18, 2022

Once we switch to ABCI++, for vote extensions, we should be able to derive the validator's Namada address from their Tendermint address/key, rather than having to include it in the vote extension itself.

pub struct Vext {
/// The block height for which this [`Vext`] was made.
pub block_height: BlockHeight,
/// TODO: the validator's address is temporarily being included
/// until we're able to map a Tendermint address to a validator
/// address (see <https://github.com/anoma/namada/issues/200>)
pub validator_addr: Address,
/// The new ethereum events seen. These should be
/// deterministically ordered.
pub ethereum_events: Vec<EthereumEvent>,
}

@james-chf
Copy link
Contributor Author

Some extra context - for initial launch with Tendermint v0.37 where vote extensions are being packaged inside regular transactions, we do need the validator addresses inside the vote extensions, as we can't work them out from the Tendermint address as we won't have that.

With ABCI++ vote extensions, Tendermint will do some signature checking/replay protection for us like that a vote extension from validator X was signed by validator X's Tendermint key. While we are using vote extensions packaged inside regular transactions, we need to explicitly check that they were actually signed over by the validator specified in the address field (we will still do this application level signature check as well when we switch to ABCI++, in addition to the signature checking which Tendermint does - but we may not need the Namada address field anymore since we should be able to derive that from the Tendermint address we receive with each vote extesnion).

Currently (as of commit dc2586d in eth-bridge-integration branch), we do this application level signature check at two places:

  • when a vote extension transaction is attempted to be added to the mempool via a ledger's CheckTx ABCI method - this catches invalidly signed vote extension transactions from external users (i.e. using namadac) and also other nodes which may be trying to gossip such invalidly signed vote extension transactions - this check won't be applicable anymore once we switch to ABCI++ and stop using vote extension transactions
  • during ProcessProposal for any vote extension transactions included in a block proposal - this covers the case where hostile block proposers may directly inject invalidly signed vote extension transactions - we will still do a similar sort of this check when we switch to ABCI++, although all the vote extensions will be compressed into a digest rather than on chain individually

@james-chf james-chf changed the title Decide on whether to include validator addresses in vote extension digests Remove validator address fields from vote extensions once we switch to ABCI++ Jan 12, 2023
@james-chf james-chf added the ABCI++ This issue should be done when we switch to ABCI++ (Tendermint v0.38) label Jan 12, 2023
@james-chf james-chf moved this from Todo to Low-prio in Namada-Old Jan 12, 2023
@cwgoes cwgoes added the post-mainnet Don't worry about this yet. label May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ABCI++ This issue should be done when we switch to ABCI++ (Tendermint v0.38) ethereum-bridge ledger post-mainnet Don't worry about this yet.
Projects
No open projects
Status: Low-prio
Development

No branches or pull requests

2 participants