Historical queries: Handle large requests being truncated by the host #5026
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…sking for something
|
ledger_fetch_completeness@65363 aka 20230220.58 vs main ewma over 20 builds from 64799 to 65360 Click to see tablemain
ledger_fetch_completeness
|
eddyashton
commented
Feb 20, 2023
eddyashton
commented
Feb 20, 2023
eddyashton
commented
Feb 20, 2023
jumaffre
reviewed
Feb 20, 2023
jumaffre
approved these changes
Feb 20, 2023
achamayou
approved these changes
Feb 20, 2023
achamayou
added
2.x-todo
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
eddyashton
added a commit
to eddyashton/CCF
that referenced
this pull request
Feb 21, 2023
…microsoft#5026) (cherry picked from commit 042cb8f)
eddyashton
added
the
backported
eddyashton
added a commit
to eddyashton/CCF
that referenced
this pull request
Feb 21, 2023
…microsoft#5026) (cherry picked from commit 042cb8f) # Conflicts: # src/node/historical_queries.h
This was referenced Feb 21, 2023
achamayou
pushed a commit
that referenced
this pull request
Feb 27, 2023
achamayou
pushed a commit
that referenced
this pull request
Feb 27, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a bug in the historical query system.
It assumed that any
ledger_get_range(from, to)write to the host ringbuffer would result in a correspondingledger_entry_range(from, to)orledger_no_entry_range(from, to)response from the (honest) host. This is not the case - the host applies amax_sizecap since #3986 to avoid producing writes larger than the ringbuffer can handle, so it may in fact respond withledger_entry_range(from, to_)(withto_<to). This PR makes the in-enclave historical query system robust to that - able to handle partial answers, and re-request entries which weren't provided.This requires a significant rewrite of the historical query book keeping, to decouple slightly the
Stores from the specificRequestwhich triggered their fetch (with a side benefit that they are now better at sharingStores between multipleRequests, without needing to refetch).There's some knock-on changes in unit tests, a new unit test simulating the original issue, and some associated changes in the host-side
ledger.h(removing the now-unnecessarystrictmode, and ensuring that read responses correctly describe the prefix they contain).Opening as a draft initially so I can let the CI find any tests I've missed, will open for review once I've done another cleanup pass.