feat!: data commitment ranges catchup #1763
Conversation
rach-id
changed the title
Codecov Report
@@ Coverage Diff @@
## main #1763 +/- ##
==========================================
- Coverage 52.05% 51.86% -0.19%
==========================================
Files 93 93
Lines 6049 6125 +76
==========================================
+ Hits 3149 3177 +28
- Misses 2576 2618 +42
- Partials 324 330 +6
|
| // we should have 19 data commitments created in the above setup | ||
| // - window 400: [1, 400], [401, 800], [801, 1200] | ||
| // - window 100: [1201, 1300], [1301, 1400], [1401, 1500], [1501, 1600], [1601, 1700], [1701, 1800], [1801, 1900] | ||
| // - window 1000: [1901, 2900] | ||
| // - window 111: [2901, 3011], [3012, 3122], [3123,3233], [3234, 3344], [3345, 3455], [3456, 3566], [3567, 3677], [3678, 3788] |
There was a problem hiding this comment.
[optional] this comment is soooo helpful so I converted it into an explicit want. Totally optional:
// TestGetDataCommitment This test will test the data commitment creation catchup mechanism.
// It will run `abci.EndBlocker` on all the heights while changing the data commitment window
// in different occasions, to see if at the end of the test, the data commitments cover all
// the needed ranges.
func TestDataCommitmentCreationCatchup(t *testing.T) {
input, ctx := testutil.SetupFiveValChain(t)
qk := input.QgbKeeper
ctx = ctx.WithBlockHeight(1)
executeHeights := func(beginHeight int64, endHeight int64) {
for i := beginHeight; i <= endHeight; i++ {
ctx = ctx.WithBlockHeight(i)
qgb.EndBlocker(ctx, *qk)
}
}
// from height 1 to 1500 with a window of 400
qk.SetParams(ctx, types.Params{DataCommitmentWindow: 400})
executeHeights(1, 1500)
// change window to 100 and execute up to 1920
qk.SetParams(ctx, types.Params{DataCommitmentWindow: 100})
executeHeights(1501, 1920)
// change window to 1000 and execute up to 3500
qk.SetParams(ctx, types.Params{DataCommitmentWindow: 1000})
executeHeights(1921, 3500)
// change window to 111 and execute up to 3800
qk.SetParams(ctx, types.Params{DataCommitmentWindow: 111})
executeHeights(3501, 3800)
// check if a data commitment was created
hasDataCommitment, err := qk.HasDataCommitmentInStore(ctx)
require.NoError(t, err)
require.True(t, hasDataCommitment)
// get the last attestation nonce
lastAttestationNonce := qk.GetLatestAttestationNonce(ctx)
got := []types.DataCommitment{}
for i := uint64(1); i <= lastAttestationNonce; i++ {
att, found, err := qk.GetAttestationByNonce(ctx, i)
require.NoError(t, err)
require.True(t, found)
dc, ok := att.(*types.DataCommitment)
if !ok {
continue
}
got = append(got, *dc)
}
// We should have 19 data commitments created in the above setup
// - window 400: [1, 400], [401, 800], [801, 1200]
// - window 100: [1201, 1300], [1301, 1400], [1401, 1500], [1501, 1600], [1601, 1700], [1701, 1800], [1801, 1900]
// - window 1000: [1901, 2900]
// - window 111: [2901, 3011], [3012, 3122], [3123,3233], [3234, 3344], [3345, 3455], [3456, 3566], [3567, 3677], [3678, 3788]
want := []types.DataCommitment{
{
Nonce: 2, // nonce 1 is the valset attestation
BeginBlock: 1,
EndBlock: 400,
},
{
Nonce: 3,
BeginBlock: 401,
EndBlock: 800,
},
{
Nonce: 4,
BeginBlock: 801,
EndBlock: 1200,
},
{
Nonce: 5,
BeginBlock: 1201,
EndBlock: 1300,
},
{
Nonce: 6,
BeginBlock: 1301,
EndBlock: 1400,
},
{
Nonce: 7,
BeginBlock: 1401,
EndBlock: 1500,
},
{
Nonce: 8,
BeginBlock: 1501,
EndBlock: 1600,
},
{
Nonce: 9,
BeginBlock: 1601,
EndBlock: 1700,
},
{
Nonce: 10,
BeginBlock: 1701,
EndBlock: 1800,
},
{
Nonce: 11,
BeginBlock: 1801,
EndBlock: 1900,
},
{
Nonce: 12,
BeginBlock: 1901,
EndBlock: 2900,
},
{
Nonce: 13,
BeginBlock: 2901,
EndBlock: 3011,
},
{
Nonce: 14,
BeginBlock: 3012,
EndBlock: 3122,
},
{
Nonce: 15,
BeginBlock: 3123,
EndBlock: 3233,
},
{
Nonce: 16,
BeginBlock: 3234,
EndBlock: 3344,
},
{
Nonce: 17,
BeginBlock: 3345,
EndBlock: 3455,
},
{
Nonce: 18,
BeginBlock: 3456,
EndBlock: 3566,
},
{
Nonce: 19,
BeginBlock: 3567,
EndBlock: 3677,
},
{
Nonce: 20,
BeginBlock: 3678,
EndBlock: 3788,
},
}
assert.Equal(t, 19, len(got))
assert.Equal(t, want, got)
}
Co-authored-by: Rootul P <[email protected]>
Co-authored-by: Rootul P <[email protected]>
| func EndBlocker(ctx sdk.Context, k keeper.Keeper) { | ||
| handleDataCommitmentRequest(ctx, k) | ||
| // we always want to create the valset at first so that if there is a new validator set, then it is | ||
| // the one responsible for signing from now on. | ||
| handleValsetRequest(ctx, k) | ||
| handleDataCommitmentRequest(ctx, k) |
There was a problem hiding this comment.
Can you explain to me a little more the rationale for switching this? Is one dependent on the other?
There was a problem hiding this comment.
This is done mainly in the case of a hardfork where there is a manual change to the validator set. This would ensure that straight after starting the chain again, a new valset is created representing the new validator set, and that one will be used to sign the new data commitments.
Otherwise, if we're in a case where a new data commitment is created straight after the hardfork, we will be in the situation where that data commitment will need to be signed by the previous validator set which can not exist anymore.
So, it's better to always check if we there is a change to the validator set and account for it, before creating any data commitment
There was a problem hiding this comment.
So the validator set of that height signs over the data hashes for the last n heights? (as opposed to the validator set at the previous height)
There was a problem hiding this comment.
yes, in the event of a validator set change, we want to change directly to the new one and use it for signatures
Co-authored-by: Callum Waters <[email protected]>
| // this will keep executing until all the needed data commitments are created and we catchup to the current height | ||
| for { | ||
| hasLastDataCommitment, err := k.HasDataCommitmentInStore(ctx) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| if hasLastDataCommitment { | ||
| // if the store already has a data commitment, we use it to check if we need to create a new data commitment | ||
| lastDataCommitment, err := k.GetLastDataCommitment(ctx) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| if ctx.BlockHeight()-int64(lastDataCommitment.EndBlock) >= int64(k.GetDataCommitmentWindowParam(ctx)) { | ||
| setDataCommitmentAttestation() | ||
| } else { | ||
| // the needed data commitments are already created and we need to wait for the next window to elapse | ||
| break | ||
| } | ||
| } else { | ||
| // if the store doesn't have a data commitment, we check if the window has passed to create a new data commitment | ||
| if ctx.BlockHeight() >= int64(k.GetDataCommitmentWindowParam(ctx)) { | ||
| setDataCommitmentAttestation() | ||
| } else { | ||
| // the first data commitment window hasn't elapsed yet to create a commitment | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
nice thanks for adding the purpose of the loop at the top and the comments at the break points! we might also want to add some docs at the top for the two conditions in which this loop breaks,
afaiu:
-
If
hasLastDataCommitment == true, and blocks last data commitment is < the data commitment window. -
If
hasLastDataCommitment == false, and height < the data commitment window.
The first condition is often the one that gets hit, so its pretty easy to find in the tests, but do you mind adding a quick comment on line 371 where we're testing the loop functionality? Also I think I missed it but are we testing the second exit condition?
I think there's a very minor gas optimization where we could load the data commitment window once instead of reading from state each loop. I know we're not the ones paying gas since its in endblock, and that shouldn't be breaking, so tbh its probably not even worth changing until we actually test the diff.
There was a problem hiding this comment.
but do you mind adding a quick comment on line 371 where we're testing the loop functionality?
I guess the line number is incorrect
Also I think I missed it but are we testing the second exit condition?
I think it is tested implicitly by specifying the nonce. This means that no data commitment was created in that range
I think there's a very minor gas optimization where we could load the data commitment window once instead of reading from state each loop. I know we're not the ones paying gas since its in endblock, and that shouldn't be breaking, so tbh its probably not even worth changing until we actually test the diff.
I will create a separate issue for this. But, who pays for end blocker? There are no transactions for the QGB module... So you mean the validators will pay for it?
There was a problem hiding this comment.
But, who pays for end blocker?
no sorry, I meant that since it's requried to come to consensus and its not a tx with fees, no one is paying. Technically tho, we are reading from the state fewer times, so we're saving "gas" or simply node/network resources 🙂
I guess the line number is incorrect
do you mind pointing our where in the test we are hitting that looping functionlity then? that still looks that the point in the test afaict 🙂
There was a problem hiding this comment.
so we're saving "gas" or simply node/network resources slightly_smiling_face
I think that that simple statements like those will not affect it much. But yes, good point, will open an issue to optimize that.
For the test thing, the catchup should start here, but good point we're not testing all the possibilities. I will open an issue to do that.
I don't want to commit anymore to this PR not to wait for another round of reviews.
Overview
Closes #1719