[RFC]: make dev-rescan-outputs safe to use #7296
Conversation
ee449ad
to
6e2ab02
Compare
6e2ab02
to
15b0f16
Compare
This PR should be title "Never assume that some shit works". Thanks for @niftynei that wake up me and said "Hey this is a DB shit", and thanks to @rustyrussell to waste 20 minutes debugging my problem. But now, back to the patch description ---- Working to fix the following issue ``` error: {\\\"code\\\":-25,\\\"message\\\":\\\"bad-txns-inputs-missingorspent\\\"}\") }) 2024-03-25T14:05:44.178Z DEBUG lightningd: Expected error broadcasting tx 020000000001018c9e9f69341019c959b5526231e2bd52bd1e7227a3b72fa40e095ed283b7ddbb0000000000cae9f0800199c20000000000002200203251c1d74d76a6a487afca13913bc09fb3cc766e12efe938c049a1c432316bcf040047304402201a96b84a9e234172ade0f055be867097ac480c2d21f8163f449bde67266dda15022060fe38faed61d573db32ab5d48a3a9eff1f9e27f7adc51e86cb7b22878aad66a014730440220764c985c44e8ae0e303c46588090dea100c997b42ba9bdec7fa66bc68f5cc1fe02205200f62950b4ac4d8a297a97bb30fdc92edd43f5fccaf35b31a46e6963f42f4d0147522102b7c0c24f0a4ed6880289c17132f63c7a9ace2db1dfbc0baaf03a19916cfc867e210382323ae01328ab397c250a3120567531e76970a8c88795cac03e488dafe81b4252ae8ea0b720: [400] Unknown error (sendrawtransaction RPC error: {\"code\":-25,\"message\":\"bad-txns-inputs-missingorspent\"}) ``` The issue is that my database is incorrectly marking outputs as AVAILABLE when they have actually been spent. Therefore, we should verify the spend height and appropriately label them as spent. This change will help ensure that we correctly identify the status of each UTXO. Is it certain that the UTXO is spent? Is the current status incorrect? There might also be a chance that the status is correct. To confirm, we should double-check against an external explorer. To avoid adding an extra bitcoin core call just for this, I am adding a new dangerous dev method that will help to fix this problem, and I implemented the recovery strategy in folgore [1] This PR adds the necessary RPC methods to fix the issue. [1] coffee-tools/folgore@6215861 Link: ElementsProject#7172 Co-Developed-by: Rusty Russell <rusty@rustcorp.com.au> Co-Developer-by: @niftynei Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
15b0f16
to
f3d75a4
Compare
There was a problem hiding this comment.
NACK
I like the use of a dedicated function for the state transition, and I think we should assert() that the state and spendheight are kept in sync, but I do not want to expose this to the end-users, papering over the issue rather than addressing the underlying issue.
| #include <stdbool.h> | ||
| #include <stdlib.h> |
There was a problem hiding this comment.
These were auto-added, and are useless.
|
|
||
| /* FIXME(vincenzopalazzo): There are different nasty case at this point that are | ||
| * | ||
| * - moving from OUTPUT_STATE_SPENT -> OUTPUT_STATE_CONFIRMENT required | ||
| * to set the spendheight to null | ||
| * - moving from OUTPUT_STATE_CONFIRMED -> OUTPUT_STATE_SPENT required | ||
| * to set the spendheight. | ||
| * | ||
| * in both bases the following code do not do that. */ |
There was a problem hiding this comment.
Shall we encapsulate the status update logic into a function (like you did further down), and then force all updates to go through that logic? It could even have an assert that ensures the enum -> spendheight? mapping, enforcing that marking as spent sets, and marking as unspent unsets.
There was a problem hiding this comment.
How we can make the transition to a spending state if we do not have an API that has the spendheight information?
| /* This is a dangerus command, but looks like that the dev rescan output is not covering | ||
| * a use case, where we have the output is marked as confirment but we want to move it to | ||
| * a spend confition. This usually do not happens but with dev rescan it can (still unclear how). | ||
| * | ||
| * So we need to interact with an external plugin in order to inject the corrent spend height | ||
| * inside the output. | ||
| * | ||
| * It can work is I rescan the blockchain? */ |
There was a problem hiding this comment.
Let's not expose this. It is here to address a transient error in our data model, that can trivially be resolved by dev-rescan-outputs as soon as it is fixed. Papering over is bad, as it teaches users to "just run this" and not report a real issue anymore.
There was a problem hiding this comment.
Well this is the only way that I found to fix the mess with my node, how I can fix the db problem with my node from dev-rescan-outputs?
Papering over is bad, as it teaches users to "just run this" and not report a real issue anymore.
We are already saying this with dev-rescan-outputs are we?
This PR should be title "Never assume that some shit works". Thanks for @niftynei that wake up me and said "Hey this is a DB issue", and thanks to @rustyrussell to waste 20 minutes debugging my problem.
But now, back to the patch description (see commits for full history)
Working to fix the following issue
The issue is that my database is incorrectly marking outputs as AVAILABLE when they have actually been spent. Therefore, we should verify the spend height and appropriately label them as spent. This change will help ensure that we correctly identify the status of each UTXO. Is it certain that the UTXO is spent? Is the current status incorrect? There might also be a chance that the status is correct. To confirm, we should double-check against an external explorer.
To avoid adding an extra bitcoin core call just for this, I am adding a new dangerous dev method that will help to fix this problem, and I implemented the recovery strategy in folgore [1]
This PR adds the necessary RPC methods to fix the issue.
Fixes: #7172
[1] coffee-tools/folgore@6215861
Co-Developed-by: Rusty Russell rusty@rustcorp.com.au
Co-Developer-by: @niftynei