Add box length to error messages #9
Closed
Conversation
This file contains hidden or 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
At least three four left to consider, as well as extensive testing. 1. It has no LRU cache for the reads and writes. I expect sqlite to do a fine job caching kv pairs, and there's not savings from skipping msgpack serialization. But sqlite has a page cache, not a row cache, so maybe it's worth it. 2. There is no loop around the sql read with round number test. 3. There's is no locking of au.accountsMu. This is clearly wrong, need to learn more about the rules that govern when the other lookup routines are _not_ taking lock. 4. Need to think through limits on total boxes read/written, and how boxes can be "named" for access by new apps.
Still need to think about the total size of boxes accessible to a txn, but that check must happen later, as it requires using state. Two options: A static limit on accessible stuff, or a dynamic limit on the sum of the size of boxes actually touched. Probbaly the static limit is better, since we may wish to prefetch.
8 tasks
jannotti
force-pushed
the
avm-box
branch
4 times, most recently
from
4b31e78 to
ac68b9d
Compare
Author
|
Closing - Feedback brought in via algorand@a0c19b8. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Proposes an optional change to add more context to Box error messages considered while reviewing algorand#4001.
To aid developers debugging Box extract and replace operations, the PR adds the box length to the out-of-bounds error message.
If it's preferable to keep as is, feel welcomed to close the PR. I don't feel strongly.