search

lightninglabs / neutrino

Privacy-Preserving Bitcoin Light Client
MIT License
894 stars 182 forks source link

pushtx: map different backend err to internal err. #278

Closed ziggie1984 closed 1 year ago

ziggie1984 commented 1 year ago

Relates to lightningnetwork/lnd#7156.

Looking for a Concept ACK here.

This makes sure our rebroadcaster continues broadcasting transactions when they are for example rejected because the min mempool fee is not met. It becomes a bit messy to now include some other backend conversions into the pushtx package which is mainly a neutrino package? So the most cleanest way would be to maybe reimplement the rebroadcaster for the different backends? This solution gets the job done, so open for suggestions.

Roasbeef commented 1 year ago

It becomes a bit messy to now include some other backend conversions into the pushtx package which is mainly a neutrino package? So the most cleanest way would be to maybe reimplement the rebroadcaster for the different backends?

Agree would be ideal to actually move this entire package into btcwallet itself. There's another path where we make a small cut out here for custom error detection, then hook that up to the existing function (basically the switch case we'd copied over here) in btcwallet when we create the rebroadcaster in lnd. At least that way we are able to re-use code, and keep all the error parsing/interpretation in a single place.

lightninglabs-deploy commented 1 year ago

@roasbeef: review reminder @bitromortac: review reminder @ziggie1984, remember to re-request review from reviewers when ready

ziggie1984 commented 1 year ago

My intention was to leave the neutrino code and therefore the Error Definition mostly untouched because it's still taylored for the neutrino backend and there this kind of description could be maybe confusing? But happy to include your comment after your final thought.

bitromortac commented 1 year ago

My intention was to leave the neutrino code and therefore the Error Definition mostly untouched because it's still taylored for the neutrino backend and there this kind of description could be maybe confusing? But happy to include your comment after your final thought.

Yes, good argument, then let's keep it like it is. It was confusing me (in the context of the lnd PR) because I thought it would not include the case of min mempool fee (but it does), so just wanted to mention it here.