-
Notifications
You must be signed in to change notification settings - Fork 66
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
Improves reorg logic by checking whether we are on sync with the backend or not #235
base: master
Are you sure you want to change the base?
Conversation
This would need further testing. Right now it has just patched the test suite so All in all, better test coverage for reorgs is needed. |
…end or not The current reorg logic does not really take into account whether we are on sync with the backend or not. On a first block connected after a reorg, it will try to send all reorged out data assuming it has knowledge of everything that is already confirmed or in the mempool. However, in a multi block reorg the backend could be at a height that the tower has not processed yet, hence some transactions may be on the chain but not on our internal `txindex`. Therefore, it could be the case that we try to re-send something that has been reorged-out and see it bounce. Under normal conditions, that would mean the transaction was confirmed a long time ago, since otherwise it would be in our index. However, in this case it may be that it is just confirmed in a subsequent block we haven't processed yet. This will lead to wrongly assuming the tracker was `IRREVOCABLY RESOLVED`, while in reality it may only have a few confirmations. This patch fixes that. In the case of a transaction bouncing we will check whether we are on sync with the backend, and only if so consider the tracker as `IRREVOCABLY RESOLVED`. Otherwise, the tracker will be flagged as `OffSync` and retried until it bounces when we are on sync, or its status is updated on block processing. For context, this edge case was introduced when adding support for prune mode. Before that (when `txindex` for the backend was required) we would have used `getrawtransaction` to check the confirmation count of the bouncing transaction.
I guess it is worth mentioning what the flow for an
A tracker can be updated to
|
This should be squashed. I just realized about this could actually be simplified. Leaving like this for an easier review.
The current reorg logic does not really take into account whether we are on sync with the backend or not. On a first block connected after a reorg, it will try to send all reorged out data assuming it has knowledge of everything that is already confirmed or in the mempool. However, in a multi block reorg the backend could be at a height that the tower has not processed yet, hence some transactions may be on the chain but not on our internal
txindex
. Therefore, it could be the case that we try to re-send something that has been reorged-out and see it bounce. Under normal conditions, that would mean the transaction was confirmed a long time ago, since otherwise it would be in our index. However, in this case it may be that it is just confirmed in a subsequent block we haven't processed yet. This will lead to wrongly assuming the tracker wasIRREVOCABLY RESOLVED
, while in reality it may only have a few confirmations.This patch fixes that. In the case of a transaction bouncing we will check whether we are on sync with the backend, and only if so consider the tracker as
IRREVOCABLY RESOLVED
. Otherwise, the tracker will be flagged asOffSync
and retried until it bounces when we are on sync, or its status is updated on block processing.For context, this edge case was introduced when adding support for prune mode. Before that (when
txindex
for the backend was required) we would have usedgetrawtransaction
to check the confirmation count of the bouncing transaction.I realized about this when reviewing #190, wrongly believing that this edge case was being introduced by the changes there.