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

Erasure cleanup #545

Merged
merged 8 commits into from
Sep 25, 2023
Merged

Erasure cleanup #545

merged 8 commits into from
Sep 25, 2023

Conversation

dryajov
Copy link
Contributor

@dryajov dryajov commented Sep 6, 2023

No description provided.

@dryajov
Copy link
Contributor Author

dryajov commented Sep 14, 2023

@emizzle @markspanbroek just a heads up, this reworks one of the tests, because right now we don't support encoding a single block - https://github.com/codex-storage/nim-codex/pull/545/files#diff-77966ace2c23c80291b17b166fbe06bdb2c48e01c76335b15e81d49b4e9d4f8eR59-R100. I'll work on a solution, but for now, we should just use some random generated blocks instead.

@markspanbroek
Copy link
Member

this reworks one of the tests, because right now we don't support encoding a single block

This might break tests at other levels as well, e.g. the distributed or continuous tests. If possible, I would suggest to fix single block encoding before merging this.

@dryajov
Copy link
Contributor Author

dryajov commented Sep 14, 2023

this reworks one of the tests, because right now we don't support encoding a single block

This might break tests at other levels as well, e.g. the distributed or continuous tests. If possible, I would suggest to fix single block encoding before merging this.

We should for now not test with one block, which are really a special case.

Our encoding is block level, which means that a single block is one buffer to be encoded, this is to say that the encoded block is just one symbol lined up in a row, and there is just nothing to encoded it with. This is very convenient in the case of two or more blocks, where we stack the two blocks together and encode the two buffers in a row, which makes two symbols encoded into <=2 symbols.

In order to support 1 block encoding, we probably need to add a dummy block, otherwise the structure of the encoding becomes too complicated and it would be hard to make it match the dispersal and the nodes/slots.

Overall, it's not a bug, this is more of a corner case that we should eventually support but it is not critical for the overall system to work.

@dryajov
Copy link
Contributor Author

dryajov commented Sep 14, 2023

Just to clarify, I do plan to spend time on this in the coming weeks, but for now this is an improvement on the existing code and we now also understand why the previous test was failing. So, in terms of this PR, it's simply a cleanup and addressing the current limitation in the tests with the appropriate comments and an accompanying issue #551.

Copy link
Contributor

@emizzle emizzle left a comment

Choose a reason for hiding this comment

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

Looks good, but as per my comment in the code, could we increase the dataset size to avoid the one Block edge case instead of removing the integration test altogether?

Comment on lines +84 to +96
# TODO: We currently do not support encoding single chunks
# test "node retrieves purchase status with 1 chunk":
# let expiry = (await provider.currentTime()) + 30
# let cid = client1.upload("some file contents").get
# let id = client1.requestStorage(cid, duration=1.u256, reward=2.u256, proofProbability=3.u256, expiry=expiry, collateral=200.u256, nodes=2, tolerance=1).get
# let request = client1.getPurchase(id).get.request.get
# check request.ask.duration == 1.u256
# check request.ask.reward == 2.u256
# check request.ask.proofProbability == 3.u256
# check request.expiry == expiry
# check request.ask.collateral == 200.u256
# check request.ask.slots == 3'u64
# check request.ask.maxSlotLoss == 1'u64
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of commenting out this test, could we increase the dataset size instead so that there's more than one Block being encoded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats what I did, look above the commented out test, it's the same but with several blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, I left the name of the original test as is and left this one as the edge case we'd want to enable after adding support for it.

codex/stores/repostore.nim Outdated Show resolved Hide resolved
Co-authored-by: Eric <5089238+emizzle@users.noreply.github.com>
Signed-off-by: Dmitriy Ryajov <dryajov@gmail.com>
@dryajov dryajov marked this pull request as ready for review September 20, 2023 23:30
@dryajov dryajov merged commit 25ea7fd into master Sep 25, 2023
8 checks passed
@dryajov dryajov deleted the erasure-cleanup branch September 25, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants