AVM: Allow access to boxes for apps made in the same group without explicit boxrefs#6309
Conversation
We know they are empty, so we need not worry about exceeded read quota. We will still treat them as dirty, so we need not worry about exceed write quota. We bound the number of boxes that can be accessed this way by the number of empty box refs. This means that the _number_ of boxes written is bounded, not just the sum of the lengths.
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for unnamed box access in newly created apps within the same group. It updates transaction and box logic to allow a one‑time empty box reference access, and adds comprehensive tests to verify the new behavior.
- Added tests in boxtxn_test.go to cover various unnamed box creation cases.
- Updated resources and box handling in the logic package to account for unnamed access.
- Modified consensus parameters in config/consensus.go to enable the new feature.
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ledger/boxtxn_test.go | New tests for unnamed box access and minor documentation updates |
| data/transactions/logic/resources.go | Added tracking for unnamedAccess in box resources |
| data/transactions/logic/box.go | Updated availableBox to permit unnamed box access in new apps |
| config/consensus.go | Added the EnableUnnamedBoxAccessInNewApps consensus parameter and set it in vFuture |
Files not reviewed (1)
- Makefile: Language not supported
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6309 +/- ##
==========================================
- Coverage 51.60% 51.59% -0.01%
==========================================
Files 649 649
Lines 87013 87070 +57
==========================================
+ Hits 44903 44925 +22
- Misses 39244 39273 +29
- Partials 2866 2872 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
[Nit] Consider amending the title to the following for clarity:
|
I hope the current title is clearer. |
|
The following concern is noted:
That's a good call-out. I'm not sure exactly how to handle it, but we should certainly do something. I'm not exactly sure what the state of auto resource population is. But yes, we certainly don't want to tell callers: You need a box ref for {28734, "hello"} just because the app created during simulation happened to be 28734. We need to report that adding {0,""} would make the call work. |
This includes some additional bugs fixes. Previously, `simulate` would report a boxref that included a newly created app's ID when used with AllowUnnamedResources. That is, in some sense, correct, but useless, since a follow-on transaction that used such a box ref would surely have the wrong app id, and would still fail. Now, such a box ref will be converted to an empty ref if EnableUnnamedBoxAccessInNewApps is true. I considered always converting, but there are corner cases in which a very clever user of simulate could correctly use the old result. After the change, the new result is always strictly easier to use.
71519d6 to
4ab48f2
Compare
It will now properly report that, say, 3 empty refs are needed, if you touch 3 boxes in a newly created app. Previously, the simulate endpoint would simply report the app,name combo for any app touched, even if the app was a new ID. If a caller used that information blindly, it would certainly fail because the appID would be different when submitted. You might be able to construct torturous uses that would work, if the caller paid enough attention to the result to note that the appID returned matches the appID created in a later top-level transaction. So we left the old behaviour until a consensus change that enables this feature. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a feature that allows newly created apps within a transaction group to access boxes without explicit box references. The change enables more flexible box creation patterns for inner apps and eliminates the need to predict app IDs for box references in certain scenarios.
Key changes:
- Adds
EnableUnnamedBoxAccessInNewAppsconsensus parameter to enable the feature - Introduces empty box references that can be "consumed" to allow box access in newly created apps
- Updates simulation and resource tracking to handle the new access pattern
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
config/consensus.go |
Adds new consensus parameter EnableUnnamedBoxAccessInNewApps |
data/transactions/logic/resources.go |
Implements tracking of unnamed access slots from empty box refs |
data/transactions/logic/eval.go |
Updates evaluation to handle new app box access and surplus budget calculation |
data/transactions/logic/box.go |
Core logic for allowing box access in newly created apps without explicit refs |
ledger/simulation/resources.go |
Updates resource tracking to support new box access patterns and I/O surplus handling |
ledger/boxtxn_test.go |
Comprehensive tests for the new box access capabilities |
| Multiple test files | Updates to handle new BoxStat structure and simplified resource tracking |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
gmalouf
left a comment
There was a problem hiding this comment.
Left a few questions/comments
Co-authored-by: Gary Malouf <982483+gmalouf@users.noreply.github.com>
| const boxQuotaBumpVersion = 41 | ||
| const ( | ||
| boxVersion = 36 | ||
| accessVersion = 38 |
There was a problem hiding this comment.
That v9 of AVM, and tx.Access is not allowed until then. (It's when resource sharing happened, so I couldn't easily make tx.Access work for earlier versions.)
Summary
We have heard many requests to allow for the creation of boxes in newly created apps. That appears to already be allowed, since 0 signals "current app" in a BoxRef. But that doesn't work for inner apps, since BoxRefs only appear at the top-level and they are "resolved" there, to the app id at the top-level.
This seems like a naming issue, "How can we name these inner create boxes"? That seems hard. But, we are in luck, because there's really no reason to be so stingy with access to boxes for newly created apps. Those boxes can't exist, because the app is new, so we can short circuit the lookup on disk.
We know they are empty, so we need not worry about exceeded read quota. We will still treat them as dirty, so standard write quota checks will handle that side.
We bound the number of boxes that can be accessed this way by the number of empty box refs. This means that the number of boxes written is bounded, not just the sum of the lengths.
Test Plan
Added new tests in
boxtxn_test.gothat exercises the new capabilities (and ensure they are no usable in old versions)