[feature]: BumpFee: return raw tx hex on success

[mohamedawnallah]

Change Description

Fixes #8470

Steps to Test

Steps for reviewers to follow to test the change.

Pull Request Checklist

Testing

• [ ] Your PR passes all CI checks.
• [ ] Tests covering the positive and negative (error paths) are included.
• [ ] Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• [ ] The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• [ ] Commits follow the Ideal Git Commit Structure.
• [ ] Any new logging statements use an appropriate subsystem and logging level.
• [ ] Any new lncli commands have appropriate tags in the comments for the rpc in the proto file.
• [ ] There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

[coderabbitai[bot]]

[!IMPORTANT]

Auto Review Skipped

Auto 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 .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.

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?

Share

• X
• Mastodon
• Reddit
• LinkedIn

---

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit-tests for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit tests for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit tests.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

[guggero]

This is the wrong transaction being returned. What the PR currently returns is the transaction that should be bumped. But instead we'll want to return the new transaction the sweeper creates. For that you have to use the result channel returned from UpdateParams and wait for the result being sent. That could take a whole while though, depending on the user-configured sweeper interval.

[mohamedawnallah]

Thanks @guggero for your mention and sorry for that I assumed that the transaction was mutated in place but after reading a bumpfee command again it follows the Replace by Fee Policy which creates a new copy of the transaction with higher fees and by default the miners would choose the transaction with higher fee.

Make the PR draft now until I make sure it is valid...

[mohamedawnallah]

@guggero Thanks a lot for the review. I've addressed your comments but one thing related to the cpfp on-chain test case I don't know why it fails:

test case failed:
=== RUN   TestLightningNetworkDaemon/tranche00/01-of-1/btcd/cpfp
=== NAME  TestLightningNetworkDaemon
    harness_rpc.go:97:
        	Error Trace:	/Users/mohamedawnallah/Desktop/lnd/lntest/rpc/harness_rpc.go:97
        	            				/Users/mohamedawnallah/Desktop/lnd/lntest/rpc/wallet_kit.go:242
        	            				/Users/mohamedawnallah/Desktop/lnd/itest/lnd_onchain_test.go:285
        	            				/Users/mohamedawnallah/Desktop/lnd/itest/lnd_onchain_test.go:219
        	            				/Users/mohamedawnallah/Desktop/lnd/lntest/harness.go:353
        	            				/Users/mohamedawnallah/Desktop/lnd/itest/lnd_test.go:139
        	Error:      	Received unexpected error:
        	            	rpc error: code = DeadlineExceeded desc = context deadline exceeded
        	Messages:   	Bob: failed to call BumpFee
=== NAME  TestLightningNetworkDaemon/tranche00/01-of-1/btcd/cpfp
    harness.go:406: finished test: cpfp, start height=440, end height=441, mined blocks=1
    harness.go:435:
        	Error Trace:	/Users/mohamedawnallah/Desktop/lnd/lntest/harness.go:435
        	            				/opt/homebrew/Cellar/go/1.21.7/libexec/src/testing/testing.go:1169
        	            				/opt/homebrew/Cellar/go/1.21.7/libexec/src/testing/testing.go:1347
        	            				/opt/homebrew/Cellar/go/1.21.7/libexec/src/testing/testing.go:1589
        	            				/opt/homebrew/Cellar/go/1.21.7/libexec/src/runtime/panic.go:523
        	            				/opt/homebrew/Cellar/go/1.21.7/libexec/src/testing/testing.go:999
        	            				/Users/mohamedawnallah/Desktop/lnd/lntest/rpc/harness_rpc.go:97
        	            				/Users/mohamedawnallah/Desktop/lnd/lntest/rpc/wallet_kit.go:242
        	            				/Users/mohamedawnallah/Desktop/lnd/itest/lnd_onchain_test.go:285
        	            				/Users/mohamedawnallah/Desktop/lnd/itest/lnd_onchain_test.go:219
        	            				/Users/mohamedawnallah/Desktop/lnd/lntest/harness.go:353
        	            				/Users/mohamedawnallah/Desktop/lnd/itest/lnd_test.go:139
        	Error:      	Should be empty, but was [23a209dd3f537f457db65e06347c47c596f5e9a8e8a7a0b936c2d0475cc7945c 5339f96f1e6b441bfaa2664a96ca8cd4866ecd100e5162f864662543cbbce526]
        	Test:       	TestLightningNetworkDaemon/tranche00/01-of-1/btcd/cpfp
        	Messages:   	mempool not cleaned, please mine blocks to clean them all.
=== NAME  TestLightningNetworkDaemon
    lnd_test.go:147: Failure time: 2024-03-11 12:04:01.404
    lnd_test.go:155: =========> tests finished for tranche: 0, tested 1 cases, end height: 441
--- FAIL: TestLightningNetworkDaemon (49.35s)
    --- FAIL: TestLightningNetworkDaemon/tranche00/01-of-1/btcd/cpfp (36.50s)
FAIL
make: *** [itest-only] Error 255

[ellemouton]

rpc error: code = DeadlineExceeded desc = context deadline exceeded

Note that this was the hint to why the itest fails :)

[mohamedawnallah]

Thank you, @ellemouton, for your feedback. I've implemented the changes. However, I'm missing some understanding regards the bumpfee feature. It seems that we don't get a response from lncli bumpfee where include_raw_tx is true until we mine at least one block?

Steps to reproduce

To reproduce this, I ran the following command [including the unconfirmed funding transaction id]:

lncli wallet bumpfee --sat_per_vbyte 10 --include_raw_tx true e6a074b15cc28e1f37197f599fdd2021052c382a0b0307fa395ea12c291492a4:1

The command waits until the configured value sweeper.batchwindowduration (which is set to 10s in my case) is reached. After that, the batcher batches the unconfirmed sweep transaction:

2024-03-12 12:10:28.925 [INF] SWPR: Candidate sweep set of size=1 (+0 wallet inputs), has yield=4.99960651 BTC, weight=445
2024-03-12 12:10:28.941 [INF] SWPR: Creating sweep transaction 42ce8661ee60ceaca54a6588393af0c4f7956788de8873a9e83fc4c55875a4f0 for 1 input (e6a074b15cc28e1f37197f599fdd2021052c382a0b0307fa395ea12c291492a4:1) using 2500 sat/kw, tx_weight=445, tx_fee=0.00001112 BTC, parents_count=0, parents_fee=0 BTC, parents_weight=0
2024-03-12 12:10:28.953 [INF] LNWL: Inserting unconfirmed transaction 42ce8661ee60ceaca54a6588393af0c4f7956788de8873a9e83fc4c55875a4f0
2024-03-12 12:10:28.958 [WRN] BTWL: Transaction 42ce8661ee60ceaca54a6588393af0c4f7956788de8873a9e83fc4c55875a4f0 not accepted by mempool: txn-already-in-mempool
2024-03-12 12:10:28.978 [INF] NTFN: New confirmation subscription: conf_id=3, txid=42ce8661ee60ceaca54a6588393af0c4f7956788de8873a9e83fc4c55875a4f0, num_confs=6 height_hint=501

However, it's still waiting without a response until I generate one block using bitcoin-cli:

bitcoin-cli -generate 1

After confirming the transaction, the logs of lnd show:

2024-03-12 12:11:55.865 [INF] LNWL: Marking unconfirmed transaction e6a074b15cc28e1f37197f599fdd2021052c382a0b0307fa395ea12c291492a4 mined in block 502
2024-03-12 12:11:55.866 [INF] CRTR: Pruning channel graph using block 00459e8025ac87baaf1b004d424c61afdd4f412f8c6a8999fa325ae1b85eb633 (height=502)
2024-03-12 12:11:55.878 [INF] CRTR: Block 00459e8025ac87baaf1b004d424c61afdd4f412f8c6a8999fa325ae1b85eb633 (height=502) closed 0 channels
2024-03-12 12:11:55.881 [INF] LNWL: Marking unconfirmed transaction 42ce8661ee60ceaca54a6588393af0c4f7956788de8873a9e83fc4c55875a4f0 mined in block 502
2024-03-12 12:11:56.111 [INF] NTFN: New block: height=502, sha=00459e8025ac87baaf1b004d424c61afdd4f412f8c6a8999fa325ae1b85eb633
2024-03-12 12:11:56.111 [INF] NTFN: Dispatching confirmed spend notification for outpoint=e6a074b15cc28e1f37197f599fdd2021052c382a0b0307fa395ea12c291492a4:1, script=1 0000000000000000000000000000000000000000000000000000000000000000 at current height=502: 42ce8661ee60ceaca54a6588393af0c4f7956788de8873a9e83fc4c55875a4f0[0] spending e6a074b15cc28e1f37197f599fdd2021052c382a0b0307fa395ea12c291492a4:1 at height=502
2024-03-12 12:11:56.112 [INF] NTFN: Canceling spend notification: spend_id=2, outpoint=e6a074b15cc28e1f37197f599fdd2021052c382a0b0307fa395ea12c291492a4:1, script=1 0000000000000000000000000000000000000000000000000000000000000000
2024-03-12 12:11:56.112 [INF] UTXN: Attempting to graduate height=502: num_kids=0, num_babies=0

Finally, I received the following response from the bumpfee command:

{
    "status":  "Successfully registered cpfp-tx with the sweeper",
    "sweep_tx_hex":  "02000000000101a49214292ca15e39fa07030b2a382c052120dd9f597f19371f8ec25cb174a0e6010000000000000000014bcbcc1d000000002251204a1c3a80076fca9bc75262f047ea86c54b84dc9fb5d07dca91b3d2cb0a4ed6df0140f6925b4abb71553d4d0b612522a006244a5976fc97e0068deec5de7321de152a2ac096ae82ed3b60419e2fa2472194868f90e7f5aa69832f947900f52af525c6f5010000"
}

Based on my assumption here increasing the default timeout here lntest/wait/timeouts_darwin.go wouldn't make the tests passed? How are we supposed to test the runCPFP on the response of the BumpFee call then?

Expected behavior

we should get the bumpfee response immediately after batching the unconfirmed sweep transaction to the mempool?

Actual behavior

We get the bumpfee response after the sweep transaction is confirmed.

Additional Information

network: regtest mode

[ellemouton]

@mohamedawnallah - I think you should do some investigation into why the 1 block generation is required. Perhaps there is a select statement that waits on block notifications before taking actions like responding on the return channel. Perhaps that is something that can be adapted so that it responds as soon as a the transaction hex is known.

Perhaps it turns out that the response channel only returns the tx if the transaction has been confirmed which would explain the 1 block thing. In that case, we cant change that but perhaps we can somehow get ahold of the tx hex sooner - since I cant think of a reason that we cant just get the tx hex as soon as the tx is created.

In any case - some investigation is required to find out why things work the way they do currently. Once that is known, then the question of what to do for this issue can come in to play :) So just to reiterate - first step is to follow that returned channel down the rabbit whole to find out where and when a response is sent on it

[mohamedawnallah]

Got it @ellemouton will do further investigation and ask follow-up questions if any :)

[guggero]

Removed my request for review for now. Please re-request when the PR is ready.

[mohamedawnallah]

Thanks again @ellemouton for your feedback! After deeper investigation (summarized into the following diagram) we need to wait for a message sent to resultChan till the given sweep input is confirmed to get the transaction id. Here is a general flow (clipped out of the diagram):

1. The ticker channel runs every 100 milliseconds which is responsible for running handleSweep function.

2. The sweep transaction is initially created here in sweep/sweeper.go/createSweepTx and then a new confirmation subscription created through sweep/sweeper.go/PublishTransaction.

3. We monitor the spend notification of the outpoint regards the sweep input here in sweep/sweeper.go/monitorSpend that then later send the spent event to UTXO sweeper spendChan.

4. The sweep/sweeper.go/collector listens for any incoming messages from spendChan and if any it calls handleInputSpent function which is responsible for signaling the result channels back to the callers.

[mohamedawnallah]

@ellemouton What should we do in this case? Should we go with using go routines + wait groups when we test CPFP?

[ellemouton]

@mohamedawnallah - great investigation!! 👏

So I think the main thing here is that we definitely dont want an RPC call to ever block while waiting for a block confirmation. From your investigation, it seems that this is what will now happen. This seems to be because it waits until the tx is in its final state (ie, has been confirmed & therefor we wont change it again (add more inputs/outputs etc) before sending the hex back.

So perhaps this PR will need to add a bit more so that we can just get the first tx hex as soon as it is available. If it ends up changing due to being RBF'd, that's fine. I think the users just want an initial "ok something did happen and here is the hex of the first tx it is trying" type of thing.

Does that make sense?

[ziggie1984]

Thanks for the PR great work so far and nice analysis (with which tool did you create the flow-chart ?) 💪,

Having the new PR-Sweeper Series in mind (which will land in 18), this waiting period will change. However I am not sure if we really wanna return the Sweep-Transaction here, because it's not constant, it might change over the course it is in the mempool. So I rather think of something were we depict the current sweep-transaction in the wallet pendingsweeps response. There we list all the inputs and I think there is the right place to also show the raw-transaction to a corresponding input sweep. Wdyt @yyforyongyu ?

[mohamedawnallah]

which tool did you create the flow-chart ?

I used draw.io to create the flow chart.

[ellemouton]

So I rather think of something were we depict the current sweep-transaction in the wallet pendingsweeps response.

I think that's a great idea

[mohamedawnallah]

Mapping Sweep Input to New Sweeper Transaction for Hex

How do we map the sweep input (outpoint or reference to the previous transaction output that we want to bump its fee) from wallet pendingsweeps to the newly created sweeper transaction to obtain the hex?

Further Details

Let's ensure we're on the same page. Here is my mental model about bumping fees and also I use the term "outpoint" as a synonym for "sweep input" (please correct me if I'm wrong).

Our goal, when running the bump fee command, is to return the new sweep transaction hex so the user can rebroadcast it if it fails to go through. To obtain the new sweep transaction hex, we need to identify the outpoint or sweep input from lncli wallet pendingsweeps.

Currently, we don't have access to the new sweep transaction. How can we access it to obtain the raw hex?

I've explored lncli listchaintxns, where sweep transactions are ending with the label *:sweep. We can filter these, but there might be multiple transactions referencing the same previous outpoint (e.g., CPFP to RBFP). How should we index the most recent, intended transaction when bumping the fee?

[ziggie1984]

Let's put this PR on Hold for a bit, we want to redesign the sweeper RPC cmds in 18.1, because things changed quite a bit in LND 18. But the main idea is, that bumping the fee of an input will not report back immediately the new hex transaction, rather querying the sweeper stats should reveal all the infos for a specific sweep input (e.g. current sweep hex), so that the user can cpy&paste it from there.

[lightninglabs-deploy]

@mohamedawnallah, remember to re-request review from reviewers when ready

[lightninglabs-deploy]

@mohamedawnallah, remember to re-request review from reviewers when ready

[ellemouton]

!lightninglabs-deploy mute