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

Make some DI responses synchronous #1920

Merged
merged 19 commits into from
Jul 25, 2023
Merged

Make some DI responses synchronous #1920

merged 19 commits into from
Jul 25, 2023

Conversation

Kailai-Wang
Copy link
Collaborator

@Kailai-Wang Kailai-Wang commented Jul 21, 2023

Context

Partially implemented #1883, it's divided into multiple PRs as otherwise the PR would be too big

solves #1883

This PR:

  • laid down skeleton to make response synchronous
  • apply parentchain effects for failed STFs too
  • store the RPC responses to top pool
  • embed top_hash into TrustedOperationStatus
  • devise a way to deliver synchronous responses for streamed trustedCalls (e.g. link_identity)
  • refactor some of the trusted calls - they should return Err so that the synchronous DI response shows Invalid
  • responses to these calls are synchronous now:
    • set_user_shielding_key
    • activate_identity
    • deactivate_identity
    • set_identity_networks
    • link_identity

The parachain callbacks are preserved to make CI backwards compatible. If you send one of the DI requests above, you should see both the rpc response AND the parachain event.

Now the WorkerRpcReturnValue would have the following structure, example of set_user_shielding_key:

setUserShieldingKey call returned {
  value: '0x8eaf04151687736326c9fea17e25fc5287613693c912909cb226aa4794f26a48fcc5ab7556950296f4f3042fd955b1a281f7683ad07cf54819ab4d4921c636f8c61a250d01bc98d1b3dd8a4e49bfaa1f17fe9d0a086e950a40c58a6a20f3152f00d0964c0fc62a4b7640706f048bda3725e56e2882631916e924d9d8133e2d628a8317593fe5a245dcf7f969e0',
  do_watch: false,
  status: {
    TrustedOperationStatus: [
      [Object],
      '0x0c2612dc5372b7ba83cbc33b1f2023cb287a7c6546ed79b53921b04dc109ca7e'
    ]
  }
}

value would contain the encoded *Response struct, please refer to the test code in DI-sample.

TODO (see comment in trusted_call.rs)

@Kailai-Wang Kailai-Wang added the C0-breaking Breaking change that will cause existing client to break label Jul 21, 2023
@Kailai-Wang Kailai-Wang self-assigned this Jul 21, 2023
@Kailai-Wang Kailai-Wang requested a review from a team July 21, 2023 17:19
@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2023

Codecov Report

Merging #1920 (073abef) into dev (5a493f9) will decrease coverage by 0.16%.
The diff coverage is 0.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##              dev    #1920      +/-   ##
==========================================
- Coverage   17.67%   17.51%   -0.16%     
==========================================
  Files         272      272              
  Lines       12936    13054     +118     
==========================================
  Hits         2287     2287              
- Misses      10649    10767     +118     
Impacted Files Coverage Δ
tee-worker/app-libs/stf/src/stf_sgx.rs 0.00% <0.00%> (ø)
tee-worker/app-libs/stf/src/trusted_call.rs 0.00% <0.00%> (ø)
...orker/core-primitives/stf-executor/src/executor.rs 0.00% <0.00%> (ø)
tee-worker/core-primitives/stf-executor/src/lib.rs 0.00% <0.00%> (ø)
...e-worker/core-primitives/stf-executor/src/mocks.rs 0.00% <0.00%> (ø)
...-worker/core-primitives/stf-interface/src/mocks.rs 0.00% <ø> (ø)
...rker/core-primitives/top-pool-author/src/author.rs 0.00% <0.00%> (ø)
...orker/core-primitives/top-pool-author/src/mocks.rs 0.00% <ø> (ø)
...-worker/core-primitives/top-pool/src/basic_pool.rs 0.00% <0.00%> (ø)
...ee-worker/core-primitives/top-pool/src/listener.rs 0.00% <0.00%> (ø)
... and 17 more

@Kailai-Wang Kailai-Wang marked this pull request as draft July 22, 2023 16:10
@Kailai-Wang Kailai-Wang changed the title Make some DI responses synchronous | Part 1 Make some DI responses synchronous Jul 22, 2023
@Kailai-Wang
Copy link
Collaborator Author

Hold on - maybe I can finish the streamed trustedCall part

@Kailai-Wang Kailai-Wang linked an issue Jul 23, 2023 that may be closed by this pull request
5 tasks
@Kailai-Wang Kailai-Wang marked this pull request as ready for review July 23, 2023 05:39
@Kailai-Wang Kailai-Wang requested a review from a team July 24, 2023 12:06
Copy link
Member

@kziemianek kziemianek left a comment

Choose a reason for hiding this comment

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

I noticed several warnings in tee-worker logs while running test-identity:local ts-tests aganist this implementation

[2023-07-24T21:07:06Z INFO  itc_direct_rpc_server::rpc_responder] set response value
[2023-07-24T21:07:06Z WARN  itp_top_pool::watcher] failed to set response value to rpc client: InvalidConnectionHash

It looks like it doesn't make sense to invoke set_rpc_response_value for TOPs that originates from indirect calls.

@Kailai-Wang
Copy link
Collaborator Author

I noticed several warnings in tee-worker logs while running test-identity:local ts-tests aganist this implementation

[2023-07-24T21:07:06Z INFO  itc_direct_rpc_server::rpc_responder] set response value
[2023-07-24T21:07:06Z WARN  itp_top_pool::watcher] failed to set response value to rpc client: InvalidConnectionHash

It looks like it doesn't make sense to invoke set_rpc_response_value for TOPs that originates from indirect calls.

I believe you'll see such warnings even for DI requests due to potential early hash swap. The swap is called in another thread, and there's no guarantee that the previous top is already imported in the sidechain block at that time. In fact, for most of the time it's not yet included in the block.

Therefore later when the previous top does get included in the block, the top pool watcher tries to send out the response and finds out the channel doesn't exist any more, hence the warning. It's expected behavior and should be fine in such case.

Copy link
Member

@kziemianek kziemianek left a comment

Choose a reason for hiding this comment

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

By making these changes and using rpc return values only for indirect calls I managed to get rid of all [WARN itp_top_pool::watcher] failed to set response value to rpc client: InvalidConnectionHash log entries.

@kziemianek kziemianek self-requested a review July 25, 2023 04:46
Copy link
Member

@kziemianek kziemianek 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 👍🏼

@Kailai-Wang Kailai-Wang merged commit a85f627 into dev Jul 25, 2023
@Kailai-Wang Kailai-Wang deleted the 1883-sync-di-response branch July 25, 2023 07:56
@linear linear bot mentioned this pull request Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C0-breaking Breaking change that will cause existing client to break
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Making some DI responses synchronous
4 participants