Fix resync fee missing

[ytx1991]

Purpose:

Fix issue: https://github.com/Chia-Network/issue-tracker/issues/558

Current Behavior:

After resync, all transactions don't have a fee

New Behavior:

After resync, all outgoing transactions will have a fee.

Testing Notes:

Tested on testnet10, waiting for another PR merged to add unit test

[Quexington]

Is this change for a functional reason or is it just for the sake of user display on resync? If the latter, I think that this is probably a bad change: 1) it adds calls to our full node peer (who's already gracious enough) 2) Introduces potentially more flakiness into syncing coin states 3) we already have a user expectation that txs during resync will lose some information. It's not great but without a more scalable system, it's what we're stuck with.

I don't mind the change at the bottom though that seems to just be an extra calculation.

[ytx1991]

Is this change for a functional reason or is it just for the sake of user display on resync? If the latter, I think that this is probably a bad change: 1) it adds calls to our full node peer (who's already gracious enough) 2) Introduces potentially more flakiness into syncing coin states 3) we already have a user expectation that txs during resync will lose some information. It's not great but without a more scalable system, it's what we're stuck with.

I don't mind the change at the bottom though that seems to just be an extra calculation.

We only do an extra call for Clawback txs. For others it just a calculation like you said. If missing fee is acceptable for Clawback then I can remove it @esaung

[arvidn]

what does it mean to be "missing a fee"? my understanding is that "fee" refers to the payment left for the farmer. that fee is paid when the block is farmed. I don't see how the current behavior is that no transactions pay a fee.

[arvidn]

as someone not very familiar with this code, would you mind explaining what problem this fixes and under what circumstances it happens?

It looks weird. The existing code also looks weird, but this doesn't seem to address it. We add a TransactionRecord with fee hardcoded to 0, if it's not considered change. Why is that? What if you make a transaction with no change, but you pay a fee? That's a possible transaction that seems to be handled wrong (at least at a first glance of this code).

Secondly, after you compute the fee (which looks like a reasonable way to compute it) it's only used if we add another TransactionRecord, but we don't always do that. Are we just not recording the fee in that case? Or is that the case I mentioned in the previous paragraph that we just don't handle right?

[ytx1991]

For your questions,

1. After users nuked their wallet or sync on a different machine, the TX fee is 0 for all TXs. We want to recover this information.
2. The code section I changed is only for the resync case. It shouldn't be executed when users syncing normally.
3. My understanding is this fee is not in use previously and seems someone try to fix this issue but it just finished half of the fix. The change I made will calculate the fee for outgoing TX and set it instead of using 0.

[arvidn]

if we get to this case: https://github.com/Chia-Network/chia-blockchain/pull/15964/files#diff-a367f650aed3b60eba2c253ce15852630c1baf5e9891223d1557ac9cb3333d20L1565-L1568
we still ignore the fee you computed. It's not clear how this can be correct.

Also, if we get to this case, we don't even try to compute the fee: https://github.com/Chia-Network/chia-blockchain/pull/15964/files#diff-a367f650aed3b60eba2c253ce15852630c1baf5e9891223d1557ac9cb3333d20R1598-R1613

[ytx1991]

For the first case, it is for reorg only. For a normal resync, there shouldn't be any existing TX. And for the reorg, we already saved the fee in the DB. It just needs to update the confirmation height.

For the second case, it is for the normal syncing, not the resync. In this case, TX record is already saved in the DB with the fee, we just need to mark it as spent.

[arvidn]

one more question:

After resync, all outgoing transactions will have a fee.

Did you confirm that the fee you compute is always correct?

You compute the fee per coin, afaict. This means if you spend two coins, but only one of them creates the output coins. That coin can end up with a negative fee, which would throw an exception since you cast it to uint64. While the other coin would end up being considered 100% fee. I'm not sure it's better to "guess" some values for these, instead of indicating to the user that we just don't know.

[github-actions]

This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties.