Inner groups #3009
Inner groups #3009
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3009 +/- ##
===========================================
- Coverage 47.63% 29.88% -17.76%
===========================================
Files 359 357 -2
Lines 57964 57966 +2
===========================================
- Hits 27611 17321 -10290
- Misses 27231 38138 +10907
+ Partials 3122 2507 -615
Continue to review full report at Codecov.
|
| @@ -1318,7 +1318,7 @@ bitlen interprets arrays as big-endian integers, unlike setbit/getbit | |||
| - Opcode: 0xb3 | |||
| - Pops: _None_ | |||
| - Pushes: _None_ | |||
| - execute the current inner transaction. Fail if 16 inner transactions have already been executed, or if the transaction itself fails. | |||
| - execute the current inner transaction group. Fail if executing this group would exceed 16 total inner transactions, or if any transaction in the group fails. | |||
There was a problem hiding this comment.
did you consider changing the name of "itxn_submit", since "itxn" in the other opcodes is referring to a single inner transaction, whereas here it's referring to the whole group of them?
There was a problem hiding this comment.
Yeah, I originally thought I'd be explicit about the whole thing, wrap a bunch of matched itxn_begin and itxn_end pairs with itxg_begin, itxg_submit. But I couldn't justify all those opcodes once I started trying to write apps with them.
This seemed like the minimal change possible, and you can use itxn_submit to submit a single transaction, so I didn't want to waste an opcode on the distinction. But it's not crazy if people think we should go that way.
There was a problem hiding this comment.
I personally like it the way you did it, I think the additional opcodes aren't worth the minimal increase in explicitness/clarity.
| @@ -169,9 +169,10 @@ var opDocByName = map[string]string{ | |||
| "b~": "X with all bits inverted", | |||
|
|
|||
| "log": "write bytes to log state of the current application", | |||
| "itxn_begin": "begin preparation of a new inner transaction", | |||
| "itxn_begin": "begin preparation of a new inner transaction in a new transaction group", | |||
| "itxn_next": "begin preparation of a new inner transaction in the same transaction group", | |||
There was a problem hiding this comment.
An expanded description for this opcode would be helpful to explain when it's useful.
I.e., all inner transactions are atomic in the sense that if one fails, the entire top-level transaction fails. However, explicitly putting transactions into groups enables fee pooling and setting up the environment for inner transaction application calls.
|
Also, I noticed you didn't implement a |
Yes, that's true at the moment. I'm going to do all the |
|
Additionally, the group ID of the inner txns is never set. I agree this isn't necessary from a technical perspective, but setting the ID for each submitted group txn would make it possible to distinguish between normal txns and group txns. This is probably useful for observers of the chain. |
I've been thinking about this a lot, and I don't have a perfect answer yet. There are two interesting "fields" that appear in two "contexts". The fields are TxId and GroupID. The contexts are "as seen by AVM code" and "as seen on chain". TxID does not appear on chain, but there is a well-defined way to determine it from on-chain data (the fields of the transaction). GroupID does appear on chain (for top-level transactions) and it had better match the hashing scheme that defines it for the group members. Both are accessible from AVM. So, first for on-chain visibility, I can see some value in chain observers looking at a txn's list of inner-txns and being able to say "The first 3 were part of a group, then the next 2, and the last was a single transaction". I don't think there's a ton of such value, but it seems reasonable. That can be done in two ways. 1) Give each inner group a sequential group id. or 2) Give the group its "real" GroupID, based on the existing hashing rule. The first is cheaper, the second seems more correct. But notice that the second is still a little strange - you can get identical group IDs, even for adjacent groups, since there's no rule against duplicate transactions. This would actually mean chain observers could have a hard time differentiating consecutive identical groups. Now txid. Since it never appears on chain, we don't have to worry too much about that. But there have been calls for some sort of identifier for inner transactions. I have so far been against that, but we need to decide what So here is my current thinking. In an inner transaction, with teal <= 5, using Starting from teal 6, I've had some discussion of this with @fabrice102 and Shai. Further input welcome. For THIS PR, I'm not concerned yet, since itxn_next is behind Teal 6. |
I believe teal <= 3 should not be allowed in inner txns, since the resource access model is more relaxed in those versions (any asset or app that an account holds can be accessed using Seeing as that's the case, I can understand extending the restriction to teal <= 5 as well. My primary security concern for inner app txns is that a program designed before they were allowed (or detectable) might somehow be abused by being invoked as an inner txn, so requiring a minimum teal version of 6 would certainly solve that. I'm not yet convinced that this is a real problem though.
That certainly works, but does the AVM-exposed txID really need to be unique? If not, I can imagine either the top-level txn ID being reused, or a txID of all 0s potentially working. I'm still thinking through the implications of these options. |
data/transactions/logic/eval.go
Outdated
| // here, because they might never itxn_submit, or they might | ||
| // change the fee. Do it in itxn_submit. | ||
| fee = basics.SubSaturate(fee, *cx.FeeCredit) | ||
| if len(cx.InnerTxns)+len(cx.subtxns) >= cx.Proto.MaxInnerTransactions || |
There was a problem hiding this comment.
Once apps can be inner transactions, this code should ideally also count any child inner transactions created by txns in cx.InnerTxns too.
There was a problem hiding this comment.
Yes, I'll have to propagate down a sort of "tx budget" saying how many txns have been consumed already.
itxn_next to give us inner groups Questions remain around how TxID and GroupID should be defined for inners.
Introduces
itxn_next(feature flagged behind LogicVersion 6). This allows contracts to create inner transaction groups, not just singleton transactions. This is not of much value on its own, but is needed to make groups for inner app invocations. Until then, the only difference between running two inner txns consecutively, and running two as part of a group is that fee pooling allows the second txn to pay for a shortfall from the first.