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

Add del_state support #640

Merged
merged 7 commits into from
Jan 12, 2022
Merged

Conversation

mbrandenburger
Copy link
Contributor

This PR adds del_state to the FPC shim.

Signed-off-by: Marcus Brandenburger [email protected]

Signed-off-by: Marcus Brandenburger <[email protected]>
@mbrandenburger mbrandenburger added the comp/shim This issue is related to the FPC shim exposed to FPC Chaincode label Dec 7, 2021
Signed-off-by: Marcus Brandenburger <[email protected]>
Signed-off-by: Marcus Brandenburger <[email protected]>
Signed-off-by: Marcus Brandenburger <[email protected]>
Signed-off-by: Marcus Brandenburger <[email protected]>
@mbrandenburger mbrandenburger marked this pull request as ready for review December 7, 2021 18:12
@mbrandenburger mbrandenburger requested a review from a team as a code owner December 7, 2021 18:12
@mbrandenburger
Copy link
Contributor Author

DO NOT merge until issue in #641 is discovered

@mbrandenburger mbrandenburger marked this pull request as draft December 8, 2021 09:52
@mbrandenburger mbrandenburger marked this pull request as ready for review December 9, 2021 22:32
@mbrandenburger mbrandenburger marked this pull request as draft December 10, 2021 16:16
@mbrandenburger
Copy link
Contributor Author

Need to check what is the problem here. Will continue after holidays :/

Signed-off-by: Marcus Brandenburger <[email protected]>
Expect(err).ShouldNot(HaveOccurred())
Expect(res).Should(Equal([]byte(value)))

_, err = echoContract.EvaluateTransaction("del_state", key)
res, err = contract.SubmitTransaction("del_state", key)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes the failing integration test.

@mbrandenburger mbrandenburger marked this pull request as ready for review January 3, 2022 11:41
ctx->write_set.erase(key);
ctx->del_set.insert(key);

ocall_del_state(key, ctx->u_shim_ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

(this is unrelated to the PR)

Why do we do this ocall? -- we do the same for the put_state.

The ocall for reading is necessary to grab the data. However, the ocalls for putting and deleting keys essentially update the transaction's rwset in Fabric, which we are not using (if I recall correctly). In fact, FPC maintains its own rwset, which is validated and replayed in the second step (of the two-step transaction commit procedure).

There may still be one reason for these ocall: checking for errors, and returning any error to the enclave.
The big problem is that any feedback/error would not be trustworthy. This is likely a showstopper for such error handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very good observation. We should look into that in ideally remove the ocall for put_state and del_state if really not needed. This may positively chaincode execution performance.

Copy link
Contributor

@bvavala bvavala 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 - not tested locally

@mbrandenburger mbrandenburger merged commit 36a60d3 into hyperledger:main Jan 12, 2022
@mbrandenburger mbrandenburger deleted the del_state branch January 12, 2022 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp/shim This issue is related to the FPC shim exposed to FPC Chaincode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants