pool: un-batch payouts

[contrem]

• Send payments one at a time

[jtgrassie]

Firstly thank you! And Welcome.

This is close to how I originally had this, one payment at a time. However, it's terribly inefficient and bad from a fee perspective. The ultimate fix is to use transfer_split as Monero intended (multiple destinations and let the wallet RPC split accordingly), then on any failures, act accordingly. The first problem problem I faced (wayback when I last worked on this bit) was that I couldn't seem to detect which payments from the batch failed or not. Hence I implemented the pool side so it would still update the users balance and record the payment, which at least allows for auditing against the wallet (i.e. call the wallet RPC get_transfers) and use the inspect util (in this project) to compare - then make any unpaid payments. This can be manual or automated but it ensures the pool doesn't payout more than it should (which would be irrecoverable obviously).

So currently it goes like:

1. make a payment with destinations [a, b, c, d]
2. on RPC failure (e.g. status 500), do nothing, try again later
3. on HTTP OK, regardless of any transfer failure (as we can't get the detail we need), decrement balances for [a, b, c ,d] and add payment records.
4. Periodically audit to ensure the wallet has actually made the payments the pool says it should have.

With this patch, it goes like:

1. make separate payments to [a, b, c, d]
2. (same as 2 above), for each
3. on HTTP OK but with error, don't update balance or add payment record
4. on HTTP OK and no error, update balance and add payment record.

Which brings me to the next problem: there are rare occasions the wallet RPC will have an error object (point 3) but still have made the payment! In this case, the pool will try and make another payment on next payout run, because the users balance has not been decremented. You're now out of pocket and some miner is lucky happy.

This is the long winded way of saying: it is how it currently is because I need it to be "safe" until I dig into the wallet RPC to see what's up with that. It may even be fixed since I last dug into this. (or I could have of course overlooked something), either way, erring on the side of caution to allow manual intervention/auditing seems the way to go until thoroughly sure it's trustworthy.

[jtgrassie]

Can you PR this separately please as this can be applied without changing the RPC/payout semantics.

[contrem]

sure

[contrem]

Thanks for the explanation. I missed the inspect tool, and was erring on the side of the miners not losing their balance. The error on success is also surprising, but I had a suspicion there was a reason it was like this.

It would be nice if the wallet indicated which parts had failed in the split. This was the only way I found to distinguish the failures, even though fee-wise it's not optimal (I also haven't looked at the wallet rpc stuff in a while, maybe it has changed). The audit solution is reasonable.

[jtgrassie]

The error on success is also surprising, but I had a suspicion there was a reason it was like this.

Indeed. I've not properly dug into it yet, could be a timeout, could be something else. Needs more investigation in any case. Pretty sure the timeouts need looking at - there are two evhttp timeout settings available, one for the connection one for the request, I'm only using the request one currently and they probably both need setting and extending to something like 60s to be safe. So two wallet timeout config settings...

It would be nice if the wallet indicated which parts had failed in the split.

Oh for sure. It may even do that now, it's been a while since I last had my head in that code too.

even though fee-wise it's not optimal

Agreed, and for the record, I'm not against sending separately If that turns out more robust in light of everything else discussed, i.e. so long as we can ensure no double payments, fine to go that route.

[jtgrassie]

Oh and I forgot to mention, this would require a DB migration on the payments DB as it changes the size of the structure.

EDIT: as ADDRESS_MAX is larger than any address by a good margin, you could use it's last byte for the transient error flag. Then it doesn't necessitate a DB migration.

[jtgrassie]

To be clear, if you want to keep this, you have to reduce the size of the address field. Otherwise it will break existing databases.

[contrem]

For a different project, I have payouts implemented as follows (high level):

Place hold for payment amount on the account

Within a DB transaction:

1. mark payment as submitted
2. submit the transfer rpc with relay false for an unsigned tx (eliminates submission errors caused by transaction creation before submission, fail fast)
3. sign the tx
4. submit the signed tx
   If any of the steps fail / timeout, the transaction is rolled back - balance is unaffected. Steps 2-4 can be simplified into one transfer call if separate signing is not needed

Store the txn hashes for each transfer along with its target address, height, amount, etc (this is done outside of the first DB transaction so that a db failure to insert doesn't rollback a submitted payment)

Periodically check the wallet for outgoing transfers:

• update stored transfer's confirmations
• missing (txn hash in DB, not in wallet) transfers are marked as lost payments (re-org, never made it into block, etc)
  • balance hold remains in place for manual checking
• unknown (txn hash in wallet, not in DB) transfers are logged for review (DB failed to insert the txn hash above, or worse, wallet was compromised and a payout was sent outside of this system)
  • in db insert failure case, balance hold remains in place for manual checking

Within a DB transaction when the transfer's confirmations meet the confirmation requirement:

1. reduce the balance by the hold amount
2. remove the hold

It also does single payouts because the users pay the fee from their balance.

Edit: Fix mistake in description: hold comes before first DB transaction
Edit: Note that separate signing is not a requirement

[jtgrassie]

Ultimately this patch, as its stands, introduces a subtle bug which had already been accounted for, hence why it cannot/shouldn't be merged. With both transfer and transfer_split in the Monero wallet RPC (currently), there is the possibility of sending funds and returning an error. The users pool balance thus has to be deducted to prevent subsequent payout runs erroneously paying out again. As things are is thus the safest approach (as well as being efficient due to batched txs).

Auditing to ensure all expected payments have actually been made is already possible via the inspect-data tool and the Monero wallet (cli / rpc). It's trivial to write a simple script which runs daily to catch any unsuccessful payments and alert, whereas it would be far from trivial to ask miners to pay back pool ops for duplicate payouts.

[contrem]

Ultimately this patch, as its stands, introduces a subtle bug which had already been accounted for, hence why it cannot/shouldn't be merged. With both transfer and transfer_split in the Monero wallet RPC (currently), there is the possibility of sending funds and returning an error. The users pool balance thus has to be deducted to prevent subsequent payout runs erroneously paying out again. As things are is thus the safest approach (as well as being efficient due to batched txs).

Agreed. I wanted to have a description of something I hope can be done here eventually too, by either myself or some other interested party. Apologies if it seemed like I was trying to force push it as is, which is not my intention.

[jtgrassie]

No apologies needed. Always good to discuss.

Personally I don't think a more convoluted approach fixes the perceived problem though. The crux of the problem is a situation where the Monero wallet RPC fails to send a payment (partial or otherwise). This is of course a situation we have to handle, for which we already do insofar as it's left in a recoverable state for manual/automated auditing/recovery.

I do think it's worth taking some time to go back to the monero wallet rpc code and see if the specific issue of partial split failures can be reported better of course. It's on my list. Ultimately though the pool error log is our friend (which can be used to alert a pool op action needs taking), and the fact we keep things in a recoverable state.

Doing payments un-batched may have merit though. In fact a configuration option would be ideal. If one runs a large private pool, there is no need for batched payments as all the miners pay to the same address (or only a small amount of addresses). If one runs a large public pool batching is certainly favorable.

[contrem]

Personally I don't think a more convoluted approach fixes the perceived problem though.

I think the procedure described does fix the problem since it only makes balance updates based on actual confirmed transactions. It won't double pay since the hold is in place, and it won't deduct if a transfer isn't confirmed on the chain. Thinking about it more, it's pretty much the same as what you are doing now by letting error'ed payments get deducted, and then auditing against the transfers.

You're right though, it is a more involved implementation that might be out of scope for this project since you already have a way to handle the problem.

I'll change this pull to have the same error handling when time permits.

I do think it's worth taking some time to go back to the monero wallet rpc code and see if the specific issue of partial split failures can be reported better of course. It's on my list.

Same here.

Doing payments un-batched may have merit though. In fact a configuration option would be ideal. If one runs a large private pool, there is no need for batched payments as all the miners pay to the same address (or only a small amount of addresses). If one runs a large public pool batching is certainly favorable.

True. It also prevents one payment from blocking another (for example, currently not enough money to send both payments in one go, but enough to send one payment), and lets you more easily associate the transfer fee with an account (if you want miners to pay the fee).

[jtgrassie]

If there's no payment there is nothing to do. I.e. it would be an error getting into this function without a payment. Thus if you're going to check for its presence, you may as well log an error and get out early, before even opening balance/payment transactions.