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

chain: match on new bitcoind v28.0 errors #948

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Sep 3, 2024

An error message was renamed in bitcoin/bitcoin#30212.

We should probably start matching on the reject-reason returned by testmempoolaccept RPC instead, as that's less likely to change in the future.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re using reject reason instead, we do use that, as we pass it in to this MapRpcErr function: https://github.com/lightningnetwork/lnd/blob/e8c5e7d5ce4b79117ef9ab497745fea3a1bffb36/lnwallet/btcwallet/btcwallet.go#L1280

@yyforyongyu
Copy link
Collaborator

I think this works, but like laolu said we already do the mapping over reject-reason. If we believe this field is less likely to be changed, what we can do instead is to always do a testmempoolaccept check in SendRawTransaction here since we can assume once it's the bitcoind backend this RPC must be supported.

@guggero
Copy link
Collaborator Author

guggero commented Sep 11, 2024

I was confused about this too... So I did some digging and we do match on reject-reason. BUT we have a special case where we still publish the transaction, even when testmempoolaccept returns false: https://github.com/lightningnetwork/lnd/blob/cc9e2b783ec983fbe3995d47d31898aa122b11c8/lnwallet/btcwallet/btcwallet.go#L1302

And in that case we then get the new error message, which we need to map as well.
So I think we still need this, cc @yyforyongyu @Roasbeef.

@guggero guggero requested a review from Roasbeef September 11, 2024 08:10
@yyforyongyu
Copy link
Collaborator

I was confused about this too... So I did some digging and we do match on reject-reason. BUT we have a special case where we still publish the transaction, even when testmempoolaccept returns false: https://github.com/lightningnetwork/lnd/blob/cc9e2b783ec983fbe3995d47d31898aa122b11c8/lnwallet/btcwallet/btcwallet.go#L1302

Yeah we do call publish tx even if there's an error in lnd, to put the tx label in the db...we may need to fix it there, meanwhile this should be good to go.

@Roasbeef
Copy link
Member

Ahh yeh, we call again just to insert it as a label...

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎻

@Roasbeef Roasbeef merged commit 738cd23 into btcsuite:master Sep 12, 2024
3 checks passed
@guggero guggero deleted the bitcoind-28-error branch September 13, 2024 06:28
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