Stop accepting transactions when number of delayed receipts is large #9222
Conversation
aborg-dev
force-pushed
the
delayed_receipts_limit
branch
3 times, most recently
from
3e38b3f to
3e15d78
Compare
aborg-dev
force-pushed
the
delayed_receipts_limit
branch
from
3e15d78 to
fa88507
Compare
aborg-dev
force-pushed
the
delayed_receipts_limit
branch
from
fa88507 to
a322cd8
Compare
aborg-dev
changed the title
aborg-dev
force-pushed
the
delayed_receipts_limit
branch
from
ce04542 to
41d0bbe
Compare
aborg-dev
force-pushed
the
delayed_receipts_limit
branch
from
41d0bbe to
789ef8b
Compare
I did test this with the Locust congestion workload with a much smaller limit of 50 receipts and didn't see any difference in the throughput of the system. The limit set in the PR most likely will be effective only against non-adverserial receipts as it is based on the count, not on size or gas, but we have to start somewhere. |
|
I guess I mean, we ideally should have some automated test coverage. Would it be much additional work to introduce a setting and then write a test against a very small limit? |
| // a bound of at most 10000 receipts processed in a chunk. | ||
| let delayed_receipts_indices: DelayedReceiptIndices = | ||
| near_store::get(&state_update, &TrieKey::DelayedReceiptIndices)?.unwrap_or_default(); | ||
| let min_fee = runtime_config.fees.fee(ActionCosts::new_action_receipt).exec_fee(); |
There was a problem hiding this comment.
This is the correct fee to look at, it's what an empty receipt would cost to execute. (Today, one can submit receipts with an empty action list. Probably we shouldn't allow it as it's useless for users. The smallest gas consumption of a useful receipt would be that of a transfer, which adds another 115 Ggas, so it would give us a factor of ~2 here.)
Anyway, back to the topic :)
I'm not sure if it's a good pattern to use config parameters in the actual code. We are doing it quite a bit, best example is a few lines below where we use storage_write_value_byte to limit the tx sizes. But it means changing those parameter values has sometimes unexpected effect on other parts of the code.
If it wasn't for the existing pattern, I'd say we should either use a hard-coded constant or add a new parameter. Where would we add the parameter? I'd probably put it in config.json. But hard coding it seems fine too.
But because we have precedence, I am also completely okay with this as it is. I'll let you decide with what you want to go.
There was a problem hiding this comment.
I agree with you that these unexpected dependencies on config parameters make code more complex. However, I think we do have this dependency even if we use a constant/new parameter here (as the chosen value need to take into account gas parameters) - it is just tracked more implicitly and not in the code. In the near future, we plan for this limit to be enforced by the protocol (at least the upper bound), so it can't be a part of the per-node config either.
It took me some effort, but I think I managed to come up with a test that is not fragile. I also rewrote the refunds test to be less fragile. PTAL |
aborg-dev
added
S-automerge
A-congestion
aborg-dev
force-pushed
the
delayed_receipts_limit
branch
from
a79ebd6 to
0fcf380
Compare
…9222) This PR introduces a soft limit on the number of delayed receipts in each shard. The limit is around 10000 delayed receipts to make sure this is enough to saturate the chunk capacity even if receipts are very small. The largest number of delayed receipts that we've seen in the last 3 months is around 400 and it stays at 0 most of the time. In the future, we would be also using other signals like the size of delayed receipts, but this information will have to be first computed and stored in the chunk headers/trie store and will be done in #9228. Addresses: #8877
…9222) This PR introduces a soft limit on the number of delayed receipts in each shard. The limit is around 10000 delayed receipts to make sure this is enough to saturate the chunk capacity even if receipts are very small. The largest number of delayed receipts that we've seen in the last 3 months is around 400 and it stays at 0 most of the time. In the future, we would be also using other signals like the size of delayed receipts, but this information will have to be first computed and stored in the chunk headers/trie store and will be done in #9228. Addresses: #8877
This PR introduces a soft limit on the number of delayed receipts in each shard. The limit is around 20000 delayed receipts to make sure this is enough to saturate the capacity of two chunks even if receipts are very small. The largest number of delayed receipts that we've seen in the last 3 months is around 400 and it stays at 0 most of the time.
In the future, we would be also using other signals like the size of delayed receipts, but this information will have to be first computed and stored in the chunk headers/trie store and will be done in #9228.
Addresses: #8877