lnrpc+cli: add optional raw tx hex resp to bumpfee cmd and rpc
#8528
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
mohamedawnallah
force-pushed
the
bumpfee-return-raw-tx-hex-on-success
branch
from
cd5acc0 to
4b2f532
Compare
|
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 |
|
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 Make the PR draft now until I make sure it is valid... |
mohamedawnallah
force-pushed
the
bumpfee-return-raw-tx-hex-on-success
branch
from
4b2f532 to
4669433
Compare
lnrpc/rpc_utils.go
Outdated
| // SerializeAndEncodeTx serializes the given wire transaction and returns | ||
| // its hexadecimal representation. If serialization fails, an error is | ||
| // returned. | ||
| func SerializeAndEncodeTx(tx wire.MsgTx) (string, error) { |
There was a problem hiding this comment.
nit: should say SerializeAndHexEncodeTx instead.
lnrpc/walletrpc/walletkit.proto
Outdated
| @@ -1166,11 +1166,19 @@ message BumpFeeRequest { | |||
| with. | |||
| */ | |||
| uint64 sat_per_vbyte = 5; | |||
|
|
|||
| // Indicates whether to include the raw transaction hex for | |||
There was a problem hiding this comment.
Need to mention that this will wait for the batcher to create a transaction, which might take a while, at least the duration of the configured sweeper.batchwindowduration value (default is 30 seconds).
cmd/lncli/walletrpc_active.go
Outdated
| @@ -227,6 +227,11 @@ var bumpFeeCommand = cli.Command{ | |||
| Name: "force", | |||
| Usage: "sweep even if the yield is negative", | |||
| }, | |||
| cli.StringFlag{ | |||
| Name: "include_raw_tx", | |||
| Usage: "include the raw transaction hex for " + | |||
There was a problem hiding this comment.
Same here re telling the user this takes a while.
itest/lnd_onchain_test.go
Outdated
| bob.RPC.BumpFee(bumpFeeReq) | ||
| bumpFeeResp := bob.RPC.BumpFee(bumpFeeReq) | ||
|
|
||
| // The sweep tx hex on success should be set and be valid. |
mohamedawnallah
force-pushed
the
bumpfee-return-raw-tx-hex-on-success
branch
from
4669433 to
f27200a
Compare
|
@guggero Thanks a lot for the review. I've addressed your comments but one thing related to the |
lnrpc/rpc_utils.go
Outdated
| // SerializeAndHexEncodeTx serializes the given wire transaction and returns | ||
| // its hexadecimal representation. If serialization fails, an error is | ||
| // returned. | ||
| func SerializeAndHexEncodeTx(tx wire.MsgTx) (string, error) { |
There was a problem hiding this comment.
I think you should just let this function take a tx pointer. That way you dont have to de-reference everywhere you call this. Since wire.MsgTxs are usually passed around as pointers
There was a problem hiding this comment.
Okay. Initially, I considered this too. However, I opted to pass the copy because SerializeAndHexEncodeTx doesn't mutate the tx in place. I'm fine using the pointer to wire.MsgTx if it's commonly used in the codebase.
There was a problem hiding this comment.
Thanks! Updated
cmd/lncli/walletrpc_active.go
Outdated
| "sweep transaction on success. It waits for " + | ||
| "the batcher duration at least " + | ||
| "sweeper.batchwindowduration, default: 30s", |
There was a problem hiding this comment.
I think how you worded it in the proto file sounds better:
The duration it waits is at least as long as the configured sweeper.batchwindowduration value, with the default set to 30 seconds.
There was a problem hiding this comment.
Thanks! Updated
itest/lnd_onchain_test.go
Outdated
| // The sweep tx hex on success should be set and be valid. | ||
| require.NotEmpty(ht, bumpFeeResp.SweepTxHex) | ||
| rawTxBytes, err := hex.DecodeString(bumpFeeResp.SweepTxHex) | ||
| require.NoError(ht, err, "bumpfee sweepTxHex invalid hex") | ||
| rawTx := &wire.MsgTx{} | ||
| err = rawTx.Deserialize(bytes.NewReader(rawTxBytes)) | ||
| require.NoError(ht, err, "bumpfee sweepTxHex invalid") | ||
| require.Equal(ht, bumpFeeResp.SweepTxHex, rawTx.TxHash().String()) |
There was a problem hiding this comment.
the reason this itest doesnt pass is cause it now waits for the sweep tx to be created which as you mentioned in the docs takes at least 30 seconds. But the itest BumpFee call times out after 30 seconds.
There was a problem hiding this comment.
nit: also, pls can you space things out a bit more for readability?
// The sweep tx hex on success should be set and be valid.
require.NotEmpty(ht, bumpFeeResp.SweepTxHex)
rawTxBytes, err := hex.DecodeString(bumpFeeResp.SweepTxHex)
require.NoError(ht, err, "bumpfee sweepTxHex invalid hex")
rawTx := &wire.MsgTx{}
err = rawTx.Deserialize(bytes.NewReader(rawTxBytes))
require.NoError(ht, err, "bumpfee sweepTxHex invalid")
require.Equal(ht, bumpFeeResp.SweepTxHex, rawTx.TxHash().String())
There was a problem hiding this comment.
Thanks! Also Gonna space things out in itest/lnd_channel_force_close_test since it is related
Note that this was the hint to why the itest fails :) |
mohamedawnallah
force-pushed
the
bumpfee-return-raw-tx-hex-on-success
branch
2 times, most recently
from
fa72e0a to
e3011d9
Compare
|
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 Steps to reproduceTo 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:1The command waits until the configured value 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=501However, it's still waiting without a response until I generate one block using bitcoin-cli: bitcoin-cli -generate 1After 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=0Finally, 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 Expected behaviorwe should get the bumpfee response immediately after batching the unconfirmed sweep transaction to the mempool? Actual behaviorWe get the bumpfee response after the sweep transaction is confirmed. Additional Informationnetwork: |
|
@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 |
|
Got it @ellemouton will do further investigation and ask follow-up questions if any :) |
rpcserver.go
Outdated
| @@ -3874,14 +3874,12 @@ func (r *rpcServer) fetchWaitingCloseChannels( | |||
| if closingTx != nil { | |||
| closingTxid = closingTx.TxHash().String() | |||
| if includeRawTx { | |||
| var txBuf bytes.Buffer | |||
| err = closingTx.Serialize(&txBuf) | |||
| closingTxHex, err = lnrpc.SerializeAndHexEncodeTx( | |||
There was a problem hiding this comment.
The linter complains about the line being too long. Maybe the function should instead be called SerializeTx and return bytes, then the hex encoding can be done here still (then it would be a more useful function since in most places we actually send raw bytes and not hex encoded transactions). I would've suggested to do the same here, but it looks like the other RPC that includes the raw transaction also uses a string instead of bytes. So I guess consistency is slightly better than wire-level efficiency in this case.
mohamedawnallah
force-pushed
the
bumpfee-return-raw-tx-hex-on-success
branch
from
ecc69dd to
e1ab980
Compare
| if err != nil { | ||
| return nil, err | ||
| } | ||
| case <-ctx.Done(): |
There was a problem hiding this comment.
nit: new line between cases 🙏
itest/lnd_onchain_test.go
Outdated
| go func() { | ||
| bumpFeeResp := bob.RPC.BumpFee(bumpFeeReq) | ||
| require.NotEmpty(ht, bumpFeeResp.SweepTxHex) | ||
| }() |
There was a problem hiding this comment.
what is the reason that we need to spin this off in a goroutine? is it because BumpFee blocks until the tx is included in a block (meaning we need the "mine blocks" command below to run)?
I think it is important that we know exactly why this blocks. Also - if it is blocking until the tx is mined, we should determine if this is in fact the behaviour that we want here. Is it possible to get the channel to return the raw hex at the time of tx construction rather? or do we wait until a block confirmation incase we update the structure of the tx?
Regarding the goroutine - if we do decide that we in fact need to use a goroutine here, we should never do it without tracking the go routine - as it currently stands, we have no idea if the code in here ever even gets run or that it completes. So you would need to add a wait group or something so that we can wait on it later.
There was a problem hiding this comment.
What is the reason that we need to spin this off in a goroutine? is it because BumpFee blocks until the tx is included in a block (meaning we need the "mine blocks" command below to run)?
Yes, that was my intent!
There was a problem hiding this comment.
Regarding the goroutine - if we do decide that we in fact need to use a goroutine here, we should never do it without tracking the go routine - as it currently stands, we have no idea if the code in here ever even gets run or that it completes. So you would need to add a wait group or something so that we can wait on it later.
Alright 👍
There was a problem hiding this comment.
Is it possible to get the channel to return the raw hex at the time of tx construction rather?
Since the resultChan is not accessible in sweep/sweeper.go/createSweepTx where we create sweep transaction may be not feasible to get the raw hex at the time of tx construction ?
There was a problem hiding this comment.
do we wait until a block confirmation incase we update the structure of the tx?
I don't see any mutation for the structure of spent sweep transaction after the transaction confirmation (monitored by monitorSpend function) in handleInputSpent.
|
Removed my request for review for now. Please re-request when the PR is ready. |
|
Thanks again @ellemouton for your feedback! After deeper investigation (summarized into the following diagram) we need to wait for a message sent to
|
|
@ellemouton What should we do in this case? Should we go with using go routines + wait groups when we test CPFP? |
|
@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? |
|
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 |
I used draw.io to create the flow chart. |
I think that's a great idea |
|
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 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 Currently, we don't have access to the new sweep transaction. How can we access it to obtain the raw hex? I've explored
|
|
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. |
|
@mohamedawnallah, remember to re-request review from reviewers when ready |
|
!lightninglabs-deploy mute |
mohamedawnallah
force-pushed
the
bumpfee-return-raw-tx-hex-on-success
branch
from
e1ab980 to
e530393
Compare
mohamedawnallah
changed the title
mohamedawnallah
changed the title
bumpfee cmd and rpc
mohamedawnallah
force-pushed
the
bumpfee-return-raw-tx-hex-on-success
branch
from
e530393 to
3fd2cbd
Compare
mohamedawnallah
force-pushed
the
bumpfee-return-raw-tx-hex-on-success
branch
from
3fd2cbd to
eea28a7
Compare
Change Description
Fixes #8470
Steps to Test
Steps for reviewers to follow to test the change.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.