fix: provider side deposit cache

[shaspitz]

PR into #746 addressing #746 (comment)

[chrmatt]

I don't quite understand the logic. Maybe worth commenting somehow how this works. What I think I understand: When a provider commits to a bid, a pending refund for the bid amount is created, linked to the commitment digest. When some commitments get opened, commitments not matching the block winner get their refund applied. When an opened commitment is stored, the refund gets dropped.

What I don't understand: Why are the two cases handled in different functions (open vs store opened)? It seems opening (some?) commitment is required to trigger checking (all?) commitments for different block winners. What if another provider won and doesn't open anything (e.g., because they haven't committed). Will the refunds still be applied? Why not do these checks (both apply and drop) when a winner is announced?

Note that these will interfere with the issue we discussed in some call recently: Winners are announced directly, but oracle waits 10 blocks to be reorg resilient. If reorg happens, "wrong" winner will be announced and also the refund logic here will be applied incorrectly.

[shaspitz]

I don't quite understand the logic. Maybe worth commenting somehow how this works. What I think I understand: When a provider commits to a bid, a pending refund for the bid amount is created, linked to the commitment digest. When some commitments get opened, commitments not matching the block winner get their refund applied. When an opened commitment is stored, the refund gets dropped.

What I don't understand: Why are the two cases handled in different functions (open vs store opened)? It seems opening (some?) commitment is required to trigger checking (all?) commitments for different block winners. What if another provider won and doesn't open anything (e.g., because they haven't committed). Will the refunds still be applied? Why not do these checks (both apply and drop) when a winner is announced?

Note that these will interfere with the issue we discussed in some call recently: Winners are announced directly, but oracle waits 10 blocks to be reorg resilient. If reorg happens, "wrong" winner will be announced and also the refund logic here will be applied incorrectly.

This sounds correct. Specifically, every commitment from a provider gets a pendingRefund stored, where the amount is the bid amount. Every pending refund should (eventually) either get applied or dropped.

Every commitment (relevant to the provider running the node) gets seen by the openCommitments function via the for-loop in that function. For every commitment where providerAddress != newL1Block.Winner, the pending refund is applied, since a different provider has won the block. Meaning we should re-credit our cached knowledge of bidder deposit specific to our provider and the relevant bidder.

In the alternative case that providerAddress is same as newL1Block.Winner for a commitment, the openCommitments function continues, the commitment is opened on-chain. Once that event is seen by handleOpenedCommitmentStored, if everything is successful, the refund is deleted. Note only commitments sent by one's own provider are tracked in handleOpenedCommitmentStored.

An event landing on-chain is the first definitive signal that one's own provider has won, and the refund can be dropped, so that's why it's applied there. If providerAddress == newL1Block.Winner AND your own node doesn't open the commitment, your node is likely acting incorrectly elsewhere anyways.

That being said, I don't see an issue with having the drop/apply logic both happen within openCommitments.

Yes this issue is related to the winner announcement/reorg issue we discussed previously.

[chrmatt]

Every commitment (relevant to the provider running the node) gets seen by the openCommitments function via the for-loop in that function. For every commitment where providerAddress != newL1Block.Winner, the pending refund is applied, since a different provider has won the block. Meaning we should re-credit our cached knowledge of bidder deposit specific to our provider and the relevant bidder.

But the openCommitments function is only called if any commitment gets opened, I assume? So what if provider A issues commitments for block N, but provider B does not issue any commitments. Now provider A has some pending refunds. Assume provider B wins the block. Then, provider B does not open any commitments. In that case, the openCommitments function would not be called and nothing would be refunded for provider A (even though all commitments for that block could/should be refunded).

[shaspitz]

But the openCommitments function is only called if any commitment gets opened, I assume?

No, openCommitments is triggered by NewL1Block events being emitted onchain. So the openCommitments will run so long as new winners are announced by the oracle

[chrmatt]

But the openCommitments function is only called if any commitment gets opened, I assume?

No, openCommitments is triggered by NewL1Block events being emitted onchain. So the openCommitments will run so long as new winners are announced by the oracle

Oh, I see. Then it should be all good!

[aloknerurkar]

This entire refund logic should be handled by tracker. There is no point adding this additional dependency to the preconfirmation protocol directly. The tracker can add the pending refund during the TrackCommitment call itself.

Also how do we handle refunds related to decayed amount? I think we should handle the refunds during the FundsUnlocked and FundsRetrieved events as well. We should refund the decayed amount.

[shaspitz]

Re first point, sound good. Re second point, see first paragraph of #746 (comment)

[shaspitz]

Per request, deduction no longer happens here and now happens in tracker.go

[aloknerurkar]

Again this looked like a good idea but now I think about it, there is scope for double spending if we dont deduct immediately.

Also, I would prefer the tracker logic to be independent of this deposit manager. The tracker runs for both bidders and providers, so having this provider specific logic is not looking right. Strictly from a design perspective.

What I would propose is that we keep this CheckAndDeduct functionality as is. Also the Refund on error part as is. What we can do is pass in the Notifications implementation to DepositManager and subscribe to notifications from the tracker. The tracker already emits CommitmentFailed event which can be used to refund in deposit manager code itself.

We can create a new event which is fired while openingCommitments that we can look at in DepositManager to refund for the case of not winning.

This way the deposit manager functionality is independent of the tracker.

[aloknerurkar]

Again this looked like a good idea but now I think about it, there is scope for double spending if we dont deduct immediately.

Also, I would prefer the tracker logic to be independent of this deposit manager. The tracker runs for both bidders and providers, so having this provider specific logic is not looking right. Strictly from a design perspective.

What I would propose is that we keep this CheckAndDeduct functionality as is. Also the Refund on error part as is. What we can do is pass in the Notifications implementation to DepositManager and subscribe to notifications from the tracker. The tracker already emits CommitmentFailed event which can be used to refund in deposit manager code itself.

We can create a new event which is fired while openingCommitments that we can look at in DepositManager to refund for the case of not winning.

This way the deposit manager functionality is independent of the tracker.

[aloknerurkar]

I am approving it. Have some minor comments, so you can address them and merge.