fix(testing): update validators before UpdateClient call #4315
Conversation
tbruyelle
requested review from
colin-axner,
AdityaSripal,
crodriguezvega,
charleenfei,
damiannolan and
chatton
as code owners
|
It also seems that the change does not affect any of the tests in the Should we target |
|
Hi, thanks for raising the issue. Can you provide more context on why exactly UpdateClient is failing? From my understanding, the signed header for an executing block does not change its validator set or next validator set after executing (delayed execution). The validator set changes would be applied to the next header to be signed. For context, the testing pkg on main doesn't fully mimic ABCI execution. I've raised a series of issues to fix this, with #3997 probably being most relevant. All the issues are listed on this milestone. If you could comment with any context I am missing, that would be very helpful! |
Yes main, thank you. We cherry pick back to release branches when we backport fixes |
|
Hey @colin-axner, thanks for your response and all the details. It seems like the code in ICS is able to replicate the bug mentioned in #1668 (funny enough the diff is exactly the same as this PR). I'll work to extract the code from ICS and provide a basic way to reproduce. This should help me to provide the missing context. EDIT: didn't find the time to finish that task, and I'm OOO the next 2 weeks, so this will wait a little 🙏 |
|
Looked into this some more by testing with your branch in the linked pr. Still trying to grapple with how to fix this issue, but I think I've narrowed in on a problem. From my understanding of looking at the SDK code and cometbft. The validator set changes returned by the SDK (which have already been applied) are not applied to the validator set which signs h + 1, rather, it is applied as we have it written in the ibc testing package, that is: res := endBlock()
nextHeader.Vals = chain.NextVals
nextHeader.NextVals = applyChanges(chain.NextVals, changes)But there's a subtle issue. Historical info in the SDK sets on begin block the current validator set it tracks. I believe the validators of the header being executed is not necessarily equal to the validator set stored in the SDK. This is because the applied changes in block H won't take affect until block H + 2. I think this is why the proposed fix works, it reverses the logic. It creates a signed header where the next vals is updated earlier (to match SDK logic). Instead, the testing pkg should likely be fixed to query the correct validators. The current issue I think is that we query for the validators stored by historical info at H + 1, but as I noted, the validators stored at H + 1, are the validators of H + 2. I guess this means to get, validators of H + 1, we need to query historical info at H. I didn't have any luck with fixing the tests using this logic, will continue looking later |
|
@colin-axner thanks for the investigation. Let me try to understand the issue you have raised. On my side I naively tried to reproduce the case by adding some packets exchange in |
|
Hi @tbruyelle, I figured it out! So, I was correct in my above logic, the trusted height validator query to historical info should occur at the trusted height (based on the logic described above). The tests were not passing though because there was double trouble. I needed to tweak ccv tests as well. While debugging I noticed that despite my logic, the historical info was returning the next validators for a block ahead. This made no sense to me, because when I queried at Then it dawned on me that it was as if the validator set stored in historical info was the same produced at the end of the block. But how could that happen? I was already aware of the finickiness of BeginBlock being called multiple times. Historical info is written on begin block based on the latest validator set. After a little investigation, my hunch turned out to be correct. BeginBlock was being called, state changes to the validator set occurred and then begin block was being called again! This occurs when we try to update the header time. Here are my changes to ibc-go (which I will open a pr to fix): diff --git a/testing/chain.go b/testing/chain.go
index 4a1a2f5db..cddb026bd 100644
--- a/testing/chain.go
+++ b/testing/chain.go
@@ -413,18 +413,19 @@ func (chain *TestChain) ConstructUpdateTMClientHeaderWithTrustedHeight(counterpa
// Once we get TrustedHeight from client, we must query the validators from the counterparty chain
// If the LatestHeight == LastHeader.Height, then TrustedValidators are current validators
// If LatestHeight < LastHeader.Height, we can query the historical validator set from HistoricalInfo
- if trustedHeight == counterparty.LastHeader.GetHeight() {
+ if trustedHeight.GTE(counterparty.LastHeader.GetHeight()) {
tmTrustedVals = counterparty.Vals
} else {
// NOTE: We need to get validators from counterparty at height: trustedHeight+1
// since the last trusted validators for a header at height h
// is the NextValidators at h+1 committed to in header h by
// NextValidatorsHash
- tmTrustedVals, ok = counterparty.GetValsAtHeight(int64(trustedHeight.RevisionHeight + 1))
+ tmTrustedVals, ok = counterparty.GetValsAtHeight(int64(trustedHeight.RevisionHeight))
if !ok {
return nil, sdkerrors.Wrapf(ibctm.ErrInvalidHeaderHeight, "could not retrieve trusted validators at trustedHeight: %d", trustedHeight)
}
}
+and here are my changes in ccv: diff --git a/tests/integration/unbonding.go b/tests/integration/unbonding.go
index 7693c782..fd323937 100644
--- a/tests/integration/unbonding.go
+++ b/tests/integration/unbonding.go
@@ -370,6 +370,8 @@ func (s *CCVTestSuite) TestRedelegationNoConsumer() {
s.providerCtx().BlockTime().Add(stakingKeeper.UnbondingTime(s.providerCtx())),
)
+ s.providerChain.NextBlock()
+
// Increment time so that the unbonding period passes on the provider
incrementTimeByUnbondingPeriod(s, Provider)
diff --git a/testutil/simibc/chain_util.go b/testutil/simibc/chain_util.go
index 64b0e913..6cc697b0 100644
--- a/testutil/simibc/chain_util.go
+++ b/testutil/simibc/chain_util.go
@@ -48,12 +48,12 @@ func EndBlock(c *ibctesting.TestChain, preCommitCallback func()) (*ibctmtypes.He
c.App.Commit()
+ c.LastHeader = c.CurrentTMClientHeader()
+
c.Vals = c.NextVals
c.NextVals = ibctesting.ApplyValSetChanges(c.T, c.Vals, ebRes.ValidatorUpdates)
- c.LastHeader = c.CurrentTMClientHeader()
-
sdkEvts := ABCIToSDKEvents(ebRes.Events)
packets := ParsePacketsFromEvents(sdkEvts)
diff --git a/testutil/simibc/relay_util.go b/testutil/simibc/relay_util.go
index 6ede5aa0..18f63b34 100644
--- a/testutil/simibc/relay_util.go
+++ b/testutil/simibc/relay_util.go
@@ -151,14 +151,14 @@ func augmentHeader(sender *ibctesting.TestChain, receiver *ibctesting.TestChain,
// Once we get TrustedHeight from client, we must query the validators from the counterparty chain
// If the LatestHeight == LastHeader.Height, then TrustedValidators are current validators
// If LatestHeight < LastHeader.Height, we can query the historical validator set from HistoricalInfo
- if trustedHeight == sender.LastHeader.GetHeight() {
- tmTrustedVals = sender.Vals
+ if trustedHeight.GTE(sender.LastHeader.GetHeight()) {
+ tmTrustedVals = sender.NextVals
} else {
// NOTE: We need to get validators from counterparty at height: trustedHeight+1
// since the last trusted validators for a header at height h
// is the NextValidators at h+1 committed to in header h by
// NextValidatorsHash
- tmTrustedVals, ok = sender.GetValsAtHeight(int64(trustedHeight.RevisionHeight + 1))
+ tmTrustedVals, ok = sender.GetValsAtHeight(int64(trustedHeight.RevisionHeight))
if !ok {
return errorsmod.Wrapf(ibctmtypes.ErrInvalidHeaderHeight, "could not retrieve trusted validators at trustedHeight: %d", trustedHeight)
}I think the |
|
Wow excellent @colin-axner! I was actually debugging The fix in About |
I'm not sure which tests require them. I only made the modification after finding that directory when I used grep to look for begin/end block calls. Maybe that package should be updated then to match the ibc-go testing pkg usage. You would like the fix to be backported to the v7.2.x line? |
Yes please 🙏 |
Yes that's also what I think, I'll keep that in mind! |
|
I can confirm the patch 668fa1d backported to the 7.2 branch in PR #4505 fixes the issue that initially motivated that PR. So I can close this PR which doesn't contain the correct fix for the issue #4484. Thanks again @colin-axner ! cc @MSalopek |
Description
The current version of ICS uses a copy of
ibc-go/testing, located in thelegacy_ibc_testingfolder, because it requires a couple of shims to work properly in its context.There's a pending PR where this folder is removed and where ICS really uses this repo for testing. I was able to workaround most of the shims, except one that in my opinion requires a change in
ibco-go/testing. This change (contained in this PR) switches the order of 2 instructions in theNextBlock()function.Currently, the validators are updated after the
chain.LastHeaderupdate, which, in the context of the ICS tests, makes theUpdateClient()call fail, because the trusted validators hash doesn't match the consensus state next validators hash. The condition that fails is exactly here:ibc-go/modules/light-clients/07-tendermint/update.go
Line 215 in 71c6d71
The patch simply ensures that the validators are updated before the
lastHeaderis updated, which fix the problem for ICS. For this repo, this change doesn't fail the tests, but I lack knowledge on IBC to undertand all the consequences, so please let me know if this is irrelevant. In that case we'll probably have to find an other workaround or to keep thelegacy_ibc_testingfolder in ICS.This change targets the
release/v7.2.xbranch because ICS depends on 7.1.0. Ideally, we expect to have this patch in a 7.2.1 or 7.3.0, please let us know what's possible.Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.