Eval: Move congestion monitoring and fee escalation on-chain #6400
Conversation
jannotti
force-pushed
the
on-chain-congestion
branch
2 times, most recently
from
dd908fe to
f17cee2
Compare
jannotti
changed the title
jannotti
force-pushed
the
on-chain-congestion
branch
10 times, most recently
from
b5f1d18 to
68c0b54
Compare
jannotti
force-pushed
the
on-chain-congestion
branch
from
53acaf3 to
574aefd
Compare
❌ 1 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
jannotti
force-pushed
the
on-chain-congestion
branch
5 times, most recently
from
87fa07d to
8b6d6ee
Compare
| // EnableFeePooling specifies that the sum of the fees in a | ||
| // group must exceed one MinTxnFee per Txn, rather than check that | ||
| // each Txn has a MinFee. | ||
| EnableFeePooling bool | ||
|
|
There was a problem hiding this comment.
We don't need this any more, so I removed it for clarity. It enabled strictly more permissive fee handling, so we can pretend it was always true.
|
|
||
| api: | ||
| make -C daemon/algod/api | ||
| $(MAKE) -j7 -C daemon/algod/api |
There was a problem hiding this comment.
doubles the speed of make api
data/bookkeeping/block_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func TestNextBaseFee(t *testing.T) { |
There was a problem hiding this comment.
BaseFee can move by 50% up or down each round. We should decide whether we want it to be able to move that fast.
In comparison, Ethereum can only change 12.5% per block (and their blocks are also slower).
| if len(txgroup) == 1 { | ||
| t := txgroup[0].Txn | ||
| if t.Type == protocol.StateProofTx && t.Sender == transactions.StateProofSender && t.Fee.IsZero() { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This is now handled by having the stateproof transaction not contribute in SummarizeTxnFees.
| err := pool.checkSufficientFee(txgroup) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
JJ's note to self. checkSufficientFee probably should not have moved down. Investigate.
| func (pool *TransactionPool) addToPendingBlockEvaluator(txgroup []transactions.SignedTxn, recomputing bool, stats *telemetryspec.AssembleBlockMetrics) error { | ||
| err := pool.addToPendingBlockEvaluatorOnce(txgroup, recomputing, stats) | ||
| if err == ledgercore.ErrNoSpace { | ||
| pool.numPendingWholeBlocks++ |
There was a problem hiding this comment.
Removing this in the name of taking out old-style congestion code may be a mistake. It means we can't remove a transaction that has been in the pool too long from here, because we're not keeping track of a sort of "virtual round".
But, who cares? It'll get kicked out by eval before it gets into an actual block.
| case *transactions.MinFeeError: | ||
| asmStats.MinFeeErrorCount++ | ||
| stats.RemovedInvalidCount++ | ||
| pool.log.Infof("Pending transaction in pool no longer valid: %v", err) |
There was a problem hiding this comment.
This error is no longer possible (and has not been for a while), because FeePooling means we don't check fees on individual transactions.
jannotti
force-pushed
the
on-chain-congestion
branch
from
8b6d6ee to
498d8b1
Compare
jannotti
force-pushed
the
on-chain-congestion
branch
2 times, most recently
from
043bc35 to
04d6e98
Compare
jannotti
force-pushed
the
on-chain-congestion
branch
from
04d6e98 to
98ce027
Compare
jannotti
force-pushed
the
on-chain-congestion
branch
from
98ce027 to
76b307d
Compare
I think that we should think about swapping I also think |
If a program says (today) that it only wishes to run when the
It only includes the amount over the minfee that the txn is willing to pay, that's why I called it an "increment". I defined it this way so that when it is 0, it can be omitted and when it is small, the encoding is small, rather than forcing every transaction to include an encoding of the minfee. It encodes a small number x, such that the group is willing to pay (1+x) * minFee for each basic transaction. The x is expressed in millionths, though I don't know that we need that much precision. |
I'm not talking about logic sigs but apps sending inner txns with To be fair I think most apps just defer fee to outers but I do recall some PyTeal contract that used
Ah ok that makes sense. |
That's what I'm hoping. Recall that there is (perhaps misguided) feature that prepopulates an inner txn's fee with the amount needed. So if an app wants to pay the fee, they should not set it at all. As for the prepopulation feature, I am considering intentionally not letting that populate with anything more than
I wonder if the field in the block should be defined the same way. Right now, it's a MicroAlgos amount, to be added to the MinFee. But perhaps it should also express a factor that should multiplies the MinFee. (And express that factor as just the amount > 1.0, so it stays small.) Perhaps instead of "Increment" I should call these things "Tax" since that is something people are familiar with that is defined the same way. 6% sales tax means multiply the cost by 1.06. |
When he said applying to inners I assume he just meant in the same way fee does now - paying for unknown numbers of opups, or c2c calls - but inner transactions right now are always 1000 microAlgo. It seems like this is all about the outer 'get in the door' fee, but also how much may still be applied to inners (I'm still assuming 1000 microAlgo here). Changing what itxns cost seems like a large change and would most certainly break contracts. |
|
I expect that inners will need to pay more during congestion, just like any other transaction.
If apps DO want to pay the inner fee themselves, how do they do it today? Do most of them allow the prefilled default fee to be used? Or do they hardcode 1000 (or global MinFee?) Either way, during congestion, the caller might need to supplement from the top-level. That doesn't seem or if keeping with the way most things will need to work. During congestion, pay more at the top. The mechanism to do so cleaning is non-trivial, but we already know simulate and wallets need to improve in that regard. |
Right, I suppose if there is some app that's using it the clients could be updated to cover the extra cost via the outer txns.
Yeah I think that makes sense. An app that fails unexpectedly under congestion seems preferable to an app that spends more than you'd expect during congestion.
Fwiw most apps I've seen just defer to the outer. TEALScript and Puya both actually do |
This has been my pattern, but I think what might happen (in the same way we have to handle MBR) is that apps might convey to callers how much is needed to call them. They wouldn't be able to ever give that info here and instead simulate becomes a hard necessity and contracts can't tell callers (whether another contract or 'user/automation') how much extra they might need. Some of my code this might be a known fixed quantity but recently I've also coded with the thought of - it costs what it costs - do simulate and it figures it out. So, maybe it's not an issue? One part of me is ok with 'a transaction' whether outer or inner scaling appropriately but they are different internally and have different costs so the 'entry fee' vs extra opcode cost always seemed logically fine to me. If we want to have the entry fee pass onto the inners then it's a different model altogether - it's not per byte, and it's also not 'per byte + x per y opcodes'. If it just gets explained as 'fee per transaction' whether outer or inner (but what does it mean when you just need to buy up opcodes? what's that unit??) that's fine but it obviously is a big change from 1000 fixed microAlgo cost per inner. |
3 participants
Currently, congestion is monitored locally, by each
algodand is not a part of consensus. So while individualalgods may require higher fees, transaction processing is unaware of that requirement. Therefore, the extra fee can end up being "double spent" to execute inner transactions. This PR introduces two header fields,Loadwhich indicates how full a particular block is, andCongestionFeewhich grows when theLoadexceeds 50% and returns towardMinFeewith less full blocks. The long term plan is that transactions must payMinFee + CongestionFee(which we'll callBaseFee) to send a transaction. A group indicates its willingness to be chargedBaseFeeper transactions by including a new transaction field,CostIncrementin the transaction header. This is an amount, inMicroAlgosthat the group is willing to be charged for each transaction, including inners. It is not added to the transaction'sFeethough. Instead, the group is stating that the fees it has included will be sufficient if, during evaluation, each transaction is charged at the higher rate.For now, concessions are being made to roll out congestion pricing over multiple consensus releases. The concession is this: Transactions will be dropped when they arrive at
algodif the current block'sCongestionFeeis higher than the submitted group'sCostIncrement, but groups will not be dropped at evaluation time unless they fail to included enoughFeefor their statedCostIncrement. The result of this policy is that once a group is admitted to the transaction queue, it will not be dropped simply because the congestion has increased, but the group has not gotten into a block yet. However, groups that were only admitted because they included a sufficientCostIncrementwill still be held to that standard.This means that congestion is still outside the protocol, but the groundwork was been laid to first track congestion in protocol, and eventually enforce it at evaluation time.
Why don't we enforce it with this update? Because it would lead to an ugly pathology. Suppose there is no congestion. Lots of transactions arrive with
MinFeefees, and noCostIncrement. As soon as one block is created that's more than 1/2 full, the next block would demand aCostIncrementso all those sent transactions would be dropped. We'd have an empty block, congestion would go down, and then a bunch ofMinFeetransactions would be sent again.To avoid that problem will require some sort of "pay-as-you-go" or "refund" model. Transactions will need to specify that they are willing to pay more, but only if there's congestion that demands it. If congestion is less than they have indicated their willingness to pay, they will keep this part of their fee. To avoid backward incompatible changes, we will leave
Feeto mean, "I always pay this fee". There will need to be a new field, perhapsRefundableFee, which does not necessarily go to the fee sink. There are several issues with that change, so prudence leads us to put that off a bit. Which transactions receive the refund, if the fee is split among transactions? How can logicsigs be protected from unintentional spend? Do apps need similar protection?So: the short term solution for this PR and the next consensus release is to only enforce the changing congestion value at ingress, then lock that choice in for checking during evaluation even if the congestion level changes. This gives the community time to understand the new congestion model, since it will appear in the block. Wallets and such can begin to offer recommended fees.
While in this code,
EnableFeePoolingis eliminated as a consensus parameter. Since it is strictly more permissive, we can simply assume it has always been true.A worthwhile discussion: How quickly should the congestion level change. The current code causes the BaseFee (sum of congestion and minfee) to go up by 50% when the block is full, or down by 50% when the block is empty. As a comparison, Ethereum only allows the congestion fee to change by 12.5% per block. I lean toward adjusting this PR to use a 10% limit each way.
Summary
Test Plan