Abstract away Tendermint height / IAVL version offset

[cwgoes]

Return the Tendermint commit height instead of the IAVL height on proof queries.

This will greatly simplify off-by-one issues on proofs in the relayer & other light clients.

cc @AdityaSripal @colin-axner @fedekunze

[cwgoes]

Suggestion by @alexanderbez - include both heights as metadata.

[AdityaSripal]

Including both heights directly in the query response would require a change to ABCI, since the store query returns abci.ResponseQuery which contains only one Height field.

I could change the abci struct to include both heights if that's the preferred solution. Though I think it makes more sense to make the ABCI query response return the tendermint height as opposed to IAVL height

[cwgoes]

Including both heights directly in the query response would require a change to ABCI, since the store query returns abci.ResponseQuery which contains only one Height field.

I could change the abci struct to include both heights if that's the preferred solution. Though I think it makes more sense to make the ABCI query response return the tendermint height as opposed to IAVL height

Hmm, that's annoying. Are we planning any changes to ABCI we could bundle this with? I still think both heights are preferable.

[milosevic]

Can you please clarify what do you mean by Tendermint commit height and by IAVL height in this context? Returning two heights is a potential source of confusion (and bugs) as we have trouble to figure out clear semantics even with one height :)

[cwgoes]

Can you please clarify what do you mean by Tendermint commit height and by IAVL height in this context? Returning two heights is a potential source of confusion (and bugs) as we have trouble to figure out clear semantics even with one height :)

For purposes of verifying proofs, the light client (e.g. in IBC, but also just running locally) needs to reason about the block height at which state updates (e.g. packets sent) executed during height n were committed, which is height n + 1, since transactions in block n aren't executed until after the block is committed, at which point the resulting state root is in the commit of block n + 1, as I understand it. Thus the "IAVL height" and "block height" are off by one.

[ancazamfir]

I am also not clear. From the relayer pov, in one possible implementation, currently:

1. The relayer gets a notification at height h that includes some Tx event for p. This is an executed transaction, included in block h. It can start querying immediately even if h+1 block with the state root is not yet committed. It sends ABCIQuery(h, p) and the response includes the proof and a height h.
2. Then the relayer waits for block h+1, retrieves the header and sends a ClientUpdate to destination chain, followed by the datagram with the proof obtained at height h. However it specifies h+1 as proof height.

With the new proposal I believe things would need to change:

1. The relayer gets a notification at height h for p and stores p.
2. Waits for h+1 and sends ABCIQuery(p, h+1) receiving the response with proof and h+1. It gets the header for h+1 and sends ClientUpdate and the datagram with without increasing the proof height.

Not sure I got this right but the new 1) is a bit of a pain. And still needs to be adjusted for other types of chains.

[ancazamfir]

Not sure I got this right but the new 1) is a bit of a pain. And still needs to be adjusted for other types of chains.

actually it's ok, I think it's just a small change in our design.

[cwgoes]

Yeah, I suppose due to the events we can't really avoid height offsets. Maybe this isn't worth doing.

It would be nice to at least abstract this somehow though...

[colin-axner]

I don't think any of the proposed solutions involving changing the actual handling of the Query function. The only difference is upstreaming the h + 1 so this only occurs once. So the only difference to relayers is not doing h+1 for proof height, but using the returned height from the query.

I do think this is a problem worth dealing with (even if it involves breaking some api and waiting for it to be included in a later release). This is because relayers are not going to want to add handlers in for specific ABCI applications. Doing h+1 for proof height works fine when relaying between two tendermint applications, but any ABCI application that does not associate a proof of block h with height h+1 will fail proof verification (please correct me if I am mistaken here). This would require relayers to add checks to see what sort of application they are relaying between. This hasn't really been an issue since tendermint is the dominant ABCI application (especially relative to the SDK)

I think returning the Query height as h+1 is worthy of consideration, but falls short as a stable option. It makes sense from a proof perspective (returning the height at which the proof can be proved), but I think querying without a set height would also result in the user thinking the block height is actually h+1 and not h (as it would use the latest height)

Adding another field to abci might make sense (block height and proof height) if it was common for ABCI apps to have block heights different from their proof height in storage. If this is not the case, it would seem a little strange to me to add a field just to satisfy tendermint characteristics. However, given the options, I think this would be the best approach and would give ABCI applications slightly more flexibility in how their protocol functions

[ancazamfir]

I don't think any of the proposed solutions involving changing the actual handling of the Query function. The only difference is upstreaming the h + 1 so this only occurs once. So the only difference to relayers is not doing h+1 for proof height, but using the returned height from the query.

To clarify ABCIQuery(p, h) will return value and proof for p and height h+1? Even if h+1 doesn't exist?

[colin-axner]

From my understanding, h+1 does exist in any query for h because the root of the consensus state for block h is the AppHash. The AppHash is the hash of the application state of the last block.

So a proof generated for block h is created using the AppHash set h+1, therefore h+1 does exist, it just may not have been signed and committed yet. If h had not been signed and committed yet it should be impossible to generate the proof since the application state could still change. Does that make sense?

[AdityaSripal]

I need to test this, but I believe the events are associated with the ABCI height (ie, the height in which the state gets committed), not the IAVL height. This would make things even easier for relayers, not the options mentioned above.

Not fully certain, I have to test this is the case tomorrow.

[cwgoes]

The path forward here isn't yet clear; let's come to consensus on this in a meeting & then tackle it again once we've updated the relayer. At minimum, we should abstract the height differential somewhere so that + 1 isn't scattered anywhere.

[milosevic]

One idea would be hiding +1 from the user, i.e., pushing this responsibility to the server side (Tendermint). So the flow would be: 1) ABCIQuery(p, h) which returns the result and the proof p that is related to AppHash after block at height h is executed, and 2) GetHeaderToProve(h) that would return header at height h+1. In this case a user (for example relayer) does not need to understand Tendermint blockchain internals (+1 offset). The potential issue would be the fact that GetHeaderToProve would potentially block until the block at height h+1 is committed.

[cwgoes]

For now, let's abstract this +1 in a helpfully named SDK function, then use it in the relayer, and avoid lots of +1s everywhere.

[cwgoes]

We should do this as soon as we start updating the relayer.