Increase max app opt ins to 50 #2750
Conversation
algorandskiy
force-pushed
the
pavel/app-optins
branch
2 times, most recently
from
0ac6af5
to
dd69c6f
Compare
Codecov Report
@@ Coverage Diff @@
## master #2750 +/- ##
==========================================
- Coverage 47.12% 47.10% -0.02%
==========================================
Files 349 349
Lines 56326 56327 +1
==========================================
- Hits 26541 26535 -6
- Misses 26814 26816 +2
- Partials 2971 2976 +5
Continue to review full report at Codecov.
|
algorandskiy
force-pushed
the
pavel/app-optins
branch
from
dd69c6f
to
f33d101
Compare
| ApprovalProgram: []byte{0x02, 0x20, 0x01, 0x01, 0x22}, | ||
| ClearStateProgram: []byte{0x02, 0x20, 0x01, 0x01, 0x22}, |
There was a problem hiding this comment.
This will get clearer after my last PR. You'll put the actual program text in ApprovalProgram, and it'll get assembled on demand.
| // TestAppInsMinBalance checks that accounts with MaxAppsOptedIn are accepted by block evaluator | ||
| // and do not cause any MaximumMinimumBalance problems |
There was a problem hiding this comment.
I guess it should also be opted into 1000 assets, to make sure MaxMinBalance is correct.
There was a problem hiding this comment.
Also, the apps being opted into must have "full" schemas to get max min balance.
There was a problem hiding this comment.
Also, the apps being opted into must have "full" schemas to get max min balance.
Agree
I guess it should also be opted into 1000 assets, to make sure MaxMinBalance is correct.
No, because 1000 assets will consume entire MinMaxBalance alone (1000 * 0.1 = 100 that is almost 100.1 of MaxMinBalance). So a user needs to decide what he wants to store - if we want to change it, it needs to be a separate PR.
There was a problem hiding this comment.
I think that MinMaxBalance value is, essentially, a bug right now. I believe the spec says you can opt into 1000 assets and 10 apps. That's not true. But I think the intent is that it is.
I guess I don't care too much if it's in this PR or another.
There was a problem hiding this comment.
The spec only says without binding it 1000 assets or 10 apps or something. I might be missing something tho
A transaction must be rejected if its inclusion in a block would cause an account's minimum balance to exceed the maximum minimum balance of 100100000 microalgos.
There was a problem hiding this comment.
It also separately says both of the asset and app limits:
"Accounts can hold up to
"The maximum number of applications that a single account may opt in to is 10."
I suppose the statement on MaxMinBalance supersedes these statements, in a sense, but I think the intent was to set MaxMinBalance so that it did not prevent the other limits but we forgot to bump it when we added apps. I'm only guessing.
|
High level question - since we will have a different value for app creation limit and app optin limit, can I do BOTH? Create 10 apps (but not opt in to them) and then opt in to 50 more? I don't know if we currently allow 10 creations and then 10 (separate) opt-ins. I suppose we do, so I suppose this 10+50 I mentioned above will be allowed as well. |
algorandskiy
force-pushed
the
pavel/app-optins
branch
2 times, most recently
from
82b9729
to
18d0cb1
Compare
Yes, it is still allowed: 1) checks are in different places - create app / optin app. 2) |
|
hm. I'm a bit concerned about the timing non-symmetry between the block assembly and validation. When we create a proposal, we limit the time to a specific threshold (250ms). The assumption is that the block validation would take roughly the same time. However, the transactions used in the block assembly are taken from the transaction pool - which means that the corresponding accounts have already been loaded into memory. When receiving a transaction to validate - we do not have that "luxury" of knowing that the account information was already loaded into memory. Instead, we'll need to fetch it from disk (bummer..). As long a the account's data wasn't substantial, it was perfectly fine. However, in case we have large accounts, we need to make sure that we'll be able to load all of them into memory in a reasonable time. @pzbitskiy's test was already on the right track - but it would need to be "fine-tuned" for the worst case scenario: |
The test already does it - it reopens the ledger so evaluation happens on empty caches. Plus all 5k txns are from random addresses out of 10k opted in accounts so they are presumably different. Data on 5k txn per block:
|
|
@tsachiherman @Priegle thoughts? evaluation of 5k txns on accounts with full local state in each of 50 app slots would takes 3.2s on a cold DB. |
No - that's exactly what I was looking for. It tells me that it's fine to increase the number to application to 50 as long as our block size is roughly 5k transaction. If we will make an attempt to increase it, we could see an inevitable increase in round time. |
|
Btw, as JJ noted, we already have a similar problem with 1k assets per account. |
|
Chris pointed out to the code // makeProposals creates a slice of block proposals for the given round and period.
func (n asyncPseudonode) makeProposals(round basics.Round, period period, accounts []account.Participation) ([]proposal, []unauthenticatedVote) {
deadline := time.Now().Add(config.ProposalAssemblyTime)
ve, err := n.factory.AssembleBlock(round, deadline)
if err != nil {So the assembly deadline is hard. For a block verification case looks like agreement switches if there is not enough cert votes to a next period that lead so a longer round time. |
|
No, that's not why it would increase the block time. Once the agreement
recieve the proposal, it starts validating it. It cannot move to the next
round without completing the validation process since it need the state
deltas ( regardless of the incoming cert votes ). If the validation starts
at r+2, and it takes 3.2s, then the new round would start at r+5.2. The
ensure digest path would be skipped, since we have the proposal.
…On Tue, Aug 24, 2021, 19:50 Pavel Zbitskiy ***@***.***> wrote:
Chris pointed out to the code
<https://github.com/algorand/go-algorand/blob/master/agreement/pseudonode.go#L268-L277>
// makeProposals creates a slice of block proposals for the given round and period.func (n asyncPseudonode) makeProposals(round basics.Round, period period, accounts []account.Participation) ([]proposal, []unauthenticatedVote) {
deadline := time.Now().Add(config.ProposalAssemblyTime)
ve, err := n.factory.AssembleBlock(round, deadline)
if err != nil {
So the assembly deadline is hard.
For a block verification case looks like agreement switches if there is
not enough cert votes to a next period that lead so a longer round time.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2750 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AF2OOH5BGYRLPSN2FQ7DR33T6QV23ANCNFSM5CIHZ5HQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
|
Chris and I went through |
Kindof. It's not an "or". It takes as many transactions as it can within the And the issue is on the validator, which doesn't have a validation time limit. |
|
It looked like AssembleBlock() signals the deadline and round number to the block evaluator, then there is an additional |
|
I think that placing hard limits on the block generation time is a good
idea. We can definatally tweak the numbers, but keep in mind that we expect
the validator to spend about the same time on the validation, and we need
to ensure we provide enough time for the proposals to propogate throughout
the network and meet the 4s timeout. We should do our best to avoid
surpassing the 5 second round time, as this is one of Algorand's advertised
key advantages.
…On Thu, Aug 26, 2021, 10:38 chris erway ***@***.***> wrote:
It looked like AssembleBlock() signals the deadline and round number to
the block evaluator, then there is an additional assemblyWaitEps of 150ms
extra time for AssembleBlock() to wait for the block evaluator to finish up
a partial block before giving up and returning an empty block? Is that
generous enough?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2750 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AF2OOH2FZ5XSD6RERA2TH63T6ZGV3ANCNFSM5CIHZ5HQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
|
On the validation side, from agreement's perspective I don't think there needs to be a deadline for BlockValidator.Validate() — as long as enough nodes validate proposed blocks eventually, agreement will progress. Giving up on valid proposed blocks may hurt liveness. If we miss the first filter/soft-vote timeout there will be more opportunities in future periods to come to consensus on the best block.. however like Tsachi says improving the performance of block validation will keep us from having to go off the fast path and into a new period. |
|
Looking at the agreement code more deeply, soft votes are issued based on the lowest observed proposal-value, but the cert vote requires a "committableEvent" (proposalCommittable) from the proposalStore that will only be returned if the block was validated (after receiving payloadVerified). So if the block was still not validated by the time the soft vote quorum is observed, the cert vote would not be issued but would eventually be issued later when the late payloadVerified event was delivered to the player. Assuming the validation takes less than 17 additional seconds after the soft vote was issued (itself a 4 second timeout from the start of the round), the cert vote would go out for period 0 without requiring a next vote to a new period. |
|
That's correct - it would still complete within the step 2 timeout of 17s;
but it also mean that we failed to reach consensus under 5s.. which is one
of our goals.
…On Fri, Aug 27, 2021, 14:32 chris erway ***@***.***> wrote:
Looking at the agreement code more deeply, soft votes are issued based on
the lowest observed proposal-value, but the cert vote requires a
"committableEvent" (proposalCommittable) from the proposalStore that will
only be returned if the block was validated (payloadVerified). So if the
block was still not validated by the time the soft vote quorum is observed, the
cert vote would not be issued
<https://github.com/algorand/go-algorand/blob/master/agreement/player.go#L298-L300>
but would eventually be issued later when the late payloadVerified event
was delivered
<https://github.com/algorand/go-algorand/blob/master/agreement/player.go#L601-L603>
to the player. Assuming the validation takes less than 17 seconds after the
soft vote was issued, the cert vote would go out for period 0 without
requiring a next vote to a new period.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2750 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AF2OOH4E5H5UTODXD6LGNGLT67K3NANCNFSM5CIHZ5HQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
| @@ -42,7 +42,7 @@ const ( | |||
|
|
|||
| // MaxEncodedAccountDataSize is a rough estimate for the worst-case scenario we're going to have of the account data and address serialized. | |||
| // this number is verified by the TestEncodedAccountDataSize function. | |||
| MaxEncodedAccountDataSize = 750000 | |||
| MaxEncodedAccountDataSize = 850000 | |||
There was a problem hiding this comment.
We talked about this earlier, but I'm wondering if this takes into account worst case asa creation. I think that can be very big.
* Fixed error check in new ledger testing harness * Unit tested ledger.apply and ledger.eval * Benchmarked 10 opt-ins vs 50 opt-ins eval times on reopened ledger
algorandskiy
force-pushed
the
pavel/app-optins
branch
from
9806ff4
to
23da91c
Compare
|
Rebased |
Summary
As a temporary relief increase max number of opt ins to 50.
Also found and fixed an error check in new ledger testing harness.
Test Plan
Benchmarks
Benchmark 1: create 100k accounts, 500 apps (no local data) and opt-in 10k accounts to max number of apps. Restart ledgers, measure a single block validation time of 50k pay transactions on random opted-in accounts.
Benchmark 2: same as 2 but with full local data (16 keys each 64 bytes long with 64 bytes of value data)
Benchmark 3: same as 2 but 5k txns per block