lnwallet: perform mempool acceptance check before publishing#8345
Conversation
yyforyongyu
force-pushed
the
use-testmempoolaccept
branch
2 times, most recently
from
085de24 to
c93ce9d
Compare
|
weird the CI isn't running |
yyforyongyu
force-pushed
the
use-testmempoolaccept
branch
2 times, most recently
from
e7283c2 to
7c84e38
Compare
yyforyongyu
force-pushed
the
use-testmempoolaccept
branch
2 times, most recently
from
a531def to
25831f8
Compare
yyforyongyu
force-pushed
the
use-testmempoolaccept
branch
2 times, most recently
from
7791dbc to
f4b782c
Compare
ProofOfKeags
left a comment
ProofOfKeags
left a comment
There was a problem hiding this comment.
Looks decent to me. I think there are opportunities for cleaning up error translation responsibility so that we can keep the error boundaries clean but afaict those issues precede this PR and don't need to be fixed here.
yyforyongyu
force-pushed
the
use-testmempoolaccept
branch
from
f4b782c to
58d4f6e
Compare
|
Important Auto Review SkippedAuto reviews are limited to the following labels: llm-review. Please add one of these labels to enable auto reviews. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe updates involve enhancing transaction handling in a Bitcoin Lightning Network Daemon (lnd). A new method for testing mempool acceptance ( Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
yyforyongyu
force-pushed
the
use-testmempoolaccept
branch
from
58d4f6e to
020daf5
Compare
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sum
Files selected for processing (5)
- chainreg/no_chain_backend.go (2 hunks)
- contractcourt/chain_arbitrator.go (2 hunks)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- lnutils/errors.go (1 hunks)
- lnwallet/btcwallet/btcwallet.go (2 hunks)
Additional comments: 7
lnutils/errors.go (1)
- 5-21: The implementation of
ErrorAsis correct and follows best practices by simplifying error handling with generics. This is a good use of Go's type parameters feature.chainreg/no_chain_backend.go (1)
- 217-221: The
TestMempoolAcceptmethod is correctly added as a stub to theNoChainSourcetype. Ensure that it is documented as a placeholder implementation since it returnsnilfor both the result and error.docs/release-notes/release-notes-0.18.0.md (1)
- 110-115: The release notes have been updated correctly to describe the new mempool acceptance check feature for transactions in
lnd. The explanation is clear and includes a link to the relevant PR.lnwallet/btcwallet/btcwallet.go (4)
- 1204-1206: The
handleErrclosure is introduced to handle errors fromPublishTransactionandTestMempoolAccept. This is a good use of a closure to avoid code duplication.- 1232-1238: The early return for the "neutrino" backend in
PublishTransactionis a good separation of logic for different backends.- 1240-1245: The use of
TestMempoolAcceptwith a max feerate of 0 to use the default value is a good implementation detail that aligns with the PR's objective to check mempool acceptance before broadcasting.- 1250-1254: The sanity check for the length of
resultsfromTestMempoolAcceptensures that the expected number of results is returned, which is a good defensive programming practice.
| case lnutils.ErrorAs[*base.ErrInMempool](err), | ||
| lnutils.ErrorAs[*base.ErrAlreadyConfirmed](err): | ||
|
|
||
| err := b.wallet.PublishTransaction(tx, label) | ||
|
|
||
| return handleErr(err) |
There was a problem hiding this comment.
The switch case that handles ErrInMempool and ErrAlreadyConfirmed by re-publishing the transaction to mark the label in the wallet is a temporary workaround. This is not ideal as it may lead to redundant network traffic and should be refactored once the rebroadcaster is implemented.
Refactor this logic to avoid broadcasting the transaction twice once the new rebroadcaster is in place.
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
Latest changes looking pretty good. I think we can cherry-pick this commit from my old PR as well to get us some extra coverage: 81b132f#diff-dce12550c3c3d6c8e301e5fd1341b398c7a0f4680f752acabeefb42a5a1f61d5R2036
Might need to be modified slightly tho for this new version
This commit adds a mempool acceptance check before broadcasting a given transaction. To maintain the current behavior from `BtcWallet.PublishTransaction`, the two errors, `ErrInMempool` and `ErrAlreadyConfirmed` returned from `TestMempoolAccept` are ignored.
Previously, `PublishTransaction` in `btcwallet` will first mark the tx label in db first before broadcasting it to the network. Even though the broadcast may fail with the error already in mempool or already confirmed, this tx label updating is still performed. To maintain the old behavior, we now catch the errors from `TestMempoolAccept`, and make sure to call the `PublishTransaction` to mark the tx label even there are errors from mempool acceptance check.
We should not fail to start the node if a republish attempt failed for a channel's closing tx. Instead, we log an error to continue the startup and let other channels continue their operations.
bitcoind v25.0 updated the `sendrawtransaction` RPC to have an optional argument `maxburnamount` with a default value of 0. This means our existing test that uses burning output cannot be published, hence, we remove the usage of it in our tests and replace it with a normal tx.
yyforyongyu
force-pushed
the
use-testmempoolaccept
branch
from
020daf5 to
783e914
Compare
yyforyongyu
force-pushed
the
use-testmempoolaccept
branch
from
d71d3cf to
051328b
Compare
yyforyongyu
force-pushed
the
use-testmempoolaccept
branch
from
051328b to
a614888
Compare
4 participants
This PR adds a mempool acceptance check before broadcasting a given transaction. To maintain the current behavior from
BtcWallet.PublishTransaction, the two errors,ErrInMempoolandErrAlreadyConfirmedreturned fromTestMempoolAcceptare ignored.Note that we choose to implement this method as an interface method on
chain.Interfaceinbtcwalletas we want to have thisTestMempoolAcceptbeing used as an independent check and can be used in other cases where we don't wanna publish transactions. However, we could implement this check inbtcwalletinstead inPublishTransactionhere,https://github.com/btcsuite/btcwallet/blob/5df09dd4335865dde2a9a6d94a26a7f5779af825/wallet/wallet.go#L3657-L3659
since every tx must pass this mempool acceptance check before it can be broadcast. This alternative approach, tho, do have a problem, that we need to pass the max feerate used by
testmempoolaccepttoPublishTransactioninbtcwallet, which seems to be a larger change.Depends on:
testmempoolacceptfor bothbitcoindandbtcdbtcsuite/btcd#2053TestMempoolAcceptto chain interface btcsuite/btcwallet#899Fixes #4760
Summary by CodeRabbit
New Features
Improvements
Documentation
Refactor