Remove publishing last sweep tx logic during startup. #7879
Conversation
2041135 to
a1a2515
Compare
|
We might want to consider making the migration optional, so using the |
|
I also find the optional path maybe not that fitting because we will remove the complete logic of broadcasting logic. Maybe roasbeef was thinking of a return path here I guess. Meaning that making this change mandatory would not allow users to run a prior version of lnd when this migration lockes in (thinking of the big taproot channel change which will come with 17?). I think finishing the OptionalMigration code part seems also doable as well in a short enough time frame. So waiting for you final thoughts before going down this way. |
5ca0220 to
4b326f5
Compare
Mainly just trying to cut down on the number of migrations that need to be run for major updates if we're able to to do. Do we have a handle on how many keys we'd be deleting here for very large nodes? Given how active the sweeper can be at times, I think it's worth it to see what a dry run instance would look like, just so we can properly communicate to nodes how long a migration might take in practice. |
There was a problem hiding this comment.
Mainly just trying to cut down on the number of migrations that need to be run for major updates if we're able to to do. Do we have a handle on how many keys we'd be deleting here for very large nodes?
Yeah luckily it's just the sweeper-last-tx bucket, which is a root bucket that has one or zero item, thus performance-wise it'd be safe to migrate.
e9efab4 to
ffcb82c
Compare
We remove the publishing of the last published sweep tx during the startup of the sweeper. This republishing can lead to situations where funds of the default wallet might be locked for neutrino backend clients. Moreover all related tests are removed as well.
The new migration removes the sweeper-last-tx top level bucket from the channel.db database.
654488a to
e705899
Compare
e705899 to
160eab2
Compare
This PR fixes #7708.
We remove the logic where we would publish the last sweep transaction during the startup of the sweeper sub-system. The main problem relates to neutrino backends which did not recognize invalid (e.g. double spends) transactions leading to used up default wallet funds almost indefinitely.
Detailed reason why we decided to remove this logic can be found in the issue and will be provided here shortly after doing some tests. The reason for publishing the last sweep tx was that we want to avoid a situation where we create a lot of empty addresses in our wallet.db when the sweeper crashes in specific situations.
Why we decided to remove this logic:
First of all no funds are at risk here, our wallet will always create addresses we can sign for.
Remains the case where we fail(crash) during the creation of the sweep. We are explicitly taking about a crash we are not anticipating here (after we created the sweep address, during the publishing of the transaction). We handle all errors we currently know of, so an error in this stage is very unlikely. If that would happen nonetheless, the sweeper only fires in special circumstances (sweep input available && finding a new block && expiration of the sweeper timeout) so the noderunner will very likely be aware of the continuous crashing and fix the problem until a lot of address space is occupied.
Therefore we decided to remove this logic.
Currently this implementation makes the migration via a mandatory path, happy to change this if we find consensus how to move forward. Opening this PR for review now.
Moreover this fix will give noderunners currently experiencing stuck funds to resolve this issue, however a wallet-rescan might be need as well.
Steps to Test
added a unit test for migration: