Sia icon indicating copy to clipboard operation
Sia copied to clipboard

WIP: Fix host counter

Open nielscastien opened this issue 6 years ago • 2 comments

@DavidVorick This is a first attempt to fix the host financial metrics that are off most of the time for all the hosts.

I noticed that whenever my host financial metrics increased a lot in a small period of time, there were at the same time consensus errors in the host.log. These errors always repeated themselves 6 times with 15s interval. See host.log.

Therefore, I looked at negotiate.go and found out that there is a loop that calls err = h.managedAddStorageObligation(so) . Worst case this function is called 6 times if there are issues with the acceptance of the transaction set (leading to the logged errors). Every time this function is called on the same storage obligation, the host financial metrics are updated.

This PR tries to fix the double counting issue. This PR will not prevent additional insertions into the host database. These additional insertions could lead to several other bugs where a host can't provide a storage proof when a storage obligation ends.

I know the focus is on upgrading the renter. But this could potentially fix a lot of errors for the hosts. Any feedback is welcome...

cc @lukechampine

nielscastien avatar Mar 16 '18 19:03 nielscastien

This seems basically correct to me. Although, it makes me suspect that the host state isn't being properly rolled back in the event of an error. For example, there's a db.Update call earlier in the function; if managedAddStorageObligation ultimately fails, then we'd want to revert those changes, right? Perhaps that can be tackled in a subsequent PR.

lukechampine avatar Mar 16 '18 23:03 lukechampine

@lukechampine I reverted the old and added some new commits. Please take a look if I am not missing something. Whenever managedAddStorageObligation returns with an error, everything should be rolled back correctly now.

The only thing that still bothers me is the way a storage obligation with obligatonRejected status is removed within removeStorageObligation. In this PR I make use of it twice (to prevent duplication of code):

  1. after AcceptTransactionSet returns an error (this PR),
  2. or if there is an error with a queueActionItem (original code)

Before the financial metrics are updated it checks: h.financialMetrics.TransactionFeeExpenses.Cmp(so.TransactionFeesAdded) >= 0 but I don't know when this is true (or not). To be honest, I think this will only be true when the TransactionFeesAdded are 0. As far as I know, the TransactionFeeExpenses value of the host has a bug; it is always zero. Maybe some other hosts can confirm this.

If not, is it possible to define a new storage obligation status, obligationRollBack or something, so that we can make a dedicated branch in removeStorageObligation.

nielscastien avatar Mar 17 '18 13:03 nielscastien