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.

Sign up for GitHub

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

Jump to bottom

Delay period on a per-connection basis #507

Merged
merged 3 commits into from
Dec 5, 2020
Merged

Delay period on a per-connection basis #507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e93172e
Delay period on a per-connection basis
cwgoes Dec 3, 2020
3fe9439
Merge branch 'master' into cwgoes/connection-processed-time
cwgoes Dec 3, 2020
86ea753
Nits
cwgoes Dec 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions spec/ics-002-client-semantics/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,8 @@ at a particular finalised height (necessarily associated with a particular commi
Client types must define functions to authenticate internal state of the state machine which the client tracks.
Internal implementation details may differ (for example, a loopback client could simply read directly from the state and require no proofs).

- The `delayPeriod` is passed to packet-related verification functions in order to allow packets to specify a period which must pass after a header is verified before the packet is allowed to be processed.
milosevic marked this conversation as resolved.
Show resolved Hide resolved

##### Required functions

`verifyClientConsensusState` verifies a proof of the consensus state of the specified client stored on the target machine.
Expand Down Expand Up @@ -366,6 +368,7 @@ type verifyChannelState = (
type verifyPacketData = (
clientState: ClientState,
height: Height,
delayPeriod: uint64,
prefix: CommitmentPrefix,
proof: CommitmentProof,
portIdentifier: Identifier,
Expand All @@ -381,6 +384,7 @@ type verifyPacketData = (
type verifyPacketAcknowledgement = (
clientState: ClientState,
height: Height,
delayPeriod: uint64,
prefix: CommitmentPrefix,
proof: CommitmentProof,
portIdentifier: Identifier,
Expand All @@ -396,6 +400,7 @@ type verifyPacketAcknowledgement = (
type verifyPacketReceiptAbsence = (
clientState: ClientState,
height: Height,
delayPeriod: uint64,
prefix: CommitmentPrefix,
proof: CommitmentProof,
portIdentifier: Identifier,
Expand All @@ -410,6 +415,7 @@ type verifyPacketReceiptAbsence = (
type verifyNextSequenceRecv = (
clientState: ClientState,
height: Height,
delayPeriod: uint64,
prefix: CommitmentPrefix,
proof: CommitmentProof,
portIdentifier: Identifier,
Expand Down Expand Up @@ -770,6 +776,7 @@ function verifyChannelState(
function verifyPacketData(
clientState: ClientState,
height: Height,
delayPeriod: uint64,
prefix: CommitmentPrefix,
proof: CommitmentProof,
portIdentifier: Identifier,
Expand All @@ -784,6 +791,7 @@ function verifyPacketData(
function verifyPacketAcknowledgement(
clientState: ClientState,
height: Height,
delayPeriod: uint64,
prefix: CommitmentPrefix,
proof: CommitmentProof,
portIdentifier: Identifier,
Expand All @@ -799,6 +807,7 @@ function verifyPacketReceiptAbsence(
clientState: ClientState,
height: Height,
prefix: CommitmentPrefix,
delayPeriod: uint64,
proof: CommitmentProof,
portIdentifier: Identifier,
channelIdentifier: Identifier,
Expand All @@ -811,6 +820,7 @@ function verifyPacketReceiptAbsence(
function verifyNextSequenceRecv(
clientState: ClientState,
height: Height,
delayPeriod: uint64,
prefix: CommitmentPrefix,
proof: CommitmentProof,
portIdentifier: Identifier,
Expand Down
27 changes: 16 additions & 11 deletions spec/ics-003-connection-semantics/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ interface ConnectionEnd {
clientIdentifier: Identifier
counterpartyClientIdentifier: Identifier
version: string | []string
delayPeriod: uint64
}
```

Expand All @@ -94,6 +95,7 @@ interface ConnectionEnd {
- The `counterpartyClientIdentifier` field identifies the client on the counterparty chain associated with this connection.
- The `version` field is an opaque string which can be utilised to determine encodings or protocols for channels or packets utilising this connection.
If not specified, a default `version` of `""` should be used.
- The `delayPeriod` indicates a period that must elapse after validation of a header before a packet, acknowledgement, proof of receipt, or timeout can be processed.

### Store paths

Expand Down Expand Up @@ -173,7 +175,7 @@ function verifyPacketData(
sequence: Height,
data: bytes) {
client = queryClient(connection.clientIdentifier)
return client.verifyPacketData(connection, height, connection.counterpartyPrefix, proof, portIdentifier, channelIdentifier, data)
return client.verifyPacketData(connection, height, connection.delayPeriod, connection.counterpartyPrefix, proof, portIdentifier, channelIdentifier, data)
}

function verifyPacketAcknowledgement(
Expand All @@ -185,7 +187,7 @@ function verifyPacketAcknowledgement(
sequence: uint64,
acknowledgement: bytes) {
client = queryClient(connection.clientIdentifier)
return client.verifyPacketAcknowledgement(connection, height, connection.counterpartyPrefix, proof, portIdentifier, channelIdentifier, acknowledgement)
return client.verifyPacketAcknowledgement(connection, height, connection.delayPeriod, connection.counterpartyPrefix, proof, portIdentifier, channelIdentifier, acknowledgement)
}

function verifyPacketReceiptAbsence(
Expand All @@ -196,7 +198,7 @@ function verifyPacketReceiptAbsence(
channelIdentifier: Identifier,
sequence: uint64) {
client = queryClient(connection.clientIdentifier)
return client.verifyPacketReceiptAbsence(connection, height, connection.counterpartyPrefix, proof, portIdentifier, channelIdentifier)
return client.verifyPacketReceiptAbsence(connection, height, connection.delayPeriod, connection.counterpartyPrefix, proof, portIdentifier, channelIdentifier)
}

function verifyNextSequenceRecv(
Expand All @@ -207,7 +209,7 @@ function verifyNextSequenceRecv(
channelIdentifier: Identifier,
nextSequenceRecv: uint64) {
client = queryClient(connection.clientIdentifier)
return client.verifyNextSequenceRecv(connection, height, connection.counterpartyPrefix, proof, portIdentifier, channelIdentifier, nextSequenceRecv)
return client.verifyNextSequenceRecv(connection, height, connection.delayPeriod, connection.counterpartyPrefix, proof, portIdentifier, channelIdentifier, nextSequenceRecv)
}

function getTimestampAtHeight(
Expand Down Expand Up @@ -296,7 +298,8 @@ function connOpenInit(
counterpartyPrefix: CommitmentPrefix,
clientIdentifier: Identifier,
counterpartyClientIdentifier: Identifier,
version: string) {
version: string,
delayPeriod: uint64) {
identifier = generateIdentifier()
abortTransactionUnless(provableStore.get(connectionPath(identifier)) == null)
state = INIT
Expand All @@ -308,7 +311,7 @@ function connOpenInit(
versions = getCompatibleVersions()
}
connection = ConnectionEnd{state, "", counterpartyPrefix,
clientIdentifier, counterpartyClientIdentifier, versions}
clientIdentifier, counterpartyClientIdentifier, versions, delayPeriod}
provableStore.set(connectionPath(identifier), connection)
addConnectionToClient(clientIdentifier, identifier)
}
Expand All @@ -324,6 +327,7 @@ function connOpenTry(
counterpartyClientIdentifier: Identifier,
clientIdentifier: Identifier,
counterpartyVersions: string[],
delayPeriod: uint64,
proofInit: CommitmentProof,
proofConsensus: CommitmentProof,
proofHeight: Height,
Expand All @@ -336,7 +340,8 @@ function connOpenTry(
previous.counterpartyConnectionIdentifier === "" &&
previous.counterpartyPrefix === counterpartyPrefix &&
previous.clientIdentifier === clientIdentifier &&
previous.counterpartyClientIdentifier === counterpartyClientIdentifier))
previous.counterpartyClientIdentifier === counterpartyClientIdentifier &&
previous.delayPeriod === delayPeriod))
identifier = previousIdentifier
} else {
// generate a new identifier if the passed identifier was the sentinel empty-string
Expand All @@ -345,11 +350,11 @@ function connOpenTry(
abortTransactionUnless(consensusHeight < getCurrentHeight())
expectedConsensusState = getConsensusState(consensusHeight)
expected = ConnectionEnd{INIT, "", getCommitmentPrefix(), counterpartyClientIdentifier,
clientIdentifier, counterpartyVersions}
clientIdentifier, counterpartyVersions, delayPeriod}
versionsIntersection = intersection(counterpartyVersions, previous !== null ? previous.version : getCompatibleVersions())
version = pickVersion(versionsIntersection) // throws if there is no intersection
connection = ConnectionEnd{TRYOPEN, counterpartyConnectionIdentifier, counterpartyPrefix,
clientIdentifier, counterpartyClientIdentifier, version}
clientIdentifier, counterpartyClientIdentifier, version, delayPeriod}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that delayPeriod is a parameter defined by the chain to provide security to its users. Shouldn't it be reasonable to give each side of the connection possibility to choose delayPeriod? Also what happen in crossing hello case when different delayPeriod is chosen by counter parties?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think chains are defining the delay period in this implementation. As you say, each side of the conenction has the possibility to choose the delayPeriod.

In the case of crossing hello's, the connection handshake will succeed with different delay periods. It can be viewed the same as if the crossing hellos selected different client identifiers (they cannot connect with each other because they have different parameters)

Copy link
Member

@AdityaSripal AdityaSripal Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of crossing hello's, the connection handshake will succeed with different delay periods.

I think you mean to say it will fail.

We want the initiator of the handshake to define the delayPeriod on both ends. Consider that I want to create a connection with ChainA and chainB, and I want to use the delayPeriod feature to protect packets going across this connection from misbehaviour on either side.

I submit a ConnOpenInit to chainA with a delayPeriod=60. Now, I intend to also set a delayPeriod on chainB, however another relayer has submitted a ConnOpenTry before me on chainB with delayPeriod=0. This tx passes, and I now have a connection with delayPeriod on chainA and no delayPeriod on chainB.

This is clearly not what I wanted as the initiator. This is why we fix the delay period at the INIT step to ensure that the final connection, regardless of which relayer completes it, has the delay period that was specified by the initiating relayer. In OpenTry, we enforce that this connectionEnd also has a delay period specified by the initiating relayer.
As @colin-axner said, in case of crossing hello's that don't agree on delay period, we error just as we do in other cases where parameters do not match.

Now, it is possible that in the INIT step, we could define different delay periods for each chain. So ConnOpenInit might specify delayPeriod AND counterpartyDelayPeriod.
I could see this being potentially useful in cases where a highly secure chain is connecting to a more risky chain, the initiating relayer may specify no delay period for the cosmos-hub client but a high delay period for the risky-chain client.

However, this is a (slight) complication that doesn't seem all that useful imo, and unnecessary for IBC 1.0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that giving possibility for each party to choose a delayPeriod would conflict with optimistic sends, so it seems like good tradeoff to have it the same on both ends.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aye, we discussed this a bit internally, I guess it should be summarised somewhere public. Great summary @AdityaSripal!

abortTransactionUnless(connection.verifyConnectionState(proofHeight, proofInit, counterpartyConnectionIdentifier, expected))
abortTransactionUnless(connection.verifyClientConsensusState(
proofHeight, proofConsensus, counterpartyClientIdentifier, consensusHeight, expectedConsensusState))
Expand Down Expand Up @@ -377,7 +382,7 @@ function connOpenAck(
expectedConsensusState = getConsensusState(consensusHeight)
expected = ConnectionEnd{TRYOPEN, identifier, getCommitmentPrefix(),
connection.counterpartyClientIdentifier, connection.clientIdentifier,
version}
version, connection.delayPeriod}
abortTransactionUnless(connection.verifyConnectionState(proofHeight, proofTry, counterpartyIdentifier, expected))
abortTransactionUnless(connection.verifyClientConsensusState(
proofHeight, proofConsensus, connection.counterpartyClientIdentifier, consensusHeight, expectedConsensusState))
Expand All @@ -398,7 +403,7 @@ function connOpenConfirm(
connection = provableStore.get(connectionPath(identifier))
abortTransactionUnless(connection.state === TRYOPEN)
expected = ConnectionEnd{OPEN, identifier, getCommitmentPrefix(), connection.counterpartyClientIdentifier,
connection.clientIdentifier, connection.version}
connection.clientIdentifier, connection.version, connection.delayPeriod}
abortTransactionUnless(connection.verifyConnectionState(proofHeight, proofAck, connection.counterpartyConnectionIdentifier, expected))
connection.state = OPEN
provableStore.set(connectionPath(identifier), connection)
Expand Down
23 changes: 22 additions & 1 deletion spec/ics-007-tendermint-client/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,9 @@ function checkValidityAndUpdateState(
// update latest timestamp
clientState.latestTimestamp = header.timestamp
// create recorded consensus state, save it
consensusState = ConsensusState{header.validatorSet, header.commitmentRoot, header.timestamp}
consensusState = ConsensusState{header.timestamp, header.validatorSet, header.commitmentRoot}
set("clients/{identifier}/consensusStates/{header.height}", consensusState)
set("clients/{identifier}/processedTimes/{header.height}", currentTimestamp())
// save the client
set("clients/{identifier}", clientState)
}
Expand Down Expand Up @@ -347,6 +348,7 @@ function verifyChannelState(
function verifyPacketData(
clientState: ClientState,
height: Height,
delayPeriod: uint64,
prefix: CommitmentPrefix,
proof: CommitmentProof,
portIdentifier: Identifier,
Expand All @@ -358,6 +360,10 @@ function verifyPacketData(
assert(clientState.latestHeight >= height)
// check that the client is unfrozen or frozen at a higher height
assert(clientState.frozenHeight === null || clientState.frozenHeight > height)
// fetch the processed time
processedTime = get("clients/{identifier}/processedTimes/{height}")
// assert that enough time has elapsed
assert(currentTimestamp() >= processedTime + delayPeriod)
// fetch the previously verified commitment root & verify membership
root = get("clients/{identifier}/consensusStates/{height}")
// verify that the provided commitment has been stored
Expand All @@ -367,6 +373,7 @@ function verifyPacketData(
function verifyPacketAcknowledgement(
clientState: ClientState,
height: Height,
delayPeriod: uint64,
prefix: CommitmentPrefix,
proof: CommitmentProof,
portIdentifier: Identifier,
Expand All @@ -378,6 +385,10 @@ function verifyPacketAcknowledgement(
assert(clientState.latestHeight >= height)
// check that the client is unfrozen or frozen at a higher height
assert(clientState.frozenHeight === null || clientState.frozenHeight > height)
// fetch the processed time
processedTime = get("clients/{identifier}/processedTimes/{height}")
// assert that enough time has elapsed
assert(currentTimestamp() >= processedTime + delayPeriod)
// fetch the previously verified commitment root & verify membership
root = get("clients/{identifier}/consensusStates/{height}")
// verify that the provided acknowledgement has been stored
Expand All @@ -387,6 +398,7 @@ function verifyPacketAcknowledgement(
function verifyPacketReceiptAbsence(
clientState: ClientState,
height: Height,
delayPeriod: uint64,
prefix: CommitmentPrefix,
proof: CommitmentProof,
portIdentifier: Identifier,
Expand All @@ -397,6 +409,10 @@ function verifyPacketReceiptAbsence(
assert(clientState.latestHeight >= height)
// check that the client is unfrozen or frozen at a higher height
assert(clientState.frozenHeight === null || clientState.frozenHeight > height)
// fetch the processed time
processedTime = get("clients/{identifier}/processedTimes/{height}")
// assert that enough time has elapsed
assert(currentTimestamp() >= processedTime + delayPeriod)
// fetch the previously verified commitment root & verify membership
root = get("clients/{identifier}/consensusStates/{height}")
// verify that no acknowledgement has been stored
Expand All @@ -406,6 +422,7 @@ function verifyPacketReceiptAbsence(
function verifyNextSequenceRecv(
clientState: ClientState,
height: Height,
delayPeriod: uint64,
prefix: CommitmentPrefix,
proof: CommitmentProof,
portIdentifier: Identifier,
Expand All @@ -416,6 +433,10 @@ function verifyNextSequenceRecv(
assert(clientState.latestHeight >= height)
// check that the client is unfrozen or frozen at a higher height
assert(clientState.frozenHeight === null || clientState.frozenHeight > height)
// fetch the processed time
processedTime = get("clients/{identifier}/processedTimes/{height}")
// assert that enough time has elapsed
assert(currentTimestamp() >= processedTime + delayPeriod)
// fetch the previously verified commitment root & verify membership
root = get("clients/{identifier}/consensusStates/{height}")
// verify that the nextSequenceRecv is as claimed
Expand Down