-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix address stats when reorg-ed and confirmed in different height #12
Conversation
…different height.
(Discussing permanent solutions to myself:)
Actually, a lot of the history APIs rely on the height being there for ordering purposes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested @ 8ca766e
I confirmed that this fixes incorrect balances for the addresses identified in issue #11:
however bc1qm34lsc65zpw79lxes69zkqmk6ee3ewf0j77s3h still reports an incorrect on-chain balance (i.e. chain_stats.funded_txo_sum - chain_stats.spent_txo_sum
) of 30476.68434636
vs 30081.72818834
according to other explorers.
I wonder if we're still counting transactions which appeared in a stale block, but never actually confirmed in the main chain (and so wouldn't have a duplicate row)?
Actually, looking at that address (bc1qm34lsc65zpw79lxes69zkqmk6ee3ewf0j77s3h) I'm not sure anymore... It might be unrelated. |
Even fixing the key structure won't fix the orphaned but never re-confirmed case... I also just remembered that the height being in the index gives the history its order so we can't remove it from the key. Fixing this problem long-term will require an overhaul in the way Esplora indexes things. |
We are storing them in the DB, but the line The problem with the orphan-getting-re-confirmed-at-diff-height thing was that both of them would get the updated blockhash in TxConfRow, and therefore both return Some() for
Figuring out the cause of that would be hard to pin down and hard to verify any fix. lol |
ah ok, that makes sense, thanks. I couldn't find any other addresses with the same issue, so perhaps I just need to re-index. in any case, this PR does solve the original issue, so it's an ACK from me. |
There might be history entries where the txid ended up confirming at a different block height following a re-org. Before this fix, these entries would get counted twice, once with the initial re-orged confirmation height, then again with the final confirmation height. See mempool/electrs#12.
Thanks for hunting down the cause for this, @junderw :) A different way to fix this is to ensure that the block where the txid confirmed matches the height specified by the history entry. This is more efficient as it doesn't require accumulating an in-memory list of all |
There might be history entries where the txid ended up confirming at a different block height following a re-org. Before this fix, these entries would get counted twice, once with the initial re-orged confirmation height, then again with the final confirmation height. See mempool/electrs#12.
There might be history entries where the txid ended up confirming at a different block height following a re-org. Before this fix, these entries would get counted twice, once with the initial re-orged confirmation height, then again with the final confirmation height. See mempool/electrs#12.
There might be history entries where the txid ended up confirming at a different block height following a re-org. Before this fix, these entries would get counted twice, once with the initial re-orged confirmation height, then again with the final confirmation height. See mempool/electrs#12.
Fixes #11
Requires re-index? == No
Affects API interface AND/OR response data format? == No
Explanation:
TxHistoryRow
is just a key in rocksdb. Which means it is encoding all its information into the key and (most likely intentionally) using the property that inserting the same key twice will de-duplicate to its advantage.In the case where txA is confirmed in orphaned block at height 10, returned to our mempool, then confirmed in new-best-chain block 15 etc. The keys are not the same (since the key contains the block height).
Other endpoints use unique() on txid etc. so there shouldn't be any problems with the other endpoints... However, this can't be properly fixed without a re-working of the index (which will require a re-index of esplora)
For now, we avoid unique() since it would just give the same result, and instead use unique_by and return a tuple of the txid + the outpoint (for spends it's the previous outpoint, for funds it's this txid+this output index. If we don't include the txid(), then it would remove the spend of each fund->spend pair)
Note:
utxo_delta
does not have the same problem because it uses a HashMap with the Outpoint as the key to accumulate the results, removing any duplicates.