Sia
Sia copied to clipboard
WIP: Fix host counter
@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
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 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):
- after
AcceptTransactionSet
returns an error (this PR), - 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
.