feat!: correctly creating the data commitments when the window is changed #1695

rach-id · 2023-05-04T08:22:49Z

Overview

This PR allows creating the data commitments correctly when the data commitments window changes. This change can be via an upgrade or a gov proposal.

Closes #1685

…nged

codecov-commenter · 2023-05-04T08:35:23Z

Codecov Report

Merging #1695 (8e57c11) into main (82b22d4) will decrease coverage by 0.14%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1695      +/-   ##
==========================================
- Coverage   52.82%   52.68%   -0.14%     
==========================================
  Files          97       97              
  Lines        6256     6265       +9     
==========================================
- Hits         3305     3301       -4     
- Misses       2618     2631      +13     
  Partials      333      333

Impacted Files	Coverage Δ
x/qgb/keeper/keeper_data_commitment.go	`53.16% <0.00%> (-12.55%)`	⬇️

x/qgb/abci_test.go

cmwaters

I think creating a data commitment should not be predicated on:

if ctx.BlockHeight() != 0 && ctx.BlockHeight()%int64(k.GetDataCommitmentWindowParam(ctx)) == 0 {

but

if ctx.BlockHeight() != 0 && ctx.BlockHeight()-k.GetLastDataCommitment(ctx).EndBlock >= int64(k.GetDataCommitmentWindowParam(ctx)) {

rach-id · 2023-05-04T14:56:39Z

In case of a window change, for example from 400 to 199 at block 900,

if ctx.BlockHeight() != 0 && ctx.BlockHeight()%int64(k.GetDataCommitmentWindowParam(ctx)) == 0 {

Would give us ranges like: [1, 400], [401, 800], [801, 995], [996, 1194]. Which gives us an intermediate data commitment that has a custom range.

if ctx.BlockHeight() != 0 && ctx.BlockHeight()-k.GetLastDataCommitment(ctx).EndBlock >= int64(k.GetDataCommitmentWindowParam(ctx)) {

Would give us: [1, 400], [401, 800], [801, 999], [1000, 1199], which wouldn't have the intermediate data commitment that has a custom range.

So that depends on whether we want to have an intermediate data commitment range or not

Edit: The second way will not give us an intermediate data commitment range if the upgrade/gov proposal was done in the range [lastDataCommitment.EndBlock, lastDataCommitment.EndBlock + newWindow]. If not, it will behave similar to the first way and have an intermediate data commitment that has a custom range

x/qgb/abci_test.go

rootulp · 2023-05-04T15:41:47Z

x/qgb/keeper/keeper_data_commitment.go

@@ -11,16 +11,29 @@ import (
 // TODO add unit tests for all the keepers

 // GetCurrentDataCommitment creates the latest data commitment at current height according to
-// the data commitment window specified
+// the data commitment window specified.


[nit] the data commitment window isn't specified as a parameter to this function so I wonder if it needs to be in the godoc.

It is defined in the params store... do you think we should reword it?

IMO describing that this function depends on the data commitment window is an implementation detail that doesn't need to be documented. Interested readers can read the function body. If we avoid documenting it then we don't need to update the godoc when implementation details change. So my proposal would be:

// GetCurrentDataCommitment creates the latest data commitment at the current height.

Although that makes me think that more than one data commitment can exist for a given height. Why is "latest" necessary?

cool, will remove the comment.

Although that makes me think that more than one data commitment can exist for a given height. Why is "latest" necessary?

Only one data commitment can exist for height. Also, latest means in here the last one that will be added

will be added to what?

to the state, this method will create a new data commitment and add it to the state, making it the latest data commitment

[non-blocking] here's what I would expect the godoc to look like strictly based on the function name:

// GetCurrentDataCommitment returns a data commitment for the current height. func (k Keeper) GetCurrentDataCommitment(ctx sdk.Context) (types.DataCommitment, error) {

In the function body, I don't see where it adds a data commitment to the state. If it does indeed update state, would a more descriptive function be:

// UpdateLatestDataCommitment creates a new data commitment for the current // height, updates it in state, and returns it. func (k Keeper) UpdateLatestDataCommitment(ctx sdk.Context) (types.DataCommitment, error) {

x/qgb/abci_test.go

Co-authored-by: Rootul P <rootulp@gmail.com>

evan-forbes

question: what happens if the data commitment gets changed to something so small that multiple data commitments are needed? for example, what if we go from 400 -> 5? 400-> 1?

mostly just doc stuff otherwise LGTM

x/qgb/keeper/keeper_data_commitment.go

evan-forbes · 2023-05-04T19:38:17Z

x/qgb/abci_test.go

+	ctx = ctx.WithBlockHeight(wantedHeight)
+	dc3, err := qk.GetCurrentDataCommitment(ctx)
+	require.Nil(t, err)
+	require.Equal(t, dc2.EndBlock+1, dc3.BeginBlock)
+	require.Equal(t, dc2.EndBlock+newShrinkedDCWindow, dc3.EndBlock)
+	require.Equal(t, uint64(3), dc3.Nonce)
+	// set the third data commitment
+	err = qk.SetAttestationRequest(ctx, &dc3)


as mentioned in a different reviewer's comment, parsing this is difficult because its difficult to understand what criteria is being checked. so we could perhaps add a few comments to elaborate on what we are looking for and why.

What do you think about this explanation? 90593fd.
If not clear enough, I will inline comments with every line of the test.
Let me know what you think is better.

rach-id · 2023-05-04T20:20:31Z

what happens if the data commitment gets changed to something so small that multiple data commitments are needed? for example, what if we go from 400 -> 5? 400-> 1?

We will have an intermediary data commitment that will contain all those data commitments, then continue with the provided range. This comment has some details about it: #1695 (comment)

So, for example if we go from 400 to 2 (1 shouldn't be possible, Also, we're setting the lower bound of windows to 101, so we shouldn't get into that case), and we activate the change at height 1000, we will have the following ranges: [1, 400], [401, 800], [801, 1000], [1001, 1002], [1003, 1004]...

evan-forbes

not blocking, but left some comments 🙂

rach-id · 2023-05-04T23:07:15Z

Thanks 👍 I will try to refactor that test tomorrow and see.

Also, your opinion on this is valuable: #1695 (comment)

Which do you think is better?

rootulp

Approving to unblock but TBH I still don't understand the ranges or how this fixes

rootulp · 2023-05-05T19:18:48Z

x/qgb/keeper/keeper_data_commitment.go

@@ -11,16 +11,29 @@ import (
 // TODO add unit tests for all the keepers

 // GetCurrentDataCommitment creates the latest data commitment at current height according to
-// the data commitment window specified
+// the data commitment window specified.


[non-blocking] here's what I would expect the godoc to look like strictly based on the function name:

// GetCurrentDataCommitment returns a data commitment for the current height. func (k Keeper) GetCurrentDataCommitment(ctx sdk.Context) (types.DataCommitment, error) {

In the function body, I don't see where it adds a data commitment to the state. If it does indeed update state, would a more descriptive function be:

// UpdateLatestDataCommitment creates a new data commitment for the current // height, updates it in state, and returns it. func (k Keeper) UpdateLatestDataCommitment(ctx sdk.Context) (types.DataCommitment, error) {

rach-id · 2023-05-05T20:28:28Z

@rootulp The data commitment is set here:

https://github.com/celestiaorg/celestia-app/blob/main/x/qgb/abci.go#L28

Also, this shows I will need to write more documentation around that. Thanks for letting me know.

rootulp · 2023-05-05T21:23:42Z

Based on https://github.com/celestiaorg/celestia-app/blob/main/x/qgb/abci.go#L28 the function GetCurrentDataCommitment doesn't update the data commitment in state so we can use this option instead of the other option:

// GetCurrentDataCommitment returns a data commitment for the current height.
func (k Keeper) GetCurrentDataCommitment(ctx sdk.Context) (types.DataCommitment, error) {

rach-id · 2023-05-05T23:17:33Z

Based on https://github.com/celestiaorg/celestia-app/blob/main/x/qgb/abci.go#L28 the function GetCurrentDataCommitment doesn't update the data commitment in state so we can use this option instead of the other option:
// GetCurrentDataCommitment returns a data commitment for the current height.
func (k Keeper) GetCurrentDataCommitment(ctx sdk.Context) (types.DataCommitment, error) {

Cool, opened issue and will update in a subsequent PR #1710, thanks

rach-id · 2023-05-05T23:29:18Z

Merging, will address the documentation changes in subsequent PRs. Thanks a lot everyone for the thorough reviews 👍 👍

cmwaters · 2023-05-08T10:22:49Z

So that depends on whether we want to have an intermediate data commitment range or not

Yeah the reasoning behind why I mentioned that was to specifically remove this intermediate commitment range. There should just be the old one and the new one.

Edit: The second way will not give us an intermediate data commitment range if the upgrade/gov proposal was done in the range [lastDataCommitment.EndBlock, lastDataCommitment.EndBlock + newWindow]. If not, it will behave similar to the first way and have an intermediate data commitment that has a custom range

I'm a bit confused by this, I thought this would remove the possibility of an intermediate data commitment range in all cases

rach-id · 2023-05-08T10:44:45Z

I'm a bit confused by this, I thought this would remove the possibility of an intermediate data commitment range in all cases

Let's take for example an initial data commitment window of 500.
Then, the first ranges, up to height 1000, will be:

[1, 500]
[501, 1000]

Then, at block 1300, we make a gov proposal to change the window down to 100. Then, the following condition:

if ctx.BlockHeight() != 0 && ctx.BlockHeight()-k.GetLastDataCommitment(ctx).EndBlock >= int64(k.GetDataCommitmentWindowParam(ctx)) {

will be true at height 1300, and we will create a new data commitment as follows:

celestia-app/x/qgb/keeper/keeper_data_commitment.go

Lines 31 to 36 in c08d1c6

    
           lastDCC, err := k.GetLastDataCommitment(ctx) 
        
           if err != nil { 
        
           	return types.DataCommitment{}, err 
        
           } 
        
           beginBlock = lastDCC.EndBlock + 1 
        
           endBlock = lastDCC.EndBlock + dcWindow

Which will be: [1001, 1300].

After that, we will have the following ranges: [1301, 1400], [1401, 1500]...

Thus, we will still have the intermediate range of [1001, 1300]. Unless I am missing your point.

If we don't want to have intermediate ranges, one way is to create multiple attestations at the same height. In the above case, at height 1300, we create the following attestations: [1001, 1100], [1101, 1200], [1201, 1300].

cmwaters · 2023-05-08T12:51:02Z

Which will be: [1001, 1300].

Won't it be [1001, 1100] if the dcWindow is 100?

rach-id · 2023-05-08T13:02:00Z

yes, that if the window change happens in height in the following [1001, 1100]. However, if it happens later, it will be the above

cmwaters · 2023-05-08T14:28:12Z

Hmm the way I understand the code here, if the window was previously 500, the last commitment at height 1000, if we change the window to 100 at height 1300, that will trigger this condition immediately and it will create a data commitment from [1001, 1100] (as it's based from the last commitment and the window) and then the very next height it will trigger again and create the [1101, 1200] and the same for the next height until height 1303 where the last commitment ends at 1300 and the condition no longer triggers (we could also trigger multiple commitments to be generated in the same block).

Am I missing something?

rach-id · 2023-05-08T17:40:06Z

This assumes that end blocker will run multiple times until we have all the attestations created. However, it's only ran once.

This is what I meant by adding the possibility to creating multiple attestations per height if needed and we won't have the intermediate range. But that would require changing the implementation of end blocker.

rach-id · 2023-05-08T17:41:29Z

I will create an issue to investigate having this. However, I still don't why having an intermediate range is bad. Please let me know if there is something I'm missing

evan-forbes · 2023-05-08T20:57:47Z

yes, that if the window change happens in height in the following [1001, 1100]. However, if it happens later, it will be the above

my understanding of the code was what @cmwaters said, and that's what appears to happen in the test if we modify it to the above scenario.

this is mainly why I asked this question, because it looked like we could only create a single commitment per block, and we would only set a data commitment if the block height is a multiple of the window. Meaning that we might be in a scenario where it takes a long time for the data commitment to catch up to the block height.

celestia-app/x/qgb/abci.go

Line 23 in e223493

    
           if ctx.BlockHeight() != 0 && ctx.BlockHeight()%int64(k.GetDataCommitmentWindowParam(ctx)) == 0 {

This was also why I thought #1713 solved, was by setting to the min to 100, its reasonable that we would always catch up.

so... imo we should keep the code as is here, but then add the ability to create multiple data commitments to cover the edge case.

perhaps we're missing something, and if so, @sweexordious, when you get the chance, do you mind adding a test that demonstrates the intermediate range? thanks! no rush either we have plenty of time 🙂

rach-id · 2023-05-09T08:48:32Z

but then add the ability to create multiple data commitments to cover the edge case.

This is so we don't have the intermediate range, right?

Why is it not good to have an intermediate range that's used to catchup?

Sure, will create a test and let you know

cmwaters · 2023-05-09T10:20:49Z

However, I still don't why having an intermediate range is bad

I don't think it's that bad (I don't have a full view on things), it's just not correct. You've gone from one range to another so those would be the two ranges we expect posted but now there's a third range to "bridge the gap".

rach-id · 2023-05-09T11:10:33Z

Per sync, will create an issue to catchup on the data commitments. Good catch guys 👍 👍

feat!: correctly creating the data commitments when the window is cha…

a6e2884

…nged

rach-id added the x/qgb label May 4, 2023

rach-id requested a review from evan-forbes as a code owner May 4, 2023 08:22

rach-id self-assigned this May 4, 2023

rach-id changed the title ~~feat!: correctly creating the data commitments when the window is cha…~~ feat!: correctly creating the data commitments when the window is changed May 4, 2023

MSevey requested review from a team and rootulp and removed request for a team May 4, 2023 08:23

evan-forbes added this to the Mainnet milestone May 4, 2023

evan-forbes reviewed May 4, 2023

View reviewed changes

x/qgb/abci_test.go Outdated Show resolved Hide resolved

cmwaters reviewed May 4, 2023

View reviewed changes

rootulp reviewed May 4, 2023

View reviewed changes

rach-id and others added 3 commits May 4, 2023 20:11

Update x/qgb/abci_test.go

a4d976c

Co-authored-by: Rootul P <rootulp@gmail.com>

Update x/qgb/abci_test.go

67a51e2

Co-authored-by: Rootul P <rootulp@gmail.com>

Update x/qgb/abci_test.go

81555ac

Co-authored-by: Rootul P <rootulp@gmail.com>

MSevey requested a review from a team May 4, 2023 18:12

rach-id and others added 10 commits May 4, 2023 20:12

Update x/qgb/abci_test.go

54b5ea3

Co-authored-by: Rootul P <rootulp@gmail.com>

Update x/qgb/abci_test.go

c0b5145

Co-authored-by: Rootul P <rootulp@gmail.com>

Update x/qgb/abci_test.go

3b6258c

Co-authored-by: Rootul P <rootulp@gmail.com>

Update x/qgb/abci_test.go

55bb037

Co-authored-by: Rootul P <rootulp@gmail.com>

Update x/qgb/abci_test.go

d249b3e

Co-authored-by: Rootul P <rootulp@gmail.com>

Update x/qgb/abci_test.go

fb7f8e1

Co-authored-by: Rootul P <rootulp@gmail.com>

Update x/qgb/abci_test.go

0277230

Co-authored-by: Rootul P <rootulp@gmail.com>

Update x/qgb/abci_test.go

2f08a4e

Co-authored-by: Rootul P <rootulp@gmail.com>

Update x/qgb/abci_test.go

11c6b89

Co-authored-by: Rootul P <rootulp@gmail.com>

docs: add comment to explain the test

90593fd

evan-forbes reviewed May 4, 2023

View reviewed changes

MSevey requested a review from a team May 4, 2023 21:29

chore: use numbers in test

3bf5bf7

evan-forbes previously approved these changes May 4, 2023

View reviewed changes

test: refactor test

eaf055c

rach-id dismissed evan-forbes’s stale review via eaf055c May 5, 2023 09:26

MSevey requested a review from a team May 5, 2023 09:26

chore: remove unused function

8e57c11

evan-forbes approved these changes May 5, 2023

View reviewed changes

rach-id mentioned this pull request May 5, 2023

Update the QGB minimum data commitment window error message #1706

Closed

rootulp approved these changes May 5, 2023

View reviewed changes

rach-id mentioned this pull request May 5, 2023

Document the data commitment ranges #1708

Closed

rach-id mentioned this pull request May 5, 2023

Update the comment on data commitment creation #1710

Closed

rach-id merged commit 9b9a481 into celestiaorg:main May 5, 2023

rach-id mentioned this pull request May 9, 2023

Data commitment ranges catchup #1719

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: correctly creating the data commitments when the window is changed #1695

feat!: correctly creating the data commitments when the window is changed #1695

rach-id commented May 4, 2023 •

edited

Loading

codecov-commenter commented May 4, 2023 •

edited

Loading

cmwaters left a comment

rach-id commented May 4, 2023 •

edited

Loading

rootulp May 4, 2023

rach-id May 4, 2023

rootulp May 4, 2023

rach-id May 4, 2023

rootulp May 5, 2023

rach-id May 5, 2023

rootulp May 5, 2023

evan-forbes left a comment

evan-forbes May 4, 2023

rach-id May 4, 2023

rach-id commented May 4, 2023

evan-forbes left a comment

rach-id commented May 4, 2023

rootulp left a comment

rootulp May 5, 2023

rach-id commented May 5, 2023

rootulp commented May 5, 2023 •

edited

Loading

rach-id commented May 5, 2023

rach-id commented May 5, 2023

cmwaters commented May 8, 2023

rach-id commented May 8, 2023

cmwaters commented May 8, 2023

rach-id commented May 8, 2023

cmwaters commented May 8, 2023

rach-id commented May 8, 2023

rach-id commented May 8, 2023

evan-forbes commented May 8, 2023 •

edited

Loading

rach-id commented May 9, 2023

cmwaters commented May 9, 2023

rach-id commented May 9, 2023

feat!: correctly creating the data commitments when the window is changed #1695

feat!: correctly creating the data commitments when the window is changed #1695

Conversation

rach-id commented May 4, 2023 • edited Loading

Overview

codecov-commenter commented May 4, 2023 • edited Loading

Codecov Report

cmwaters left a comment

Choose a reason for hiding this comment

rach-id commented May 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rach-id commented May 4, 2023

evan-forbes left a comment

Choose a reason for hiding this comment

rach-id commented May 4, 2023

rootulp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rach-id commented May 5, 2023

rootulp commented May 5, 2023 • edited Loading

rach-id commented May 5, 2023

rach-id commented May 5, 2023

cmwaters commented May 8, 2023

rach-id commented May 8, 2023

cmwaters commented May 8, 2023

rach-id commented May 8, 2023

cmwaters commented May 8, 2023

rach-id commented May 8, 2023

rach-id commented May 8, 2023

evan-forbes commented May 8, 2023 • edited Loading

rach-id commented May 9, 2023

cmwaters commented May 9, 2023

rach-id commented May 9, 2023

rach-id commented May 4, 2023 •

edited

Loading

codecov-commenter commented May 4, 2023 •

edited

Loading

rach-id commented May 4, 2023 •

edited

Loading

rootulp commented May 5, 2023 •

edited

Loading

evan-forbes commented May 8, 2023 •

edited

Loading