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.

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

refactor(share): GetShare -> GetSamples #3905

Draft
wants to merge 8 commits into
base: get-samples
Choose a base branch
from

Conversation

cristaloleg
Copy link
Contributor

Continuation for #3891

@cristaloleg cristaloleg added the kind:refactor Attached to refactoring PRs label Oct 30, 2024
@cristaloleg cristaloleg self-assigned this Oct 30, 2024
@@ -904,7 +915,7 @@ func createService(ctx context.Context, t testing.TB, shares []libshare.Share) *
})
shareGetter.EXPECT().GetSamples(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().
DoAndReturn(func(ctx context.Context, h *header.ExtendedHeader, indices []shwap.SampleIndex) ([]shwap.Sample, error) {
return smpls, nil
return []shwap.Sample{}, nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea what should be here.

require.NoError(t, err)

assert.Equal(t, sh, resultShares[shareOffset])
assert.Equal(t, smpls[0].Share, resultShares[shareOffset])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it be always [0] or somethings else? idx ?

smpls, err := service.shareGetter.GetSamples(ctx, h, []shwap.SampleIndex{idx})
require.NoError(t, err)

assert.Equal(t, smpls[0].Share, resultShares[shareOffset], fmt.Sprintf("issue on %d attempt", i))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it be always [0] or somethings else? idx ?

require.NoError(t, err)

assert.Equal(t, sh, resultShares[0])
assert.Equal(t, smpls[0].Share, resultShares[0])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it be always [0] or somethings else? idx ?

require.True(t, bytes.Equal(sh.ToBytes(), resultShares[shareOffset].ToBytes()),
smpls, err := service.shareGetter.GetSamples(ctx, h, []shwap.SampleIndex{idx})
require.NoError(t, err)
require.True(t, bytes.Equal(smpls[0].Share.ToBytes(), resultShares[shareOffset].ToBytes()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it be always [0] or somethings else? idx ?

share/eds/accessor.go Outdated Show resolved Hide resolved
@@ -86,7 +86,7 @@ func (sb *SampleBlock) Marshal() ([]byte, error) {
}

func (sb *SampleBlock) Populate(ctx context.Context, eds eds.Accessor) error {
smpl, err := eds.Sample(ctx, sb.ID.RowIndex, sb.ID.ShareIndex)
smpl, err := eds.Sample(ctx, shwap.SampleIndex(sb.ID.ShareIndex))
Copy link
Member

Choose a reason for hiding this comment

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

ShareIndex is only the column

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

The data structure we work with is a table of Rows and Columns. Cells in the table are called shares. Every share can be addressed by coordinates or index. Coordinate in 2D space and index in 1D vector space.

In this case ShareIndex is ColumnIndex(I know it might be confusing in this context), so
(RowIndex; ShareIndex) is a coordinate.

SampleIndex is an index in 1D space, so we cant use ShareIndex ad SampleIndex.

P.S. Sample is Share + Proof, so in the context of positiom Share and Sample can be used interchangeably.

rowIdx, colIdx, err := idx.Coordinates(shwap.EdsIDSize)
if err != nil {
return shwap.Sample{}, err
}
return eds.SampleForProofAxis(rowIdx, colIdx, rsmt2d.Row)
Copy link
Member

Choose a reason for hiding this comment

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

We should use the SampleIndex here as well. The objective is to use a single type everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, SampleForProofAxis is now:

func (eds *Rsmt2D) SampleForProofAxis(
	idx shwap.SampleIndex,
	proofType rsmt2d.Axis,
) (shwap.Sample, error) {

Comment on lines +239 to +242
axisType, axisIdx := rsmt2d.Row, 0 // rowIdx, colIdx
// if colIdx < o.size()/2 && rowIdx >= o.size()/2 {
// axisType, axisIdx, shrIdx = rsmt2d.Col, colIdx, rowIdx
// }
Copy link
Member

Choose a reason for hiding this comment

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

we still need to swap coordinates here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we don't need rowIdx, colIdx (idx is enough here). The only question I have is: how to get from SampleIndex which axis do we want?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:refactor Attached to refactoring PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants