fix(congestion) - remove error log in promis yield handling

[wacban]

There is no need to print an error in this case. Congestion Control does not need to account for promise yields as indicated in the comment. This is to fix #11873.

[jakmeier]

Do you know why this code path is triggered? I thought PromiseYield is never in the delayed receipt queue, nor in the outgoing receipts buffer.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.64%. Comparing base (3ce5ed5) to head (fcdc4f0).

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11875      +/-   ##
==========================================
+ Coverage   71.62%   71.64%   +0.01%     
==========================================
  Files         806      806              
  Lines      164328   164327       -1     
  Branches   164328   164327       -1     
==========================================
+ Hits       117703   117726      +23     
+ Misses      41556    41536      -20     
+ Partials     5069     5065       -4     

Flag	Coverage Δ
backward-compatibility	0.23% <ø> (ø)
db-migration	0.23% <ø> (ø)
genesis-check	1.34% <ø> (+<0.01%)	⬆️
integration-tests	37.78% <ø> (+0.07%)	⬆️
linux	71.42% <ø> (+0.01%)	⬆️
linux-nightly	71.25% <ø> (+0.02%)	⬆️
macos	54.52% <ø> (+0.61%)	⬆️
pytests	1.61% <ø> (+<0.01%)	⬆️
sanity-checks	1.41% <ø> (+<0.01%)	⬆️
unittests	66.06% <ø> (-0.01%)	⬇️
upgradability	0.28% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[wacban]

Do you know why this code path is triggered? I thought PromiseYield is never in the delayed receipt queue, nor in the outgoing receipts buffer.

Not fully but judging by the following lines, PromiseYield is a valid receipt type to be buffered.

nearcore/runtime/runtime/src/lib.rs

Lines 846 to 852 in 3ce5ed5

	new_receipt.set_receipt_id(receipt_id);
	let is_action = matches!(
	new_receipt.receipt(),
	ReceiptEnum::Action(_) | ReceiptEnum::PromiseYield(_)
	);
	let res = receipt_sink.forward_or_buffer_receipt(

[wacban]

cc @saketh-are Can you have a look? We may be missing something here. The comment in the receipt_congestion_gas states that PromiseYield shouldn't move across shards but it seems to be doing just that in testnet.

[jakmeier]

Yeah we should figure out why this happens. The error was placed there to ensure we miss no receipts that take up queue or buffer resources.

If we are indeed not tracking PromiseYield receipts properly, we need to document it properly and should probably make a fix. But I think we could delay a fix to a future protocol version, it probably shouldn't block the feature from going to mainnet.

It could also be that we somehow do the gas calculation on receipts without checking first if it's for a remote account or not. That would be an easier fix.

[saketh-are]

cc @saketh-are Can you have a look? We may be missing something here. The comment in the receipt_congestion_gas states that PromiseYield shouldn't move across shards but it seems to be doing just that in testnet.

Are you sure? The reciever id for both PromiseYield and PromiseResume receipts is always the current account (I just double checked this in the code here and here), so they should never move across shards. Could it be that what you are seeing is that receipts which are destined for the local shard still go through the outgoing receipts buffer?

I thought PromiseYield is never in the delayed receipt queue

I don't think there is anything special about PromiseYield/PromiseResume in this regard. They can end up in the delayed receipts queue if they arrive as an incoming receipt and the compute limit is reached.

[jancionear]

It makes sense to me that congestion control ignores PromiseYields as they're sir receipts and we have infinite gas limit for receipts sent to the same shard. We can't reduce the number of receipts on this shard by buffering them, so it makes sense to just let them through 👍

AFAIK sir receipts still go through the outgoing buffer. I remember that I had to consider receipts sent to the same shard when implementing the outgoing size limit and there were always some receipts in outgoing buffer to the same shard.

Congestion control wouldn't buffer sir receipts to the same shard, as the gas limit is infinite, but they might get buffered by the outgoing size limit.

IMO the PR makes sense 👍

I'd just remove the comment saying which only counts gas in the outgoing buffers and delayed receipts queue.

Sorry for late review, I missed the PR :/

[wacban]

Thanks for the context @jancionear! I think anything that is stored in buffers / queues should be accounted for, regardless of being local or x-shard. Let me see if there is an easy way to calculate the gas for a promise yield and solve this problem this way.

For the time being I would like to add a TODO and merge this PR in order to at least get rid of the annoying error. Please let me know if anyone is against it otherwise I'll merge tomorrow.

[jakmeier]

I thought PromiseYield is never in the delayed receipt queue

I don't think there is anything special about PromiseYield/PromiseResume in this regard. They can end up in the delayed receipts queue if they arrive as an incoming receipt and the compute limit is reached.

Hm, I thought only PromiseResume can be incoming, while PromiseYield is always a local receipt.

Congestion control wouldn't buffer sir receipts to the same shard, as the gas limit is infinite, but they might get buffered by the outgoing size limit.

Right, I think that's what I missed! Makes sense then that we see it in the buffer.

Thanks for the context @jancionear! I think anything that is stored in buffers / queues should be accounted for, regardless of being local or x-shard. Let me see if there is an easy way to calculate the gas for a promise yield and solve this problem this way.

For the time being I would like to add a TODO and merge this PR in order to at least get rid of the annoying error. Please let me know if anyone is against it otherwise I'll merge tomorrow.

Fully agree, I will approve to unblock.