-
Notifications
You must be signed in to change notification settings - Fork 450
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
Application transactions #2661
Application transactions #2661
Conversation
data/transactions/logic/eval.go
Outdated
@@ -159,6 +173,8 @@ type LedgerForLogic interface { | |||
DelGlobal(key string) error | |||
|
|||
GetDelta(txn *transactions.Transaction) (evalDelta basics.EvalDelta, err error) | |||
|
|||
Perform(txn *transactions.Transaction, spec transactions.SpecialAddresses) error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LedgerForLogic needs to be able to "Perform" a transaction now.
ledger.NewApp(txn.Txn.Sender, 100, basics.AppParams{}) | ||
ledger.NewLocal(txn.Txn.Sender, 100, "ALGO", algoValue) | ||
ledger.NewAsset(txn.Txn.Sender, 5, params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaner ways to set up complicated balance tables for testing. This was, coincidentally, recently requested from an outside contributor.
@@ -379,6 +380,16 @@ type AssetParams struct { | |||
Clawback Address `codec:"c"` | |||
} | |||
|
|||
func (app AppIndex) ToBeHashed() (protocol.HashID, []byte) { | |||
buf := make([]byte, 8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a wild idea - the address we're going to pick is a pseudo-account public address, right ?
if so, can we pick an address that has no private key ?
another thought is to have that address be programmable - like the Rekey/AuthAddr. I'm not sure what this is good for, but I think that someone could find a good use for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're just putting our faith in the magic of cryptographic hashes, right? The intent is that the newly allocated address does not have a private key (or, equivalently, such a key exists, but nobody can find it). It's the same reasoning behind logicsig's controlling an address that we certainly hope does already belong to somebody.
I wrote all that before really realizing what you meant - I'll leave it in case it's helpful to someone else. I now realize you're asking if, upon app creation, maybe we can pick our app address, as long as the account is not in use. I think that's insecure because I may know your public address before you fund it (this would be particularly true of a templatized logic sig that has not been instantiated yet, it would also be true if I watch the transaction pool). I should not be able to make my app use that address, or else when you "come into existence" the app will control your funds.
As for setting the address. I think the rule you would want is that you can only give the app authority over an account that you yourself have authority. That is, you certainly don't want to reset an app address to a big foundation address and ask it for money! So I believe that we accomplish this by approaching it from the opposite direction. We allow you to rekey your account to the app's address. And then it has authority. But, of course, you can only rekey that account because you own it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @tsachiherman is actually asking about choosing an invalid ed25519 curve point such that no private key exists that could match the address. I believe the MainNet fee sink and rewards pool are invalid points, though I'm not sure how to derive such an invalid point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct - what I meant was to select a ed25519 which no private key exists for. This is a post-quantum guarantee equivalent, since no crypto operation would generate that point. The x coordinates on ed25519 curve are mostly on the positive side; in fact, if you'll pick a number on the negative side that is smaller than -486662, I believe that there won't be any solution for that. ( i.e. that number would not be on the curve.. )
In fact, you don't even have to use a hash here; you can just use the -486662 - AppIndex
. But I do realize it might not be as obfuscated as a hash ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had no idea that the public key was used so directly - I sort of assumed it seeded a PRNG that was then used to pick points. I assume by "negative", you mean the 32 bytes is treated as a two-complement u256. So really we could pretty just flip the high bit?
I am far too timid to do anything like that without three or four cryptographers telling me it's a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely agree that we should consult a cryptographer or two here. The randomness you're referring to is used when generating the private key; the public key is a pretty trivial calculation from there :
int
crypto_sign_ed25519_seed_keypair(unsigned char *pk, unsigned char *sk,
const unsigned char *seed)
{
ge25519_p3 A;
crypto_hash_sha512(sk, seed, 32);
sk[0] &= 248;
sk[31] &= 127;
sk[31] |= 64;
ge25519_scalarmult_base(&A, sk);
ge25519_p3_tobytes(pk, &A);
memmove(sk, seed, 32);
memmove(sk + 32, pk, 32);
return 0;
}
Note that there is another randomness taking place when verifying a signature ( irrelevant in here ).
I do like the idea that you've had of using a hash function, as it "distributes" the account addresses as well, btw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work and looks like a clever integration into existing code. I guess a little more code for updating ApplyData
with side effects left, and quite a bit tests in ledger.
I do not like some code duplication in Perform
and tx_submit
but have no better ideas on how to get rid of it.
@@ -464,6 +464,7 @@ FirstValidTime causes the program to fail. The field is reserved for future use. | |||
| 7 | LatestTimestamp | uint64 | Last confirmed block UNIX timestamp. Fails if negative. LogicSigVersion >= 2. | | |||
| 8 | CurrentApplicationID | uint64 | ID of current application executing. Fails if no such application is executing. LogicSigVersion >= 2. | | |||
| 9 | CreatorAddress | []byte | Address of the creator of the current application. Fails if no such application is executing. LogicSigVersion >= 3. | | |||
| 10 | CurrentApplicationAddress | []byte | Address that the current application controls. Fails if no such application is executing. LogicSigVersion >= 5. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a controlled address is cool. Is there any application benefit for supporting multiple application controlled accounts for a single application ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is at least one - suppose you wanted to escrow algos from many accounts, and always be wiling to give them back, with the rewards they've earned in the meantime. Since rewards are impossible to calculate in contract, you'd need to segregate your deposits, rather than keep them in one pot.
I originally planned on having a way to create any number of app accounts, for exactly this reason (and some more obscure ideas). But I think that the ability to rekey to the app addr is powerful enough. To use such a contract, a depositor would make a new account, opt it into the contract, and then, in an atomic group:
- make a deposit
- call the contract to bind the new address to the depositor (by writing the deposit address in address's app local state)
- rekey the new account to the app account
Now, when depositing or withdrawing, the caller tells the contract where his deposit account is. So the contract can use these extra accounts that were ceded over to it because of rekeying. Which also prevents the caller from messing with them ever again.
(If you really like cleanliness, you'd have an operation to ask the contract to close out the account if you wanted to withdraw it all.)
xrun(['goal', '-v'], env=env, timeout=5) | ||
xrun(['goal', 'node', 'status'], env=env, timeout=5) | ||
|
||
rs = RunSet(env) | ||
for scriptname in args.scripts: | ||
rs.start(scriptname, args.timeout-10) | ||
rs.start(os.path.abspath(scriptname), args.timeout-10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is a good change, but I'm not sure it has to be coupled with the rest of this PR.
Allow devs to get an interactive shell in e2e_client_runner. Make it easier to run an e2e sub test by hand with client runner.
29da897
to
ad3f328
Compare
EvalDelta moved because we need to put a []SignedTxnWithAD inside it, which would have been circular. LogItem moved with it because it seems appropriate - not a very "basic" type. It is only in EvalDelta.
This also simplifies Log handling by using the same technique. Lots more test of Pay in teal (with proper rewards handling)
a13c0a3
to
d43b0e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess all my remarks are resolved:
- fee credit is calculated based on MinFee proto param in order to be deterministic. Max inner txn is set to 16.
- min balance checks are applied to inner txns and accounts appear in cow's modifiedAccounts() that is confirmed in tests
TxnCounter
update might be done in a separate PR
@@ -811,7 +811,10 @@ | |||
], | |||
"responses": { | |||
"200": { | |||
"$ref": "#/responses/PendingTransactionResponse" | |||
"description": "Given a transaction id of a recently submitted transaction, it returns information about it. There are several cases when this might succeed:\n- transaction committed (committed round \u003e 0)\n- transaction still in the pool (committed round = 0, pool error = \"\")\n- transaction removed from pool due to error (committed round = 0, pool error != \"\")\n\nOr the transaction may have happened sufficiently long ago that the node no longer remembers it, and this will return an error.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my comment was ambiguous. I mean the description
field here should probably have value OK
, since I believe it's purpose is to describe what a 200 response means. For example see the respones for /v2/applications/{application-id}
.
require.Equal(t, amount, uint64(0)) | ||
|
||
eval = l.nextBlock(t) | ||
eval.txn(t, withdraw.Noted("3"), "underflow on subtracting") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to test that an app can't overspend assets, correct? So why isn't the error "insufficient balance" or similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually: "underflow on subtracting 101 from sender amount 100" or some such. I suppose I could construct the entire message.
b46ee96
to
6ba1f0e
Compare
3fd6681
to
7d3e1e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny request, but other than this and updating the old LogItem tests we talked about, this is looking good
@@ -1748,7 +1751,7 @@ | |||
"logs": { | |||
"type": "array", | |||
"items": { | |||
"$ref": "#/definitions/LogItem" | |||
"type": "string" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add "format": "byte"
here too to convey this is a base64 encoded binary string. This can maybe be done in a separate PR though.
|
||
// Intentionally, temporarily wrong - need to decide how to | ||
// allocbound properly when structure is recursive. Even a bound | ||
// of 2 would allow arbitrarily large object if deep. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the solution for this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turned out that the allocbounds are not our only protection. We don't decode streamed msgpack messages. We take them off the wire, with bounded size, and then decode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given what Nickolai said about msgpack pre-allocating an array with a decoded integer size, I think there can an amplification attack where a few decoded bytes allocate 16 new SignedTxnWithAD
objects.
Something like this when decoding SignedTxnWithAD
:
dt itx 16 dt itx 16 dt itx 16 ...
Allow AVM 1.0 programs to "execute transactions"
This is a pretty big change so I will comment on individual pieces by reviewing my own code, but the high level summary is.
So far, only pay and axfer are allowed. acfg and afrz are under consideration, but have separate issues written.
This makes ApplyData a recursive datastructure, so that the "inner" transactions of app calls can be encoded in EvalDelta and, themselves, have ApplyData. However, this PR does not yet expose that new inner transactions list to the REST API. Another issue covers that need.