Skip to content
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

boltz: Validate initial claim destination address #246

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

s373nZ
Copy link

@s373nZ s373nZ commented Oct 21, 2024

Tries to prevent the following error in the logs:

Unhandled exception in concurrent task!
lightningd[181525]: Bitcoin::UnknownAddrType

In my case, the database table BoltzServiceFactory_rsub has four entries, all of which contain empty strings for the destinationAddress field. These blank fields are predictably un-parsable as Bitcoin addresses.

Without the validation, the empty addresses get passed in to building an initial claim tx here and an error is thrown here.

I have next to 0 experience with clboss code and am no C++ expert by any means. Submitting this as a PoC / draft PR for a quick glance and if it's on the right track, I'll attempt to finish the address parsing validation.

@s373nZ
Copy link
Author

s373nZ commented Oct 25, 2024

After a bit more research on the code and my DB state, I see there is a more robust system around clboss creating the swaps. Particularly, I've been trying to understand more about the SwapManager. To note, and possibly related to this issue are the facts that:

  • SELECT * FROM SwapManager; return one row, with state=2 (enum AwaitingResult), but has no value initialized for the address column.
  • SELECT * FROM SwapManager_addrcache; returns a total count of 95 rows, all except one have no values for the address field.

So... perhaps there is also an issue with address generation prior to the validation crash, as well.

@s373nZ
Copy link
Author

s373nZ commented Oct 25, 2024

Changed the query to exclude loading empty addresses from the address cache. An additional idea might be to run a quick SQL query to clean up the address cache on plugin startup by deleting records with empty address fields.

Need to revisit whether the initial validation changes are still appropriate.

@s373nZ s373nZ force-pushed the validate-boltz-init-claim-destination-address branch from 1cf7746 to 58f96fc Compare October 25, 2024 13:14
@s373nZ
Copy link
Author

s373nZ commented Oct 25, 2024

Added an additional sanity check to not insert blank addresses from failed swaps into the address cache.

@s373nZ s373nZ force-pushed the validate-boltz-init-claim-destination-address branch from fba0bc3 to b267ef8 Compare October 25, 2024 13:41
@s373nZ
Copy link
Author

s373nZ commented Oct 25, 2024

Also added an SQL query run when the SwapManager is initialized which would remove any erroneously created records in the address cache which contain blank addresses.

Haven't tested this PR yet, as I only have a prod node right now, but taking this PR out of draft to solicit initial feedback.

@s373nZ s373nZ marked this pull request as ready for review October 25, 2024 13:45
@ksedgwic
Copy link
Collaborator

Chris Guida recommends checking whether CLBOSS needs any P2TR support.

@chrisguida
Copy link
Contributor

@ksedgwic I don't think that's the cause here as clboss is calling newaddr without params (which returns bc1q by default) and explicitly selects the bech32 key from the result.

But yes, generally it it probably good for us to make sure clboss is cool with taproot addresses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants