feat: Add RBF and CPFP Batcher wallets #21
Conversation
WalkthroughRecent updates enhance Bitcoin transaction management by introducing the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BatcherWallet
participant Cache
participant Indexer
User->>BatcherWallet: Start()
BatcherWallet->>Cache: Load pending requests
BatcherWallet->>Indexer: Get fee rates
BatcherWallet->>BatcherWallet: Create batch
BatcherWallet->>Cache: Save batch
User->>BatcherWallet: Send()
BatcherWallet->>Indexer: Build transaction
BatcherWallet->>Cache: Update request status
User->>BatcherWallet: Stop()
BatcherWallet->>Cache: Save state
Poem
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 Configuration File (
|
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- btc/batcher.go (1 hunks)
- btc/batcher_test.go (1 hunks)
- btc/cpfp.go (1 hunks)
- btc/cpfp_test.go (1 hunks)
- btc/rbf.go (1 hunks)
- btc/rbf_test.go (1 hunks)
- btc/wallet.go (1 hunks)
- go.mod (1 hunks)
Files skipped from review due to trivial changes (1)
- go.mod
Additional context used
golangci-lint
btc/batcher_test.go
55-55: S1005: unnecessary assignment to the blank identifier
(gosimple)
btc/cpfp_test.go
160-160: ineffectual assignment to err
(ineffassign)
Additional comments not posted (43)
btc/rbf_test.go (2)
79-82: LGTM!The test function is well-structured and covers the necessary steps to verify the functionality.
55-55: Remove unnecessary assignment to the blank identifier.The assignment to the blank identifier is unnecessary and can be removed.
- _, err := localnet.FundBitcoin(wallet.Address().EncodeAddress(), indexer) + err = localnet.FundBitcoin(wallet.Address().EncodeAddress(), indexer)Likely invalid or redundant comment.
btc/batcher_test.go (16)
17-22: LGTM!The function is well-structured and correctly initializes the test cache.
24-31: LGTM!The function is well-structured and correctly reads a batch by request ID.
33-39: LGTM!The function is well-structured and correctly reads a batch by transaction ID.
41-49: LGTM!The function is well-structured and correctly reads pending batches.
50-62: LGTM!The function is well-structured and correctly saves a batch.
Tools
golangci-lint
55-55: S1005: unnecessary assignment to the blank identifier
(gosimple)
64-73: LGTM!The function is well-structured and correctly updates the statuses of batches.
76-82: LGTM!The function is well-structured and correctly reads a request by ID.
83-91: LGTM!The function is well-structured and correctly reads pending requests.
93-100: LGTM!The function is well-structured and correctly saves a request.
102-113: LGTM!The function is well-structured and correctly updates the fees of batches.
115-128: LGTM!The function is well-structured and correctly reads the latest batch.
130-140: LGTM!The function is well-structured and correctly reads requests by IDs.
142-164: LGTM!The function is well-structured and correctly deletes pending batches.
166-174: LGTM!The function is well-structured and correctly reads pending change UTXOs.
175-183: LGTM!The function is well-structured and correctly reads pending funding UTXOs.
203-207: LGTM!The function is well-structured and correctly initializes the mock fee estimator.
btc/cpfp_test.go (3)
82-95: LGTM!The test function is well-structured and covers the necessary steps to verify the functionality.
97-142: LGTM!The test function is well-structured and covers the necessary steps to verify the functionality.
39-39: Remove unnecessary assignment to the blank identifier.The assignment to the blank identifier is unnecessary and can be removed.
- _, err := localnet.FundBitcoin(wallet.Address().EncodeAddress(), indexer) + err = localnet.FundBitcoin(wallet.Address().EncodeAddress(), indexer)Likely invalid or redundant comment.
btc/batcher.go (14)
1-17: Imports look good.All necessary imports are present and there are no unused imports.
19-38: Constants and error variables look good.All necessary constants and error variables are defined and there are no unnecessary ones.
40-76: Interfaces look good.All necessary methods in the interfaces are defined and there are no unnecessary ones.
78-120: Structs look good.All necessary fields in the structs are defined and there are no unnecessary ones.
122-143: Structs look good.All necessary fields in the structs are defined and there are no unnecessary ones.
145-167: FunctionNewBatcherWalletlooks good.The function is correctly defined and handles errors properly.
169-188: FunctiondefaultBatcherOptionslooks good.The function is correctly defined and returns default options.
190-215: FunctionsWithStrategy,WithPTI, andparseStrategylook good.The functions are correctly defined and handle errors properly.
217-219: MethodAddresslooks good.The method is correctly defined and returns the address.
221-235: MethodSendlooks good.The method is correctly defined and handles errors properly.
237-256: MethodStatuslooks good.The method is correctly defined and handles errors properly.
258-291: MethodsStart,Stop, andRestartlook good.The methods are correctly defined and handle errors properly.
293-387: Methodsrun,runPTIBatcher,updateFeeRate, andcreateBatchlook good.The methods are correctly defined and handle errors properly.
389-462: FunctionsvalidateUpdate,selectFee,filterPendingBatches,getTransaction, andwithContextTimeoutlook good.The functions are correctly defined and handle errors properly.
btc/wallet.go (4)
Line range hint
1-14: Imports look good.All necessary imports are present and there are no unused imports.
252-254: Update inbuildTransactionfunction looks good.The function is correctly defined and handles the new logic properly.
252-254: Update inbuildTransactionfunction looks good.The function is correctly defined and handles the new logic properly.
252-254: Update inbuildTransactionfunction looks good.The function is correctly defined and handles the new logic properly.
btc/rbf.go (4)
1-16: Imports look good.All necessary imports are present and there are no unused imports.
18-67: FunctioncreateRBFBatchlooks good.The function is correctly defined and handles errors properly.
69-119: FunctionreSubmitRBFBatchlooks good.The function is correctly defined and handles errors properly.
121-510: FunctionsgetConfirmedBatch,getMissingRequestIds,createNewRBFBatch,updateRBF,createRBFTx,getUtxosForFee,getUnconfirmedUtxos, andbuildRBFTransactionlook good.The functions are correctly defined and handle errors properly.
btc/batcher.go
Outdated
| ErrBatchNotFound = errors.New("batch not found") | ||
| ErrBatcherStillRunning = errors.New("batcher is still running") | ||
| ErrBatcherNotRunning = errors.New("batcher is not running") | ||
| ErrBatchParametersNotMet = errors.New("batch parameters not met") |
There was a problem hiding this comment.
The error is generic and does not specify what exactly went wrong. Maybe you can break the error into multiple errors and use them.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- btc/batcher.go (1 hunks)
- btc/batcher_test.go (1 hunks)
- btc/cpfp.go (1 hunks)
- btc/rbf_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- btc/batcher.go
- btc/batcher_test.go
- btc/rbf_test.go
Additional context used
golangci-lint
btc/cpfp.go
393-393: func
reconstructCPFPBatchesis unused(unused)
Additional comments not posted (18)
btc/cpfp.go (18)
22-34: Avoid nested context timeouts.The nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { - requests, err = w.cache.ReadPendingRequests(ctx) - return err - }) + requests, err = w.cache.ReadPendingRequests(c) + if err != nil { + return err + }
58-66: Avoid nested context timeouts.The nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { - batches, err = w.cache.ReadPendingBatches(ctx, w.opts.Strategy) - return err - }) + batches, err = w.cache.ReadPendingBatches(c, w.opts.Strategy) + if err != nil { + return err + }
74-79: Avoid nested context timeouts.The nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { - return w.cache.ConfirmBatchStatuses(ctx, confirmedTxs, false, CPFP) - }) + err = w.cache.ConfirmBatchStatuses(c, confirmedTxs, false, CPFP) + if err != nil { + return err + }
94-102: Avoid nested context timeouts.The nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { - utxos, err = w.indexer.GetUTXOs(ctx, w.address) - return err - }) + utxos, err = w.indexer.GetUTXOs(c, w.address) + if err != nil { + return err + }
122-127: Avoid nested context timeouts.The nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { - return w.indexer.SubmitTx(ctx, tx) - }) + err = w.indexer.SubmitTx(c, tx) + if err != nil { + return err + }
131-137: Avoid nested context timeouts.The nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { - transaction, err = getTransaction(w.indexer, tx.TxHash().String()) - return err - }) + transaction, err = getTransaction(w.indexer, tx.TxHash().String()) + if err != nil { + return err + }
155-160: Avoid nested context timeouts.The nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { - return w.cache.SaveBatch(ctx, batch) - }) + err = w.cache.SaveBatch(c, batch) + if err != nil { + return ErrSavingBatch + }
172-178: Avoid nested context timeouts.The nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { - batches, err = w.cache.ReadPendingBatches(ctx, w.opts.Strategy) - return err - }) + batches, err = w.cache.ReadPendingBatches(c, w.opts.Strategy) + if err != nil { + return err + }
186-191: Avoid nested context timeouts.The nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { - return w.cache.ConfirmBatchStatuses(ctx, confirmedTxs, false, CPFP) - }) + err = w.cache.ConfirmBatchStatuses(c, confirmedTxs, false, CPFP) + if err != nil { + return err + }
200-206: Avoid nested context timeouts.The nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { - utxos, err = w.indexer.GetUTXOs(ctx, w.address) - return err - }) + utxos, err = w.indexer.GetUTXOs(c, w.address) + if err != nil { + return err + }
237-242: Avoid nested context timeouts.The nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { - return w.indexer.SubmitTx(ctx, tx) - }) + err = w.indexer.SubmitTx(c, tx) + if err != nil { + return err + }
245-250: Avoid nested context timeouts.The nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { - return w.cache.UpdateBatchFees(ctx, pendingTxs, int64(requiredFeeRate)) - }) + err = w.cache.UpdateBatchFees(c, pendingTxs, int64(requiredFeeRate)) + if err != nil { + return err + }
261-275: Fix the recursion depth check.The recursion depth check might cause unintended behavior. Consider changing the condition to
depth <= 0.- if depth < 0 { + if depth <= 0 {
283-289: Avoid nested context timeouts.The nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { - spendUTXOs, spendUTXOsMap, balanceOfScripts, err = getUTXOsForSpendRequest(ctx, w.indexer, spendRequests) - return err - }) + spendUTXOs, spendUTXOsMap, balanceOfScripts, err = getUTXOsForSpendRequest(c, w.indexer, spendRequests) + if err != nil { + return err + }
339-344: Avoid nested context timeouts.The nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { - return signSpendTx(ctx, tx, signIdx, spendRequests, spendUTXOsMap, w.indexer, w.privateKey) - }) + err = signSpendTx(c, tx, signIdx, spendRequests, spendUTXOsMap, w.indexer, w.privateKey) + if err != nil { + return nil, err + }
356-361: Avoid nested context timeouts.The nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { - sacpsIn, sacpOut, err = getTotalInAndOutSACPs(ctx, sacps, w.indexer) - return err - }) + sacpsIn, sacpOut, err = getTotalInAndOutSACPs(c, sacps, w.indexer) + if err != nil { + return nil, err + }
393-399: Implement thereconstructCPFPBatchesfunction.The function is currently a placeholder with a TODO comment. Implement the function to ensure proper functionality.
Do you want me to help implement this function or open a GitHub issue to track this task?
Tools
golangci-lint
393-393: func
reconstructCPFPBatchesis unused(unused)
405-414: Fix the error handling.The error handling might cause unintended behavior. Consider adding a check for
ErrCPFPFeeUpdateParamsNotMet.- if err == ErrFeeUpdateNotNeeded && feeStats.FeeDelta > 0 { + if (err == ErrFeeUpdateNotNeeded || err == ErrCPFPFeeUpdateParamsNotMet) && feeStats.FeeDelta > 0 {
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- btc/batcher.go (1 hunks)
Additional context used
golangci-lint
btc/batcher.go
309-309: Error return value of
w.runis not checked(errcheck)
Additional comments not posted (14)
btc/batcher.go (14)
201-220: LGTM!The function provides sensible default options for the
batcherWallet.
222-231: LGTM!The function correctly handles strategy validation and configuration.
233-237: LGTM!The function correctly sets the periodic time interval for the
batcherWallet.
240-247: LGTM!The function correctly validates the provided strategy.
249-251: LGTM!The function correctly returns the address of the
batcherWallet.
253-255: LGTM!The function correctly delegates the generation of SACP to the underlying simple wallet.
257-259: LGTM!The function correctly delegates the signing of SACP transactions to the underlying simple wallet.
313-326: LGTM!The function correctly handles service stop and error conditions.
328-334: LGTM!The function correctly handles service restart and error conditions.
336-349: LGTM!The function correctly handles strategy selection and error conditions.
394-413: LGTM!The function correctly handles fee rate updates and strategy selection.
415-429: LGTM!The function correctly handles batch creation and strategy selection.
494-509: LGTM!The function correctly handles fee rate validation and threshold checking.
351-392: Check the error return value ofw.run.The error return value of
w.runis not checked, which may lead to unhandled errors.w.run(ctx) + if err := w.run(ctx); err != nil { + return err + }Likely invalid or redundant comment.
btc/batcher.go
Outdated
| // Start starts the batcher wallet service | ||
| func (w *batcherWallet) Start(ctx context.Context) error { | ||
| if w.quit != nil { | ||
| return ErrBatcherStillRunning | ||
| } | ||
| w.quit = make(chan struct{}) | ||
|
|
||
| w.logger.Info("starting batcher wallet") | ||
| w.run(ctx) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Check the error return value of w.run.
The error return value of w.run is not checked, which may lead to unhandled errors.
w.run(ctx)
+ if err := w.run(ctx); err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Start starts the batcher wallet service | |
| func (w *batcherWallet) Start(ctx context.Context) error { | |
| if w.quit != nil { | |
| return ErrBatcherStillRunning | |
| } | |
| w.quit = make(chan struct{}) | |
| w.logger.Info("starting batcher wallet") | |
| w.run(ctx) | |
| return nil | |
| } | |
| // Start starts the batcher wallet service | |
| func (w *batcherWallet) Start(ctx context.Context) error { | |
| if w.quit != nil { | |
| return ErrBatcherStillRunning | |
| } | |
| w.quit = make(chan struct{}) | |
| w.logger.Info("starting batcher wallet") | |
| if err := w.run(ctx); err != nil { | |
| return err | |
| } | |
| return nil | |
| } |
Tools
golangci-lint
309-309: Error return value of
w.runis not checked(errcheck)
btc/batcher.go
Outdated
| // Send creates a batch request , saves it in the cache and returns a tracking id | ||
| func (w *batcherWallet) Send(ctx context.Context, sends []SendRequest, spends []SpendRequest, sacps [][]byte) (string, error) { | ||
| if err := w.validateBatchRequest(ctx, w.opts.Strategy, spends, sends, sacps); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| // generate random id | ||
| id := chainhash.HashH([]byte(fmt.Sprintf("%v", time.Now().UnixNano()))).String() | ||
|
|
||
| req := BatcherRequest{ | ||
| ID: id, | ||
| Spends: spends, | ||
| Sends: sends, | ||
| SACPs: sacps, | ||
| Status: false, | ||
| } | ||
| return id, w.cache.SaveRequest(ctx, id, req) | ||
| } |
There was a problem hiding this comment.
Add logging for successful request saving.
Consider adding a log statement after successfully saving the batch request to the cache.
return id, w.cache.SaveRequest(ctx, id, req)
+ if err == nil {
+ w.logger.Info("Batch request saved successfully", zap.String("id", id))
+ }Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- btc/rbf.go (1 hunks)
- btc/wallet_test.go (42 hunks)
Files skipped from review as they are similar to previous changes (2)
- btc/rbf.go
- btc/wallet_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- .github/workflows/test.yml (1 hunks)
- btc/batcher.go (1 hunks)
- btc/cpfp.go (1 hunks)
- btc/rbf.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- btc/rbf.go
Additional context used
golangci-lint
btc/cpfp.go
379-379: func
reconstructCPFPBatchesis unused(unused)
btc/batcher.go
309-309: Error return value of
w.runis not checked(errcheck)
Additional comments not posted (15)
.github/workflows/test.yml (4)
10-10: LGTM!The change to set up Go using
actions/setup-go@v4is standard and correct.
12-12: LGTM!Specifying the Go version as ">=1.19.0" ensures compatibility with newer features.
18-18: LGTM!Installing Ginkgo using
go installis necessary for running the tests.
20-24: LGTM!The updated test command includes Ginkgo commands for specific test modes, enhancing the testing process.
btc/cpfp.go (4)
389-400: LGTM!The function
getFeeStatsis well-implemented.
22-149: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
createCPFPBatchare correctly implemented.
243-373: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
buildCPFPTxare correctly implemented.
153-241: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
updateCPFPare correctly implemented.Verification successful
Function usage verified and correctly implemented.
The function
updateCPFPis called within a switch statement inbtc/batcher.go, ensuring it is invoked based on the strategy option. The context and fee rate are appropriately set up before the call.
btc/batcher.go: Verified correct implementation ofupdateCPFP.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `updateCPFP` in the codebase. # Test: Search for the function usage. Expect: Only occurances of the new function. rg --type go -A 5 $'updateCPFP'Length of output: 680
Script:
#!/bin/bash # Description: Gather more context around the call to `updateCPFP` in `btc/batcher.go`. # Test: Search for the function usage and get more lines of context. rg --type go -A 20 -B 10 $'updateCPFP' btc/batcher.goLength of output: 741
btc/batcher.go (7)
431-491: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
validateBatchRequestare correctly implemented.Verification successful
Function usage verified in the codebase.
The function
validateBatchRequestis only used within thebtc/batcher.gofile, and its usage appears to be correctly implemented.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `validateBatchRequest` in the codebase. # Test: Search for the function usage. Expect: Only occurances of the new function. rg --type go -A 5 $'validateBatchRequest'Length of output: 740
415-429: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
createBatchare correctly implemented.
341-349: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
runare correctly implemented.Verification successful
Verified: Function usage is correct.
The
runfunction is only called withinbtc/batcher.go, ensuring its usage is consistent and contained within the same file.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `run` in the codebase. # Test: Search for the function usage. Expect: Only occurances of the new function. rg --type go -A 5 $'run'Length of output: 2636
395-413: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
updateFeeRateare correctly implemented.Verification successful
Function usage verified and correct.
The function
updateFeeRateis only used within the same file (btc/batcher.go) and handles errors appropriately. No further issues found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `updateFeeRate` in the codebase. # Test: Search for the function usage. Expect: Only occurances of the new function. rg --type go -A 5 $'updateFeeRate'Length of output: 718
356-392: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
runPTIBatcherare correctly implemented.Verification successful
The function
runPTIBatcheris used correctly within the codebase.The function
runPTIBatcheris defined and called within thebtc/batcher.gofile. The usage appears to be in the correct context.
- File:
btc/batcher.go
- Line:
w.runPTIBatcher(ctx)Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `runPTIBatcher` in the codebase. # Test: Search for the function usage. Expect: Only occurances of the new function. rg --type go -A 5 $'runPTIBatcher'Length of output: 452
169-199: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
NewBatcherWalletare correctly implemented.
261-278: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
Sendare correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 14
Outside diff range, codebase verification and nitpick comments (4)
btc/cpfp.go (4)
148-149: Add logging for successful submission of CPFP batch.Consider adding a log statement after successfully submitting the CPFP batch.
+ w.logger.Info("submitted CPFP batch", zap.String("txid", tx.TxHash().String()))
238-239: Add logging for successful submission of CPFP transaction.Consider adding a log statement after successfully submitting the CPFP transaction.
+ w.logger.Info("submitted CPFP transaction", zap.String("txid", tx.TxHash().String()))
313-321: Add logging for buffer fee calculation.Consider adding a log statement for the buffer fee calculation to help with debugging.
+ w.logger.Debug("buffer fee calculated", zap.Int("bufferFee", bufferFee))
358-370: Add logging for rebuilding CPFP transaction.Consider adding a log statement for rebuilding the CPFP transaction to help with debugging.
+ w.logger.Info( + "rebuilding CPFP transaction", + zap.Int("depth", depth), + zap.Int("fee", fee), + zap.Int("feeOverhead", feeOverhead), + zap.Int("feeBuffer", bufferFee), + zap.Int("required", newFeeEstimate), + zap.Int("coverUtxos", len(utxos)), + zap.Int("TxIns", len(tx.TxIn)), + zap.Int("TxOuts", len(tx.TxOut)), + zap.String("TxData", hex.EncodeToString(txBytes)), + )
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- .github/workflows/test.yml (1 hunks)
- btc/batcher.go (1 hunks)
- btc/cpfp.go (1 hunks)
- btc/rbf.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- .github/workflows/test.yml
- btc/rbf.go
Additional context used
golangci-lint
btc/cpfp.go
379-379: func
reconstructCPFPBatchesis unused(unused)
btc/batcher.go
309-309: Error return value of
w.runis not checked(errcheck)
Additional comments not posted (11)
btc/cpfp.go (4)
389-399: LGTM!The function correctly generates fee stats and handles errors.
403-423: LGTM!The function correctly calculates fee stats.
425-437: LGTM!The function correctly removes double spends from UTXOs.
Line range hint
570-592: LGTM!The function correctly calculates the total input and output amounts for SACPs.
Tools
golangci-lint
379-379: func
reconstructCPFPBatchesis unused(unused)
btc/batcher.go (7)
169-199: LGTM!The function correctly initializes a new batcher wallet and validates the provided options.
201-219: LGTM!The function correctly returns the default options for a batcher wallet.
222-230: LGTM!The function correctly sets the strategy for a batcher wallet.
233-237: LGTM!The function correctly sets the periodic time interval for a batcher wallet.
240-247: LGTM!The function correctly validates the provided strategy.
249-251: LGTM!The function correctly returns the address of the batcher wallet.
253-255: LGTM!The function correctly generates a SACP for a spend request.
btc/cpfp.go
Outdated
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | ||
| return w.cache.SaveBatch(ctx, batch) | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error {
- return w.cache.SaveBatch(ctx, batch)
- })
+ err = w.cache.SaveBatch(c, batch)
+ if err != nil {
+ return ErrSavingBatch
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | |
| return w.cache.SaveBatch(ctx, batch) | |
| }) | |
| err = w.cache.SaveBatch(c, batch) | |
| if err != nil { | |
| return ErrSavingBatch | |
| } |
btc/cpfp.go
Outdated
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | ||
| utxos, err = w.indexer.GetUTXOs(ctx, w.address) | ||
| return err | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error {
- utxos, err = w.indexer.GetUTXOs(ctx, w.address)
- return err
- })
+ utxos, err = w.indexer.GetUTXOs(c, w.address)
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | |
| utxos, err = w.indexer.GetUTXOs(ctx, w.address) | |
| return err | |
| }) | |
| utxos, err = w.indexer.GetUTXOs(c, w.address) | |
| if err != nil { | |
| return err | |
| } |
btc/cpfp.go
Outdated
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | ||
| return w.cache.UpdateBatchFees(ctx, pendingTxs, int64(requiredFeeRate)) | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error {
- return w.cache.UpdateBatchFees(ctx, pendingTxs, int64(requiredFeeRate))
- })
+ err = w.cache.UpdateBatchFees(c, pendingTxs, int64(requiredFeeRate))
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | |
| return w.cache.UpdateBatchFees(ctx, pendingTxs, int64(requiredFeeRate)) | |
| }) | |
| err = w.cache.UpdateBatchFees(c, pendingTxs, int64(requiredFeeRate)) | |
| if err != nil { | |
| return err | |
| } |
| // Return error if no requests found | ||
| if len(requests) == 0 { | ||
| return ErrBatchParametersNotMet | ||
| } |
There was a problem hiding this comment.
Improve error message for missing requests.
The error message ErrBatchParametersNotMet is not descriptive enough. Consider providing a more specific error message.
- return ErrBatchParametersNotMet
+ return errors.New("no pending requests found")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Return error if no requests found | |
| if len(requests) == 0 { | |
| return ErrBatchParametersNotMet | |
| } | |
| // Return error if no requests found | |
| if len(requests) == 0 { | |
| return errors.New("no pending requests found") | |
| } |
btc/cpfp.go
Outdated
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | ||
| return w.indexer.SubmitTx(ctx, tx) | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error {
- return w.indexer.SubmitTx(ctx, tx)
- })
+ err = w.indexer.SubmitTx(c, tx)
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | |
| return w.indexer.SubmitTx(ctx, tx) | |
| }) | |
| err = w.indexer.SubmitTx(c, tx) | |
| if err != nil { | |
| return err | |
| } |
btc/cpfp.go
Outdated
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | ||
| utxos, err = w.indexer.GetUTXOs(ctx, w.address) | ||
| return err | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error {
- utxos, err = w.indexer.GetUTXOs(ctx, w.address)
- return err
- })
+ utxos, err = w.indexer.GetUTXOs(c, w.address)
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | |
| utxos, err = w.indexer.GetUTXOs(ctx, w.address) | |
| return err | |
| }) | |
| utxos, err = w.indexer.GetUTXOs(c, w.address) | |
| if err != nil { | |
| return err | |
| } |
btc/cpfp.go
Outdated
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | ||
| return w.cache.ConfirmBatchStatuses(ctx, confirmedTxs, false, CPFP) | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error {
- return w.cache.ConfirmBatchStatuses(ctx, confirmedTxs, false, CPFP)
- })
+ err = w.cache.ConfirmBatchStatuses(c, confirmedTxs, false, CPFP)
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | |
| return w.cache.ConfirmBatchStatuses(ctx, confirmedTxs, false, CPFP) | |
| }) | |
| err = w.cache.ConfirmBatchStatuses(c, confirmedTxs, false, CPFP) | |
| if err != nil { | |
| return err | |
| } |
btc/cpfp.go
Outdated
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | ||
| return w.cache.ConfirmBatchStatuses(ctx, confirmedTxs, false, CPFP) | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error {
- return w.cache.ConfirmBatchStatuses(ctx, confirmedTxs, false, CPFP)
- })
+ err = w.cache.ConfirmBatchStatuses(c, confirmedTxs, false, CPFP)
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | |
| return w.cache.ConfirmBatchStatuses(ctx, confirmedTxs, false, CPFP) | |
| }) | |
| err = w.cache.ConfirmBatchStatuses(c, confirmedTxs, false, CPFP) | |
| if err != nil { | |
| return err | |
| } |
btc/cpfp.go
Outdated
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | ||
| transaction, err = getTransaction(w.indexer, tx.TxHash().String()) | ||
| return err |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error {
- transaction, err = getTransaction(w.indexer, tx.TxHash().String())
- return err
- })
+ transaction, err = getTransaction(w.indexer, tx.TxHash().String())
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | |
| transaction, err = getTransaction(w.indexer, tx.TxHash().String()) | |
| return err | |
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | |
| transaction, err = getTransaction(w.indexer, tx.TxHash().String()) | |
| if err != nil { | |
| return err | |
| } | |
| return nil | |
| }) |
btc/cpfp.go
Outdated
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | ||
| requests, err = w.cache.ReadPendingRequests(ctx) | ||
| return err | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error {
- requests, err = w.cache.ReadPendingRequests(ctx)
- return err
- })
+ requests, err = w.cache.ReadPendingRequests(c)
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | |
| requests, err = w.cache.ReadPendingRequests(ctx) | |
| return err | |
| }) | |
| requests, err = w.cache.ReadPendingRequests(c) | |
| if err != nil { | |
| return err | |
| } |
btc/rbf.go
Outdated
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Ensure proper error wrapping.
When returning errors, consider wrapping them to provide more context.
- return err
+ return fmt.Errorf("failed to read pending requests: %w", err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil { | |
| return err | |
| } | |
| if err != nil { | |
| return fmt.Errorf("failed to read pending requests: %w", err) | |
| } |
btc/rbf.go
Outdated
| sacpsInAmount, sacpsOutAmount, err = getTotalInAndOutSACPs(ctx, sacps, w.indexer) | ||
| return err | ||
| }) |
There was a problem hiding this comment.
Ensure proper error wrapping.
When returning errors, consider wrapping them to provide more context.
- return err
+ return fmt.Errorf("failed to get total in and out SACPs: %w", err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sacpsInAmount, sacpsOutAmount, err = getTotalInAndOutSACPs(ctx, sacps, w.indexer) | |
| return err | |
| }) | |
| sacpsInAmount, sacpsOutAmount, err = getTotalInAndOutSACPs(ctx, sacps, w.indexer) | |
| return fmt.Errorf("failed to get total in and out SACPs: %w", err) |
btc/rbf.go
Outdated
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | ||
| batches, err = w.cache.ReadPendingBatches(ctx, RBF) | ||
| return err |
There was a problem hiding this comment.
Ensure proper error wrapping.
When returning errors, consider wrapping them to provide more context.
- return err
+ return fmt.Errorf("failed to read pending batches: %w", err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | |
| batches, err = w.cache.ReadPendingBatches(ctx, RBF) | |
| return err | |
| err = withContextTimeout(c, DefaultContextTimeout, func(ctx context.Context) error { | |
| batches, err = w.cache.ReadPendingBatches(ctx, RBF) | |
| return fmt.Errorf("failed to read pending batches: %w", err) |
btc/rbf.go
Outdated
| if txBytes, err = GetTxRawBytes(tx); err != nil { | ||
| return nil, nil, nil, err |
There was a problem hiding this comment.
Ensure proper error wrapping.
When returning errors, consider wrapping them to provide more context.
- return err
+ return fmt.Errorf("failed to get raw transaction bytes: %w", err)Committable suggestion was skipped due to low confidence.
btc/rbf.go
Outdated
| err = signSendTx(tx, utxos, signIdx+len(spendUTXOs), w.address, w.privateKey) | ||
| if err != nil { | ||
| return nil, nil, nil, err |
There was a problem hiding this comment.
Ensure proper error wrapping.
When returning errors, consider wrapping them to provide more context.
- return nil, nil, nil, err
+ return nil, nil, nil, fmt.Errorf("failed to sign send transaction: %w", err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = signSendTx(tx, utxos, signIdx+len(spendUTXOs), w.address, w.privateKey) | |
| if err != nil { | |
| return nil, nil, nil, err | |
| err = signSendTx(tx, utxos, signIdx+len(spendUTXOs), w.address, w.privateKey) | |
| if err != nil { | |
| return nil, nil, nil, fmt.Errorf("failed to sign send transaction: %w", err) |
btc/rbf.go
Outdated
| latestBatch, err = w.cache.ReadLatestBatch(ctx, RBF) | ||
| return err | ||
| }) |
There was a problem hiding this comment.
Ensure proper error wrapping.
When returning errors, consider wrapping them to provide more context.
- return err
+ return fmt.Errorf("failed to read the latest RBF batch: %w", err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| latestBatch, err = w.cache.ReadLatestBatch(ctx, RBF) | |
| return err | |
| }) | |
| latestBatch, err = w.cache.ReadLatestBatch(ctx, RBF) | |
| return fmt.Errorf("failed to read the latest RBF batch: %w", err) |
| utxos, _, err = w.getUtxosWithFee(ctx, totalOut+int64(newFeeEstimate)-totalIn, int64(feeRate), avoidUtxos) | ||
| return err | ||
| }) |
There was a problem hiding this comment.
Ensure proper error wrapping.
When returning errors, consider wrapping them to provide more context.
- return err
+ return fmt.Errorf("failed to get UTXOs with fee: %w", err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| utxos, _, err = w.getUtxosWithFee(ctx, totalOut+int64(newFeeEstimate)-totalIn, int64(feeRate), avoidUtxos) | |
| return err | |
| }) | |
| utxos, _, err = w.getUtxosWithFee(ctx, totalOut+int64(newFeeEstimate)-totalIn, int64(feeRate), avoidUtxos) | |
| return fmt.Errorf("failed to get UTXOs with fee: %w", err) |
| tx, err = w.indexer.GetTx(ctx, batch.Tx.TxID) | ||
| return err | ||
| }) |
There was a problem hiding this comment.
Ensure proper error wrapping.
When returning errors, consider wrapping them to provide more context.
- return err
+ return fmt.Errorf("failed to get transaction: %w", err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tx, err = w.indexer.GetTx(ctx, batch.Tx.TxID) | |
| return err | |
| }) | |
| tx, err = w.indexer.GetTx(ctx, batch.Tx.TxID) | |
| return fmt.Errorf("failed to get transaction: %w", err) |
btc/rbf.go
Outdated
| tx, fundingUtxos, selfUtxos, err = w.createRBFTx( | ||
| c, | ||
| nil, | ||
| spendRequests, | ||
| sendRequests, | ||
| sacps, | ||
| nil, | ||
| avoidUtxos, | ||
| 0, // will be calculated in the function | ||
| requiredFeeRate, | ||
| false, | ||
| 2, | ||
| ) | ||
| return err | ||
| }) |
There was a problem hiding this comment.
Ensure proper error wrapping.
When returning errors, consider wrapping them to provide more context.
- return err
+ return fmt.Errorf("failed to create RBF transaction: %w", err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tx, fundingUtxos, selfUtxos, err = w.createRBFTx( | |
| c, | |
| nil, | |
| spendRequests, | |
| sendRequests, | |
| sacps, | |
| nil, | |
| avoidUtxos, | |
| 0, // will be calculated in the function | |
| requiredFeeRate, | |
| false, | |
| 2, | |
| ) | |
| return err | |
| }) | |
| tx, fundingUtxos, selfUtxos, err = w.createRBFTx( | |
| c, | |
| nil, | |
| spendRequests, | |
| sendRequests, | |
| sacps, | |
| nil, | |
| avoidUtxos, | |
| 0, // will be calculated in the function | |
| requiredFeeRate, | |
| false, | |
| 2, | |
| ) | |
| return fmt.Errorf("failed to create RBF transaction: %w", err) |
btc/rbf.go
Outdated
| if depth < 0 { | ||
| w.logger.Debug( | ||
| ErrBuildRBFDepthExceeded.Error(), | ||
| zap.Any("utxos", utxos), | ||
| zap.Any("spendRequests", spendRequests), | ||
| zap.Any("sendRequests", sendRequests), | ||
| zap.Any("sacps", sacps), | ||
| zap.Any("sequencesMap", sequencesMap), | ||
| zap.Any("avoidUtxos", avoidUtxos), | ||
| zap.Uint("fee", fee), | ||
| zap.Int("requiredFeeRate", feeRate), | ||
| zap.Bool("checkValidity", checkValidity), | ||
| zap.Int("depth", depth), | ||
| ) | ||
| return nil, nil, nil, ErrBuildRBFDepthExceeded |
There was a problem hiding this comment.
Improve logging for recursion depth exceeded.
The logging should include the error context for better debugging.
- w.logger.Debug(
- ErrBuildRBFDepthExceeded.Error(),
+ w.logger.Debug("recursion depth exceeded",Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if depth < 0 { | |
| w.logger.Debug( | |
| ErrBuildRBFDepthExceeded.Error(), | |
| zap.Any("utxos", utxos), | |
| zap.Any("spendRequests", spendRequests), | |
| zap.Any("sendRequests", sendRequests), | |
| zap.Any("sacps", sacps), | |
| zap.Any("sequencesMap", sequencesMap), | |
| zap.Any("avoidUtxos", avoidUtxos), | |
| zap.Uint("fee", fee), | |
| zap.Int("requiredFeeRate", feeRate), | |
| zap.Bool("checkValidity", checkValidity), | |
| zap.Int("depth", depth), | |
| ) | |
| return nil, nil, nil, ErrBuildRBFDepthExceeded | |
| if depth < 0 { | |
| w.logger.Debug("recursion depth exceeded", | |
| zap.Any("utxos", utxos), | |
| zap.Any("spendRequests", spendRequests), | |
| zap.Any("sendRequests", sendRequests), | |
| zap.Any("sacps", sacps), | |
| zap.Any("sequencesMap", sequencesMap), | |
| zap.Any("avoidUtxos", avoidUtxos), | |
| zap.Uint("fee", fee), | |
| zap.Int("requiredFeeRate", feeRate), | |
| zap.Bool("checkValidity", checkValidity), | |
| zap.Int("depth", depth), | |
| ) | |
| return nil, nil, nil, ErrBuildRBFDepthExceeded |
There was a problem hiding this comment.
Actionable comments posted: 21
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- btc/batcher.go (1 hunks)
- btc/cpfp.go (1 hunks)
- btc/rbf.go (1 hunks)
- btc/wallet.go (15 hunks)
Files skipped from review as they are similar to previous changes (1)
- btc/rbf.go
Additional context used
golangci-lint
btc/cpfp.go
379-379: func
reconstructCPFPBatchesis unused(unused)
btc/batcher.go
298-298: Error return value of
w.runis not checked(errcheck)
Additional comments not posted (14)
btc/batcher.go (6)
484-497: LGTM!The logic for validating the fee rate delta is comprehensive and correct.
398-403: LGTM!The function correctly calculates the fee used in the given SACPs.
Line range hint
622-634:
LGTM!The function correctly signs the spend transaction with the given inputs and script.
Tools
golangci-lint
298-298: Error return value of
w.runis not checked(errcheck)
Line range hint
733-813:
LGTM!The function correctly validates spend, send, and SACP requests.
Tools
golangci-lint
298-298: Error return value of
w.runis not checked(errcheck)
Line range hint
825-838:
LGTM!The function correctly counts the number of Segwit and Schnorr signatures in the spend requests.
Tools
golangci-lint
298-298: Error return value of
w.runis not checked(errcheck)
Line range hint
853-864:
LGTM!The function correctly validates the SACP transaction.
Tools
golangci-lint
298-298: Error return value of
w.runis not checked(errcheck)
btc/wallet.go (8)
206-211: LGTM!The function correctly validates requests and estimates fees for sending funds.
249-249: LGTM!The function correctly validates requests and generates a SACP transaction.
Line range hint
279-299:
LGTM!The function correctly generates a SACP transaction with the given details.
Line range hint
372-396:
LGTM!The function correctly calculates the total input and output amounts for the given SACPs.
398-403: LGTM!The function correctly calculates the fee used in the given SACPs.
Line range hint
417-431:
LGTM!The function correctly fetches the previous outputs and txouts for the given SACPs.
Line range hint
622-634:
LGTM!The function correctly signs the spend transaction with the given inputs and script.
Line range hint
733-813:
LGTM!The function correctly validates spend, send, and SACP requests.
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | ||
| utxos, err = w.indexer.GetUTXOs(ctx, w.address) | ||
| return err | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error {
- utxos, err = w.indexer.GetUTXOs(ctx, w.address)
- return err
- })
+ utxos, err = w.indexer.GetUTXOs(c, w.address)
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | |
| utxos, err = w.indexer.GetUTXOs(ctx, w.address) | |
| return err | |
| }) | |
| utxos, err = w.indexer.GetUTXOs(c, w.address) | |
| if err != nil { | |
| return err | |
| } |
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | ||
| return w.indexer.SubmitTx(ctx, tx) | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error {
- return w.indexer.SubmitTx(ctx, tx)
- })
+ err = w.indexer.SubmitTx(c, tx)
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | |
| return w.indexer.SubmitTx(ctx, tx) | |
| }) | |
| err = w.indexer.SubmitTx(c, tx) | |
| if err != nil { | |
| return err | |
| } |
| func (w *batcherWallet) buildCPFPTx(c context.Context, utxos []UTXO, spendRequests []SpendRequest, sendRequests []SendRequest, sacps [][]byte, sequencesMap map[string]uint32, fee, feeOverhead, feeRate int, depth int) (*wire.MsgTx, error) { | ||
| // Check recursion depth to prevent infinite loops | ||
| // 1 depth is optimal for most cases | ||
| if depth < 0 { |
There was a problem hiding this comment.
Fix the recursion depth check.
The recursion depth check might cause unintended behavior. Consider changing the condition to depth <= 0.
- if depth < 0 {
+ if depth <= 0 {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if depth < 0 { | |
| if depth <= 0 { |
btc/cpfp.go
Outdated
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | ||
| return w.cache.ConfirmBatchStatuses(ctx, confirmedTxs, false, CPFP) | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error {
- return w.cache.ConfirmBatchStatuses(ctx, confirmedTxs, false, CPFP)
- })
+ err = w.cache.ConfirmBatchStatuses(c, confirmedTxs, false, CPFP)
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | |
| return w.cache.ConfirmBatchStatuses(ctx, confirmedTxs, false, CPFP) | |
| }) | |
| err = w.cache.ConfirmBatchStatuses(c, confirmedTxs, false, CPFP) | |
| if err != nil { | |
| return err | |
| } |
btc/cpfp.go
Outdated
| return w.cache.UpdateBatchFees(ctx, pendingTxs, int64(requiredFeeRate)) | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error {
- return w.cache.UpdateBatchFees(ctx, pendingTxs, int64(requiredFeeRate))
- })
+ err = w.cache.UpdateBatchFees(c, pendingTxs, int64(requiredFeeRate))
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return w.cache.UpdateBatchFees(ctx, pendingTxs, int64(requiredFeeRate)) | |
| }) | |
| err = w.cache.UpdateBatchFees(c, pendingTxs, int64(requiredFeeRate)) | |
| if err != nil { | |
| return err | |
| } |
btc/cpfp.go
Outdated
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | ||
| batches, err = w.cache.ReadPendingBatches(ctx, w.opts.Strategy) | ||
| return err | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error {
- batches, err = w.cache.ReadPendingBatches(ctx, w.opts.Strategy)
- return err
- })
+ batches, err = w.cache.ReadPendingBatches(c, w.opts.Strategy)
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | |
| batches, err = w.cache.ReadPendingBatches(ctx, w.opts.Strategy) | |
| return err | |
| }) | |
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | |
| batches, err = w.cache.ReadPendingBatches(ctx, w.opts.Strategy) | |
| return err | |
| }) | |
| batches, err = w.cache.ReadPendingBatches(c, w.opts.Strategy) | |
| if err != nil { | |
| return err | |
| } |
btc/batcher.go
Outdated
| w.quit = make(chan struct{}) | ||
|
|
||
| w.logger.Info("starting batcher wallet") | ||
| w.run(ctx) |
There was a problem hiding this comment.
Check the error return value of w.run.
The error return value of w.run is not checked, which may lead to unhandled errors.
- w.run(ctx)
+ if err := w.run(ctx); err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| w.run(ctx) | |
| w.run(ctx) | |
| if err := w.run(ctx); err != nil { | |
| return err | |
| } |
Tools
golangci-lint
298-298: Error return value of
w.runis not checked(errcheck)
| err = withContextTimeout(ctx, DefaultAPITimeout, func(ctx context.Context) error { | ||
| spendsUtxos, _, spendsAmount, err = getUTXOsForSpendRequest(ctx, w.indexer, spends) | ||
| return err | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var sacpsIn int64 | ||
| var sacpOut int64 | ||
| err = withContextTimeout(ctx, DefaultAPITimeout, func(ctx context.Context) error { | ||
| sacpsIn, sacpOut, err = getSACPAmounts(ctx, sacps, w.indexer) | ||
| return err | ||
| }) |
There was a problem hiding this comment.
Define timeout value as a global constant.
The timeout value of 5s is used consistently across all timeout contexts. Defining this value as a global constant will improve code maintainability and readability.
- err = withContextTimeout(ctx, 5*time.Second, func(ctx context.Context) error {
+ err = withContextTimeout(ctx, DefaultAPITimeout, func(ctx context.Context) error {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(ctx, DefaultAPITimeout, func(ctx context.Context) error { | |
| spendsUtxos, _, spendsAmount, err = getUTXOsForSpendRequest(ctx, w.indexer, spends) | |
| return err | |
| }) | |
| if err != nil { | |
| return err | |
| } | |
| var sacpsIn int64 | |
| var sacpOut int64 | |
| err = withContextTimeout(ctx, DefaultAPITimeout, func(ctx context.Context) error { | |
| sacpsIn, sacpOut, err = getSACPAmounts(ctx, sacps, w.indexer) | |
| return err | |
| }) | |
| err = withContextTimeout(ctx, DefaultAPITimeout, func(ctx context.Context) error { | |
| spendsUtxos, _, spendsAmount, err = getUTXOsForSpendRequest(ctx, w.indexer, spends) | |
| return err | |
| }) | |
| if err != nil { | |
| return err | |
| } | |
| var sacpsIn int64 | |
| var sacpOut int64 | |
| err = withContextTimeout(ctx, DefaultAPITimeout, func(ctx context.Context) error { | |
| sacpsIn, sacpOut, err = getSACPAmounts(ctx, sacps, w.indexer) | |
| return err | |
| }) |
btc/batcher.go
Outdated
| if err := w.createBatch(); err != nil { | ||
| if !errors.Is(err, ErrBatchParametersNotMet) { | ||
| w.logger.Error("failed to create batch", zap.Error(err)) | ||
| } else { | ||
| w.logger.Info("waiting for new batch") | ||
| } | ||
|
|
||
| if err := w.updateFeeRate(); err != nil { | ||
| if !errors.Is(err, ErrFeeUpdateNotNeeded) { | ||
| w.logger.Error("failed to update fee rate", zap.Error(err)) | ||
| } else { | ||
| w.logger.Info("fee update skipped") | ||
| } | ||
| } else { | ||
| w.logger.Info("batch fee updated", zap.String("strategy", string(w.opts.Strategy))) | ||
| } | ||
| } else { | ||
| w.logger.Info("new batch created", zap.String("strategy", string(w.opts.Strategy))) | ||
| } |
There was a problem hiding this comment.
Avoid potential data corruption when calling updateFeeRate.
createBatch might fail for different reasons. Calling updateFeeRate on error might lead to corruption of data.
- if err := w.createBatch(); err != nil {
- if !errors.Is(err, ErrBatchParametersNotMet) {
- w.logger.Error("failed to create batch", zap.Error(err))
- } else {
- w.logger.Info("waiting for new batch")
- }
- if err := w.updateFeeRate(); err != nil {
- if !errors.Is(err, ErrFeeUpdateNotNeeded) {
- w.logger.Error("failed to update fee rate", zap.Error(err))
- } else {
- w.logger.Info("fee update skipped")
- }
- } else {
- w.logger.Info("batch fee updated", zap.String("strategy", string(w.opts.Strategy)))
- }
- } else {
- w.logger.Info("new batch created", zap.String("strategy", string(w.opts.Strategy)))
- }
+ err := w.createBatch()
+ if err != nil && !errors.Is(err, ErrBatchParametersNotMet) {
+ w.logger.Error("failed to create batch", zap.Error(err))
+ return
+ }
+ if err == nil {
+ w.logger.Info("new batch created", zap.String("strategy", string(w.opts.Strategy)))
+ return
+ }
+ w.logger.Info("waiting for new batch")
+ if err := w.updateFeeRate(); err != nil {
+ if !errors.Is(err, ErrFeeUpdateNotNeeded) {
+ w.logger.Error("failed to update fee rate", zap.Error(err))
+ } else {
+ w.logger.Info("fee update skipped")
+ }
+ } else {
+ w.logger.Info("batch fee updated", zap.String("strategy", string(w.opts.Strategy)))
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := w.createBatch(); err != nil { | |
| if !errors.Is(err, ErrBatchParametersNotMet) { | |
| w.logger.Error("failed to create batch", zap.Error(err)) | |
| } else { | |
| w.logger.Info("waiting for new batch") | |
| } | |
| if err := w.updateFeeRate(); err != nil { | |
| if !errors.Is(err, ErrFeeUpdateNotNeeded) { | |
| w.logger.Error("failed to update fee rate", zap.Error(err)) | |
| } else { | |
| w.logger.Info("fee update skipped") | |
| } | |
| } else { | |
| w.logger.Info("batch fee updated", zap.String("strategy", string(w.opts.Strategy))) | |
| } | |
| } else { | |
| w.logger.Info("new batch created", zap.String("strategy", string(w.opts.Strategy))) | |
| } | |
| err := w.createBatch() | |
| if err != nil && !errors.Is(err, ErrBatchParametersNotMet) { | |
| w.logger.Error("failed to create batch", zap.Error(err)) | |
| return | |
| } | |
| if err == nil { | |
| w.logger.Info("new batch created", zap.String("strategy", string(w.opts.Strategy))) | |
| return | |
| } | |
| w.logger.Info("waiting for new batch") | |
| if err := w.updateFeeRate(); err != nil { | |
| if !errors.Is(err, ErrFeeUpdateNotNeeded)) { | |
| w.logger.Error("failed to update fee rate", zap.Error(err)) | |
| } else { | |
| w.logger.Info("fee update skipped") | |
| } | |
| } else { | |
| w.logger.Info("batch fee updated", zap.String("strategy", string(w.opts.Strategy))) | |
| } |
| func NewBatcherWallet(privateKey *secp256k1.PrivateKey, indexer IndexerClient, feeEstimator FeeEstimator, chainParams *chaincfg.Params, cache Cache, logger *zap.Logger, opts ...func(*batcherWallet) error) (BatcherWallet, error) { | ||
| address, err := PublicKeyAddress(chainParams, waddrmgr.WitnessPubKey, privateKey.PubKey()) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Handle potential nil pointer dereference.
If PublicKeyAddress returns an error, address will be nil, which might cause a nil pointer dereference later.
- address, err := PublicKeyAddress(chainParams, waddrmgr.WitnessPubKey, privateKey.PubKey())
+ address, err := PublicKeyAddress(chainParams, waddrmgr.WitnessPubKey, privateKey.PubKey())
+ if err != nil {
+ return nil, err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func NewBatcherWallet(privateKey *secp256k1.PrivateKey, indexer IndexerClient, feeEstimator FeeEstimator, chainParams *chaincfg.Params, cache Cache, logger *zap.Logger, opts ...func(*batcherWallet) error) (BatcherWallet, error) { | |
| address, err := PublicKeyAddress(chainParams, waddrmgr.WitnessPubKey, privateKey.PubKey()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| func NewBatcherWallet(privateKey *secp256k1.PrivateKey, indexer IndexerClient, feeEstimator FeeEstimator, chainParams *chaincfg.Params, cache Cache, logger *zap.Logger, opts ...func(*batcherWallet) error) (BatcherWallet, error) { | |
| address, err := PublicKeyAddress(chainParams, waddrmgr.WitnessPubKey, privateKey.PubKey()) | |
| if err != nil { | |
| return nil, err | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- btc/batcher.go (1 hunks)
- btc/cpfp.go (1 hunks)
- btc/rbf.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- btc/rbf.go
Additional context used
golangci-lint
btc/cpfp.go
379-379: func
reconstructCPFPBatchesis unused(unused)
btc/batcher.go
298-298: Error return value of
w.runis not checked(errcheck)
Additional comments not posted (23)
btc/cpfp.go (18)
28-31: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - requests, err = w.cache.ReadPendingRequests(ctx) - return err - }) + requests, err = w.cache.ReadPendingRequests(c) + if err != nil { + return err + }
37-39: Improve error message for missing requests.The error message
ErrBatchParametersNotMetis not descriptive enough. Consider providing a more specific error message.- return ErrBatchParametersNotMet + return errors.New("no pending requests found")
46-49: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - batches, err = w.cache.ReadPendingBatches(ctx, w.opts.Strategy) - return err - }) + batches, err = w.cache.ReadPendingBatches(c, w.opts.Strategy) + if err != nil { + return err + }
60-62: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - return w.cache.ConfirmBatchStatuses(ctx, confirmedTxs, false, CPFP) - }) + err = w.cache.ConfirmBatchStatuses(c, confirmedTxs, false, CPFP) + if err != nil { + return err + }
82-85: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - utxos, err = w.indexer.GetUTXOs(ctx, w.address) - return err - }) + utxos, err = w.indexer.GetUTXOs(c, w.address) + if err != nil { + return err + }
108-110: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - return w.indexer.SubmitTx(ctx, tx) - }) + err = w.indexer.SubmitTx(c, tx) + if err != nil { + return err + }
117-120: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - transaction, err = w.indexer.GetTx(ctx, tx.TxHash().String()) - return err - }) + transaction, err = w.indexer.GetTx(c, tx.TxHash().String()) + if err != nil { + return err + }
141-143: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - return w.cache.SaveBatch(ctx, batch) - }) + err = w.cache.SaveBatch(c, batch) + if err != nil { + return ErrSavingBatch + }
158-161: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - batches, err = w.cache.ReadPendingBatches(ctx, w.opts.Strategy) - return err - }) + batches, err = w.cache.ReadPendingBatches(c, w.opts.Strategy) + if err != nil { + return err + }
172-174: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - return w.cache.ConfirmBatchStatuses(ctx, confirmedTxs, false, CPFP) - }) + err = w.cache.ConfirmBatchStatuses(c, confirmedTxs, false, CPFP) + if err != nil { + return err + }
186-189: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - utxos, err = w.indexer.GetUTXOs(ctx, w.address) - return err - }) + utxos, err = w.indexer.GetUTXOs(c, w.address) + if err != nil { + return err + }
223-225: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - return w.indexer.SubmitTx(ctx, tx) - }) + err = w.indexer.SubmitTx(c, tx) + if err != nil { + return err + }
232-233: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - return w.cache.UpdateBatchFees(ctx, pendingTxs, int64(requiredFeeRate)) - }) + err = w.cache.UpdateBatchFees(c, pendingTxs, int64(requiredFeeRate)) + if err != nil { + return err + }
247-247: Fix the recursion depth check.The recursion depth check might cause unintended behavior. Consider changing the condition to
depth <= 0.- if depth < 0 { + if depth <= 0 {
269-272: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - spendUTXOs, spendUTXOsMap, balanceOfScripts, err = getUTXOsForSpendRequest(ctx, w.indexer, spendRequests) - return err - }) + spendUTXOs, spendUTXOsMap, balanceOfScripts, err = getUTXOsForSpendRequest(c, w.indexer, spendRequests) + if err != nil { + return nil, err + }
325-327: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - return signSpendTx(ctx, tx, signIdx, spendRequests, spendUTXOsMap, w.indexer, w.privateKey) - }) + err = signSpendTx(c, tx, signIdx, spendRequests, spendUTXOsMap, w.indexer, w.privateKey) + if err != nil { + return err + }
344-347: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - sacpsInAmount, sacpOutAmount, err = getSACPAmounts(ctx, sacps, w.indexer) - return err - }) + sacpsInAmount, sacpOutAmount, err = getSACPAmounts(c, sacps, w.indexer) + if err != nil { + return nil, err + }
379-385: Implement thereconstructCPFPBatchesfunction.The function is currently a placeholder with a TODO comment. Implement the function to ensure proper functionality.
Do you want me to help implement this function or open a GitHub issue to track this task?
Tools
golangci-lint
379-379: func
reconstructCPFPBatchesis unused(unused)
btc/batcher.go (5)
167-170: Handle potential nil pointer dereference.If
PublicKeyAddressreturns an error,addresswill benil, which might cause a nil pointer dereference later.- address, err := PublicKeyAddress(chainParams, waddrmgr.WitnessPubKey, privateKey.PubKey()) + address, err := PublicKeyAddress(chainParams, waddrmgr.WitnessPubKey, privateKey.PubKey()) + if err != nil { + return nil, err + }
266-267: Add logging for successful request saving.Consider adding a log statement after successfully saving the batch request to the cache.
return id, w.cache.SaveRequest(ctx, id, req) + if err == nil { + w.logger.Info("Batch request saved successfully", zap.String("id", id)) + }
287-288: Add logging for successful status retrieval.Consider adding a log statement after successfully retrieving the transaction status.
return tx, true, nil + w.logger.Info("Transaction status retrieved successfully", zap.String("id", id))
298-298: Check the error return value ofw.run.The error return value of
w.runis not checked, which may lead to unhandled errors.- w.run(ctx) + if err := w.run(ctx); err != nil { + return err + }Tools
golangci-lint
298-298: Error return value of
w.runis not checked(errcheck)
357-375: Avoid potential data corruption when callingupdateFeeRate.
createBatchmight fail for different reasons. CallingupdateFeeRateon error might lead to corruption of data.- if err := w.createBatch(); err != nil { - if !errors.Is(err, ErrBatchParametersNotMet) { - w.logger.Error("failed to create batch", zap.Error(err)) - } else { - w.logger.Info("waiting </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
btc/batcher.go (1)
3-19: Avoid unused imports.Ensure all imported packages are used in the code to avoid unnecessary dependencies.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- btc/batcher.go (1 hunks)
Additional context used
golangci-lint
btc/batcher.go
298-298: Error return value of
w.runis not checked(errcheck)
Additional comments not posted (32)
btc/batcher.go (32)
21-24: Define timeout value as a global constant.The timeout value of
5sis used consistently across all timeout contexts. Defining this value as a global constant will improve code maintainability and readability.
53-56: Ensure consistency in interface definitions.The
BatcherWalletinterface embedsWalletandLifecycle. Ensure that all methods are consistently defined and documented.
58-64: Ensure consistency in interface definitions.The
Lifecycleinterface defines methods for starting, stopping, and restarting the wallet service. Ensure that all methods are consistently defined and documented.
66-97: Ensure consistency in interface definitions.The
Cacheinterface defines methods for managing the state of theBatcherWallet. Ensure that all methods are consistently defined and documented.
99-107: Ensure consistency in struct definitions.The
BatcherRequeststruct is well-defined with necessary fields. Ensure that all fields are consistently defined and documented.
109-114: Ensure consistency in struct definitions.The
BatcherOptionsstruct is well-defined with necessary fields. Ensure that all fields are consistently defined and documented.
133-154: Ensure consistency in struct definitions.The
batcherWalletstruct is well-defined with necessary fields. Ensure that all fields are consistently defined and documented.
166-196: Handle potential nil pointer dereference.If
PublicKeyAddressreturns an error,addresswill benil, which might cause a nil pointer dereference later.+ if err != nil { + return nil, err + }
198-209: Ensure consistency in default options.The
defaultBatcherOptionsfunction returns default options for theBatcherWallet. Ensure that all default values are consistently defined and documented.
211-219: Ensure consistency in option parsing.The
WithStrategyfunction parses the strategy option for theBatcherWallet. Ensure that all options are consistently defined and documented.
222-227: Ensure consistency in option parsing.The
WithPTIfunction parses the PTI option for theBatcherWallet. Ensure that all options are consistently defined and documented.
229-236: Ensure consistency in strategy parsing.The
parseStrategyfunction parses the strategy for theBatcherWallet. Ensure that all strategies are consistently defined and documented.
238-240: Ensure consistency in method definitions.The
Addressmethod returns the wallet address. Ensure that all methods are consistently defined and documented.
242-244: Ensure consistency in method definitions.The
GenerateSACPmethod generates a SACP for the wallet. Ensure that all methods are consistently defined and documented.
246-248: Ensure consistency in method definitions.The
SignSACPTxmethod signs a SACP transaction for the wallet. Ensure that all methods are consistently defined and documented.
250-267: Add logging for successful request saving.Consider adding a log statement after successfully saving the batch request to the cache.
+ if err == nil { + w.logger.Info("Batch request saved successfully", zap.String("id", id)) + }
269-288: Add logging for successful status retrieval.Consider adding a log statement after successfully retrieving the transaction status.
+ w.logger.Info("Transaction status retrieved successfully", zap.String("id", id))
290-300: Check the error return value ofw.run.The error return value of
w.runis not checked, which may lead to unhandled errors.- w.run(ctx) + if err := w.run(ctx); err != nil { + return err + }Tools
golangci-lint
298-298: Error return value of
w.runis not checked(errcheck)
302-318: Ensure consistency in lifecycle methods.The
Stopmethod gracefully stops the batcher wallet service. Ensure that all lifecycle methods are consistently defined and documented.
320-326: Ensure consistency in lifecycle methods.The
Restartmethod restarts the batcher wallet service. Ensure that all lifecycle methods are consistently defined and documented.
328-341: Ensure consistency in batching methods.The
runmethod starts the batcher based on the strategy. Ensure that all batching methods are consistently defined and documented.
343-364: Ensure consistency in batching methods.The
runPTIBatchermethod triggers the batching process at regular intervals. Ensure that all batching methods are consistently defined and documented.
366-391: Avoid potential data corruption when callingupdateFeeRate.
createBatchmight fail for different reasons. CallingupdateFeeRateon error might lead to corruption of data.+ return + }
393-412: Ensure consistency in fee update methods.The
updateFeeRatemethod updates the fee rate based on the strategy. Ensure that all fee update methods are consistently defined and documented.
414-428: Ensure consistency in batch creation methods.The
createBatchmethod creates a batch based on the strategy. Ensure that all batch creation methods are consistently defined and documented.
430-491: Add logging for successful batch request validation.Consider adding a log statement after successfully validating the batch request.
+ w.logger.Info("Batch request validated successfully")
493-508: Ensure consistency in fee validation methods.The
validateUpdatemethod verifies if the fee rate delta is within the threshold. Ensure that all fee validation methods are consistently defined and documented.
510-522: Ensure consistency in fee selection methods.The
selectFeemethod selects the fee rate based on the fee level option. Ensure that all fee selection methods are consistently defined and documented.
524-543: Ensure consistency in batch filtering methods.The
filterPendingBatchesmethod filters pending batches based on their status. Ensure that all batch filtering methods are consistently defined and documented.
545-549: Ensure consistency in context timeout methods.The
withContextTimeoutmethod applies a timeout to a context. Ensure that all context timeout methods are consistently defined and documented.
551-566: Ensure consistency in request unpacking methods.The
unpackBatcherRequestsmethod unpacks batcher requests into spend requests, send requests, and SACPs. Ensure that all request unpacking methods are consistently defined and documented.
26-47: Ensure consistency in error definitions.The error variables and functions are well-defined. Ensure that all error definitions are consistently defined and documented.
There was a problem hiding this comment.
Actionable comments posted: 42
Outside diff range, codebase verification and nitpick comments (2)
btc/wallet_test.go (1)
1207-1207: Update the function parameter documentation.The function
generateSACPnow usesfaucetinstead ofwalletas the first parameter. Ensure the function documentation reflects this change.- // generateSACP generates an SACP transaction with single input and output. - // It uses wallet's address as recipient and a script as the sender. + // generateSACP generates an SACP transaction with single input and output. + // It uses faucet's address as recipient and a script as the sender.btc/wallet.go (1)
279-279: Function signature mismatch found ingenerateSACPusage.The
generateSACPfunction has been updated to include a context parameter, but the following locations still use the old signature:
btc/wallet_test.go:
- Line 1:
sacp, err := generateSACP(faucet, chainParams, privateKey, wallet.Address(), 10000, 1000)- Line 2:
sacp1, err := generateSACP(bobWallet, chainParams, bobPk, wallet.Address(), 5000, 0)- Line 3:
sacp2, err := generateSACP(faucet, chainParams, privateKey, bobWallet.Address(), 10000, 1000)- Line 4:
sacp1, err := generateSACP(bobWallet, chainParams, bobPk, wallet.Address(), 10000, 100)- Line 5:
sacp, err := generateSACP(faucet, chainParams, privateKey, randAddr, 10000, 100)- Line 6:
sacp1, err := generateSACP(faucet, chainParams, privateKey, randAddr, sendAmount, 0)- Line 7:
sacp2, err := generateSACP(faucet, chainParams, privateKey, bobAddr, sendAmount, 1000)- Line 8:
sacp, err := generateSACP(faucet, chainParams, privateKey, wallet.Address(), 1000, 1)
btc/rbf_test.go:
- Line 1:
sacp, err = generateSACP(faucet, *chainParams, privateKey, randAddr, 10000, 100)Please update these instances to match the new function signature.
Analysis chain
LGTM! But verify the function usage in the codebase.
The code changes are approved.
However, ensure that all function calls to
generateSACPmatch the new signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `generateSACP` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go -A 5 $'generateSACP'Length of output: 4763
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (17)
- btc/batcher.go (1 hunks)
- btc/batcher_test.go (1 hunks)
- btc/client.go (1 hunks)
- btc/client_test.go (1 hunks)
- btc/cpfp.go (1 hunks)
- btc/cpfp_test.go (1 hunks)
- btc/fee_test.go (3 hunks)
- btc/htlc.go (5 hunks)
- btc/htlc_test.go (4 hunks)
- btc/rbf.go (1 hunks)
- btc/rbf_test.go (1 hunks)
- btc/scripts_test.go (3 hunks)
- btc/store.go (1 hunks)
- btc/store_test.go (1 hunks)
- btc/wallet.go (22 hunks)
- btc/wallet_test.go (44 hunks)
- go.mod (4 hunks)
Files skipped from review due to trivial changes (3)
- btc/client.go
- btc/client_test.go
- btc/scripts_test.go
Files skipped from review as they are similar to previous changes (5)
- btc/batcher.go
- btc/batcher_test.go
- btc/cpfp_test.go
- btc/rbf_test.go
- go.mod
Additional context used
Learnings (3)
btc/htlc_test.go (5)
Learnt from: yanivas PR: catalogfi/blockchain#12 File: btc/htlc.go:19-32 Timestamp: 2024-07-03T12:23:25.339Z Learning: When generating unit tests for the `HTLCWallet` interface in `btc/htlc.go`, use a mock implementation of the `Wallet` interface to test the methods.Learnt from: yanivas PR: catalogfi/blockchain#12 File: btc/htlc.go:19-32 Timestamp: 2024-07-03T12:23:25.339Z Learning: When generating unit tests for the `RedeemLeaf`, `InstantRefundLeaf`, and `toLeaf` functions in `btc/htlc.go`, ensure to cover various scenarios including valid inputs, empty inputs, and error cases.Learnt from: yanivas PR: catalogfi/blockchain#12 File: btc/htlc.go:19-32 Timestamp: 2024-07-03T12:23:25.339Z Learning: When generating unit tests for the `TxInputRequest` and `TxOutputRequest` structs in `btc/wallet.go`, ensure to validate the fields' values.Learnt from: yanivas PR: catalogfi/blockchain#12 File: btc/htlc.go:19-32 Timestamp: 2024-07-03T12:20:39.303Z Learning: When generating unit tests for the `RedeemLeaf` function in `btc/htlc.go`, ensure to cover various scenarios including valid inputs, empty `redeemerPubkey`, and empty `secretHash`.Learnt from: Revantark PR: catalogfi/blockchain#20 File: btc/htlc.go:0-0 Timestamp: 2024-07-15T05:16:35.525Z Learning: The ineffectual assignment to `err` in the `GenerateInstantRefundSACP` function in `btc/htlc.go` was fixed in commit `e0b37abcf9c6f20c2e9e762b313da4c346926a4f`.btc/htlc.go (2)
Learnt from: Revantark PR: catalogfi/blockchain#20 File: btc/htlc.go:0-0 Timestamp: 2024-07-15T05:16:35.525Z Learning: The ineffectual assignment to `err` in the `GenerateInstantRefundSACP` function in `btc/htlc.go` was fixed in commit `e0b37abcf9c6f20c2e9e762b313da4c346926a4f`.Learnt from: yanivas PR: catalogfi/blockchain#12 File: btc/htlc.go:19-32 Timestamp: 2024-07-03T12:23:25.339Z Learning: When generating unit tests for the `RedeemLeaf`, `InstantRefundLeaf`, and `toLeaf` functions in `btc/htlc.go`, ensure to cover various scenarios including valid inputs, empty inputs, and error cases.btc/wallet_test.go (1)
Learnt from: yanivas PR: catalogfi/blockchain#12 File: btc/htlc.go:19-32 Timestamp: 2024-07-03T12:23:25.339Z Learning: When generating unit tests for the `HTLCWallet` interface in `btc/htlc.go`, use a mock implementation of the `Wallet` interface to test the methods.
golangci-lint
btc/store.go
381-381: func
(*BatcherCache).saveLatestBatchis unused(unused)
btc/cpfp.go
377-377: func
reconstructCPFPBatchesis unused(unused)
btc/rbf.go
712-712: func
getSelfUtxosis unused(unused)
Additional comments not posted (77)
btc/htlc_test.go (8)
Line range hint
48-58: LGTM!The test case correctly verifies that no errors occur during the generation of an HTLC address.
Line range hint
68-78: LGTM!The test case correctly verifies that no errors occur during the initiation of an HTLC.
Line range hint
88-118: LGTM!The test case correctly verifies that no errors occur during the initiation and redemption of an HTLC.
Line range hint
128-148: LGTM!The test case correctly verifies that no errors occur during the initiation and refund of an HTLC.
Line range hint
158-198: LGTM!The test case correctly verifies that no errors occur during the initiation and instant refund of an HTLC.
Line range hint
208-238: LGTM!The test case correctly verifies that an instant refund fails if the SACP is not correct.
Line range hint
248-278: LGTM!The test case correctly verifies that an instant refund succeeds even if the fee is low.
Line range hint
288-318: LGTM!The test case correctly verifies that multiple initiates can be redeemed or refunded.
btc/store_test.go (16)
54-67: LGTM!The test case correctly verifies the SaveBatch functionality.
68-75: LGTM!The test case correctly verifies that a batch that already exists is not saved again.
76-94: LGTM!The test case correctly verifies that a finalized batch is saved correctly.
99-112: LGTM!The test case correctly verifies the ReadBatchByReqId functionality.
117-136: LGTM!The test case correctly verifies the ReadPendingBatches functionality.
140-156: LGTM!The test case correctly verifies the ReadLatestBatch functionality.
160-172: LGTM!The test case correctly verifies the UpdateBatches functionality.
173-192: LGTM!The test case correctly verifies that all pending requests are removed if a batch is confirmed.
193-202: LGTM!The test case correctly verifies that a batch that does not exist can be created.
206-224: LGTM!The test case correctly verifies the UpdateAndDeletePendingBatches functionality.
226-229: LGTM!The test case correctly verifies that an error is returned if there is nothing to update.
231-239: LGTM!The test case correctly verifies that a batch that does not exist can be created.
241-278: LGTM!The test case correctly verifies that all pending batches and requests are deleted.
282-303: LGTM!The test case correctly verifies the DeletePendingBatches functionality.
307-321: LGTM!The test case correctly verifies the ReadRequests functionality.
325-347: LGTM!The test case correctly verifies the ReadPendingRequests functionality.
btc/htlc.go (6)
Line range hint
138-158: LGTM!The function correctly generates the SACP transaction needed for instant refunds.
247-253: LGTM!The function correctly performs an instant refund given the counterparty signed SACP transaction.
282-284: LGTM!The function correctly performs a refund of the HTLC if it is expired.
Line range hint
417-421: LGTM!The function correctly creates a new instance of
htlcTapLeaves.
443-445: LGTM!The function correctly retrieves the control block for a given HTLC and leaf.
Line range hint
358-398: LGTM!The function correctly validates the instant refund SACP transaction.
btc/store.go (14)
20-28: LGTM!The
serializableSpendRequesttype is correctly defined.
30-34: LGTM!The
serializableSendRequesttype is correctly defined.
36-43: LGTM!The
serializableBatcherRequesttype is correctly defined.
119-125: LGTM!The
serializableBatchtype is correctly defined.
45-73: LGTM!The
serializeBatcherRequestfunction correctly serializes aBatcherRequestto a byte slice.
75-117: LGTM!The
deserializeBatcherRequestfunction correctly deserializes a byte slice to aBatcherRequest.
127-141: LGTM!The
serializeBatchfunction correctly serializes aBatchto a byte slice.
143-163: LGTM!The
deserializeBatchfunction correctly deserializes a byte slice to aBatch.
172-178: LGTM!The
NewBatcherCachefunction correctly initializes aBatcherCachewith the given database and strategy.
180-189: LGTM!The
getfunction correctly retrieves data from the LevelDB database and handles errors.
191-197: LGTM!The
ReadBatchByReqIDfunction correctly reads a batch by request ID from the cache.
199-216: LGTM!The
searchBatchfunction correctly searches for a batch by ID in the pending and finalized batches.
218-233: LGTM!The
ReadPendingBatchesfunction correctly reads all pending batches from the cache and handles errors.
235-244: LGTM!The
ReadLatestBatchfunction correctly reads the latest batch from the cache and handles errors.btc/wallet_test.go (1)
1207-1212: Verify the mining of Bitcoin blocks.Ensure that the
localnet.MineBitcoinBlocksfunction correctly mines the required number of blocks.Verification successful
Verified the mining of Bitcoin blocks.
The
localnet.MineBitcoinBlocksfunction inlocalnet/localnet.gocorrectly mines the required number of blocks and handles errors appropriately.
- The function retrieves the current block height.
- It generates the specified number of blocks.
- It checks the new block height to ensure the blocks are mined.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the mining of Bitcoin blocks. # Test: Search for the function usage. Expect: Correct mining of blocks. rg --type go -A 5 $'localnet.MineBitcoinBlocks'Length of output: 4908
Script:
#!/bin/bash # Description: Locate the implementation of the localnet.MineBitcoinBlocks function. # Search for the function definition in Go files. rg --type go 'func MineBitcoinBlocks' -A 10Length of output: 645
btc/cpfp.go (9)
37-39: Improve error message for missing requests.The error message
ErrBatchParametersNotMetis not descriptive enough. Consider providing a more specific error message.- return ErrBatchParametersNotMet + return errors.New("no pending requests found")Likely invalid or redundant comment.
108-110: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - return w.indexer.SubmitTx(ctx, tx) - }) + err = w.indexer.SubmitTx(c, tx) + if err != nil { + return err + }Likely invalid or redundant comment.
245-245: Fix the recursion depth check.The recursion depth check might cause unintended behavior. Consider changing the condition to
depth <= 0.- if depth < 0 { + if depth <= 0 {Likely invalid or redundant comment.
178-181: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - utxos, err = w.indexer.GetUTXOs(ctx, w.address) - return err - }) + utxos, err = w.indexer.GetUTXOs(c, w.address) + if err != nil { + return err + }Likely invalid or redundant comment.
267-270: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - spendUTXOs, spendUTXOsMap, balanceOfScripts, err = getUTXOsForSpendRequest(ctx, w.indexer, spendRequests) - return err - }) + spendUTXOs, spendUTXOsMap, balanceOfScripts, err = getUTXOsForSpendRequest(c, w.indexer, spendRequests) + if err != nil { + return nil, err + }Likely invalid or redundant comment.
133-135: Avoid nested context timeouts.Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - return w.cache.SaveBatch(ctx, batch) - }) + err = w.cache.SaveBatch(c, batch) + if err != nil { + return ErrSavingBatch + }Likely invalid or redundant comment.
241-372: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
buildCPFPTxare correctly implemented.Verification successful
Function usage verified successfully.
All calls to the
buildCPFPTxfunction in the codebase match the new implementation and provide the necessary arguments.
btc/cpfp.go: Multiple calls tobuildCPFPTxwith appropriate parameters.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `buildCPFPTx` match the new implementation. # Test: Search for the function usage. Expect: Only occurances of the new implementation. rg --type go -A 5 $'buildCPFPTx'Length of output: 1428
145-239: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
updateCPFPare correctly implemented.Verification successful
Function usage verified successfully
The function
updateCPFPis correctly implemented and used in the codebase.
btc/batcher.go: The call toupdateCPFPmatches the new implementation and correctly passes the required parameters.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `updateCPFP` match the new implementation. # Test: Search for the function usage. Expect: Only occurances of the new implementation. rg --type go -A 5 $'updateCPFP'Length of output: 676
Script:
#!/bin/bash # Extract more lines around the call to `updateCPFP` in `btc/batcher.go` to verify the function usage in detail. rg --type go -A 10 -B 10 $'updateCPFP' btc/batcher.goLength of output: 512
22-142: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
createCPFPBatchare correctly implemented.Verification successful
The function
createCPFPBatchis defined inbtc/cpfp.goand is called inbtc/batcher.go. We need to ensure that the function call inbtc/batcher.gomatches the new implementation ofcreateCPFPBatch.
To ensure the function call to
createCPFPBatchis correctly implemented, we need to inspect the full context of the function where it is called inbtc/batcher.go.
Function call to
createCPFPBatchis correctly implementedThe function call to
createCPFPBatchinbtc/batcher.gois correctly implemented within thecreateBatchfunction and matches the new implementation.
btc/batcher.go:
- The
createBatchfunction callscreateCPFPBatchbased on the strategy set in the wallet's options.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `createCPFPBatch` match the new implementation. # Test: Search for the function usage. Expect: Only occurances of the new implementation. rg --type go -A 5 $'createCPFPBatch'Length of output: 683
Script:
#!/bin/bash # Inspect the function call to `createCPFPBatch` in `btc/batcher.go` to verify it matches the new implementation. rg --type go -A 10 $'createCPFPBatch' btc/batcher.goLength of output: 422
Script:
#!/bin/bash # Search for the function definition in `btc/batcher.go` to get the complete context of the function where `createCPFPBatch` is called. rg --type go -A 20 $'func' btc/batcher.goLength of output: 10921
btc/fee_test.go (3)
68-68: LGTM!The added assertion improves the robustness of the test by explicitly checking for errors.
118-118: LGTM!The added assertion improves the robustness of the test by explicitly checking for errors.
424-424: LGTM!The added assertion improves the robustness of the test by explicitly checking for errors.
btc/rbf.go (11)
307-313: Ensure proper error wrapping.When returning errors, consider wrapping them to provide more context.
- return err + return fmt.Errorf("failed to update and delete pending batches: %w", err)Likely invalid or redundant comment.
269-271: Ensure proper error wrapping.When returning errors, consider wrapping them to provide more context.
- return w.cache.SaveBatch(ctx, batch) + return fmt.Errorf("failed to save RBF batch: %w", w.cache.SaveBatch(ctx, batch))Likely invalid or redundant comment.
546-548: Ensure proper error wrapping.When returning errors, consider wrapping them to provide more context.
- return err + return fmt.Errorf("failed to get UTXOs from indexer: %w", err)Likely invalid or redundant comment.
469-471: Ensure proper error wrapping.When returning errors, consider wrapping them to provide more context.
- return err + return fmt.Errorf("failed to get UTXOs with fee: %w", err)Likely invalid or redundant comment.
29-31: Ensure proper error wrapping.When returning errors, consider wrapping them to provide more context.
- return err + return fmt.Errorf("failed to read pending requests: %w", err)Likely invalid or redundant comment.
320-323: Ensure proper error wrapping.When returning errors, consider wrapping them to provide more context.
- return err + return fmt.Errorf("failed to validate fee rate update: %w", err)Likely invalid or redundant comment.
68-68: Ensure proper error wrapping.When returning errors, consider wrapping them to provide more context.
- return w.reSubmitRBFBatch(c, latestBatch, pendingRequests, 0) + return fmt.Errorf("re-submitting existing RBF batch: %w", w.reSubmitRBFBatch(c, latestBatch, pendingRequests, 0))Likely invalid or redundant comment.
418-419: Ensure proper error wrapping.When returning errors, consider wrapping them to provide more context.
- return signSpendTx(ctx, tx, signIdx, spendRequests, spendUTXOsMap, w.indexer, w.privateKey) + return fmt.Errorf("failed to sign spend transaction: %w", signSpendTx(ctx, tx, signIdx, spendRequests, spendUTXOsMap, w.indexer, w.privateKey))Likely invalid or redundant comment.
242-244: Ensure proper error wrapping.When returning errors, consider wrapping them to provide more context.
- return w.indexer.SubmitTx(ctx, tx) + return fmt.Errorf("failed to submit RBF transaction: %w", w.indexer.SubmitTx(ctx, tx))Likely invalid or redundant comment.
202-204: Ensure proper error wrapping.When returning errors, consider wrapping them to provide more context.
- return err + return fmt.Errorf("failed to get fee suggestion: %w", err)Likely invalid or redundant comment.
146-148: Ensure proper error wrapping.When returning errors, consider wrapping them to provide more context.
- return err + return fmt.Errorf("failed to get transaction: %w", err)Likely invalid or redundant comment.
btc/wallet.go (9)
73-74: LGTM!The error variable
ErrSCAPNoInputsFoundis correctly defined.
76-77: LGTM!The error variable
ErrSCAPInputsNotEqualOutputsis correctly defined.
399-399: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
getFeeUsedInSACPsmatch the new signature.Verification successful
Function usage matches the new signature.
The function
getFeeUsedInSACPsis correctly called with the new context parameter in the codebase.
btc/wallet.go:sacpsFee, err := getFeeUsedInSACPs(ctx, sacps, sw.indexer)Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `getFeeUsedInSACPs` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go -A 5 $'getFeeUsedInSACPs'Length of output: 748
643-643: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
signSpendTxmatch the new signature.Verification successful
All calls to
signSpendTxmatch the new function signature.The function calls in the following files correctly include the
ctxparameter:
btc/wallet.gobtc/rbf.gobtc/cpfp.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `signSpendTx` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go -A 5 $'signSpendTx'Length of output: 1438
206-206: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
Sendmatch the new signature.Verification successful
All instances of the
Sendfunction match the new signature.The function calls to
Sendhave been updated to include thecontext.Contextparameter as expected.
- Verified instances in:
btc/wallet_test.goevm/wallet_test.goevm/wallet.gobtc/rbf_test.gobtc/htlc_test.gobtc/cpfp_test.gobtc/fee_test.gobtc/htlc.gobtc/rbf.gobtc/cpfp.gobtc/client.gobtc/batcher.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `Send` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go -A 5 $'Send'Length of output: 45817
328-328: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
spendAndSendmatch the new signature.Verification successful
Function usage verification successful.
All instances of the
spendAndSendfunction inbtc/wallet.gouse the new signature with the context parameter.
btc/wallet.go:
- Line 1:
tx, err := sw.spendAndSend(ctx, sendRequests, spendRequests, sacps, sacpsFee, fee, 0)- Line 2:
return sw.spendAndSend(ctx, sendRequests, spendRequests, sacps, sacpFee, feeToBePaid, depth+1)Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `spendAndSend` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go -A 5 $'spendAndSend'Length of output: 915
417-417: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
getPrevoutsForSACPsmatch the new signature.Verification successful
Verify all function calls to
getPrevoutsForSACPsmatch the new signature.The code changes are approved. However, ensure that all function calls to
getPrevoutsForSACPsmatch the new signature.
Verified: All function calls to
getPrevoutsForSACPsmatch the new signature.The code changes are approved, and all instances of the function usage have been updated accordingly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `getPrevoutsForSACPs` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go -A 5 $'getPrevoutsForSACPs'Length of output: 874
Script:
#!/bin/bash # Description: Verify all function calls to `getPrevoutsForSACPs` match the new signature. # Test: Search for all occurrences of the function usage. Expect: Only occurrences of the new signature. rg --type go 'getPrevoutsForSACPs'Length of output: 416
249-249: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
GenerateSACPmatch the new signature.Verification successful
All function calls to
GenerateSACPmatch the new signature.The verification confirms that the function
GenerateSACPis consistently used with the updated signature across the codebase.
btc/wallet_test.gobtc/htlc.gobtc/batcher.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GenerateSACP` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go -A 5 $'GenerateSACP'Length of output: 2604
373-373: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
getSACPAmountsmatch the new signature.Verification successful
Verification complete: All calls to
getSACPAmountsmatch the new signature.All instances of the function
getSACPAmountsin the codebase include thectxparameter, confirming the update is consistent.
btc/wallet.gobtc/rbf.gobtc/cpfp.gobtc/batcher.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `getSACPAmounts` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go -A 5 $'getSACPAmounts'Length of output: 1406
| func skipFor(mode MODE, modes ...MODE) { | ||
| for _, m := range modes { | ||
| if m == mode { | ||
| Skip("Skipping test for mode: " + string(m)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure proper mode handling in skipFor.
The function skipFor should handle modes more robustly and provide meaningful messages.
- if m == mode {
- Skip("Skipping test for mode: " + string(m))
- }
+ if m == mode {
+ Skip(fmt.Sprintf("Skipping test for mode: %v", mode))
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func skipFor(mode MODE, modes ...MODE) { | |
| for _, m := range modes { | |
| if m == mode { | |
| Skip("Skipping test for mode: " + string(m)) | |
| } | |
| } | |
| } | |
| func skipFor(mode MODE, modes ...MODE) { | |
| for _, m := range modes { | |
| if m == mode { | |
| Skip(fmt.Sprintf("Skipping test for mode: %v", mode)) | |
| } | |
| } | |
| } |
| func assertSuccess(wallet btc.Wallet, tx *btc.Transaction, txid string, mode MODE) { | ||
| var ok bool | ||
| var err error | ||
| switch mode { | ||
| case SIMPLE: | ||
| *tx, ok, err = wallet.Status(context.Background(), txid) | ||
| Expect(err).To(BeNil()) | ||
| Expect(ok).Should(BeTrue()) | ||
| Expect(tx).ShouldNot(BeNil()) | ||
| case BATCHER_CPFP, BATCHER_RBF: | ||
| var ok bool | ||
| for { | ||
| fmt.Println("waiting for tx", txid) | ||
| *tx, ok, err = wallet.Status(context.Background(), txid) | ||
| Expect(err).To(BeNil()) | ||
| if ok { | ||
| Expect(tx).ShouldNot(BeNil()) | ||
| break | ||
| } | ||
| time.Sleep(1 * time.Second) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure robust error handling in assertSuccess.
The function assertSuccess should handle errors more robustly, especially in the loop for batcher modes.
- for {
- fmt.Println("waiting for tx", txid)
- *tx, ok, err = wallet.Status(context.Background(), txid)
- Expect(err).To(BeNil())
- if ok {
- Expect(tx).ShouldNot(BeNil())
- break
- }
- time.Sleep(1 * time.Second)
- }
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
+ for {
+ fmt.Println("waiting for tx", txid)
+ *tx, ok, err = wallet.Status(ctx, txid)
+ if err != nil {
+ fmt.Println("Error fetching status:", err)
+ return
+ }
+ if ok {
+ Expect(tx).ShouldNot(BeNil())
+ break
+ }
+ select {
+ case <-ctx.Done():
+ fmt.Println("Timeout waiting for tx", txid)
+ return
+ default:
+ time.Sleep(1 * time.Second)
+ }
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func assertSuccess(wallet btc.Wallet, tx *btc.Transaction, txid string, mode MODE) { | |
| var ok bool | |
| var err error | |
| switch mode { | |
| case SIMPLE: | |
| *tx, ok, err = wallet.Status(context.Background(), txid) | |
| Expect(err).To(BeNil()) | |
| Expect(ok).Should(BeTrue()) | |
| Expect(tx).ShouldNot(BeNil()) | |
| case BATCHER_CPFP, BATCHER_RBF: | |
| var ok bool | |
| for { | |
| fmt.Println("waiting for tx", txid) | |
| *tx, ok, err = wallet.Status(context.Background(), txid) | |
| Expect(err).To(BeNil()) | |
| if ok { | |
| Expect(tx).ShouldNot(BeNil()) | |
| break | |
| } | |
| time.Sleep(1 * time.Second) | |
| } | |
| } | |
| } | |
| func assertSuccess(wallet btc.Wallet, tx *btc.Transaction, txid string, mode MODE) { | |
| var ok bool | |
| var err error | |
| switch mode { | |
| case SIMPLE: | |
| *tx, ok, err = wallet.Status(context.Background(), txid) | |
| Expect(err).To(BeNil()) | |
| Expect(ok).Should(BeTrue()) | |
| Expect(tx).ShouldNot(BeNil()) | |
| case BATCHER_CPFP, BATCHER_RBF: | |
| var ok bool | |
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| defer cancel() | |
| for { | |
| fmt.Println("waiting for tx", txid) | |
| *tx, ok, err = wallet.Status(ctx, txid) | |
| if err != nil { | |
| fmt.Println("Error fetching status:", err) | |
| return | |
| } | |
| if ok { | |
| Expect(tx).ShouldNot(BeNil()) | |
| break | |
| } | |
| select { | |
| case <-ctx.Done(): | |
| fmt.Println("Timeout waiting for tx", txid) | |
| return | |
| default: | |
| time.Sleep(1 * time.Second) | |
| } | |
| } | |
| } | |
| } |
btc/rbf.go
Outdated
| latestBatch, err = w.cache.ReadLatestBatch(ctx) | ||
| return err | ||
| }) |
There was a problem hiding this comment.
Ensure proper error wrapping.
When returning errors, consider wrapping them to provide more context.
- return err
+ return fmt.Errorf("failed to read latest batch: %w", err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| latestBatch, err = w.cache.ReadLatestBatch(ctx) | |
| return err | |
| }) | |
| latestBatch, err = w.cache.ReadLatestBatch(ctx) | |
| return fmt.Errorf("failed to read latest batch: %w", err) |
| sacpsInAmount, sacpsOutAmount, err = getSACPAmounts(ctx, sacps, w.indexer) | ||
| return err | ||
| }) |
There was a problem hiding this comment.
Ensure proper error wrapping.
When returning errors, consider wrapping them to provide more context.
- return err
+ return fmt.Errorf("failed to get SACP amounts: %w", err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sacpsInAmount, sacpsOutAmount, err = getSACPAmounts(ctx, sacps, w.indexer) | |
| return err | |
| }) | |
| sacpsInAmount, sacpsOutAmount, err = getSACPAmounts(ctx, sacps, w.indexer) | |
| return fmt.Errorf("failed to get SACP amounts: %w", err) |
btc/rbf.go
Outdated
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | ||
| return w.cache.DeletePendingBatches(ctx) | ||
| }) |
There was a problem hiding this comment.
Ensure proper error wrapping.
When returning errors, consider wrapping them to provide more context.
- return w.cache.DeletePendingBatches(ctx)
+ return fmt.Errorf("failed to delete pending batch: %w", w.cache.DeletePendingBatches(ctx))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | |
| return w.cache.DeletePendingBatches(ctx) | |
| }) | |
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | |
| return fmt.Errorf("failed to delete pending batch: %w", w.cache.DeletePendingBatches(ctx)) | |
| }) |
btc/cpfp.go
Outdated
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | ||
| return w.cache.UpdateBatches(ctx, confirmedTxs...) | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error {
- return w.cache.UpdateBatches(ctx, confirmedTxs...)
- })
+ err = w.cache.UpdateBatches(c, confirmedTxs...)
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | |
| return w.cache.UpdateBatches(ctx, confirmedTxs...) | |
| }) | |
| err = w.cache.UpdateBatches(c, confirmedTxs...) | |
| if err != nil { | |
| return err | |
| } |
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | ||
| return w.indexer.SubmitTx(ctx, tx) | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error {
- return w.indexer.SubmitTx(ctx, tx)
- })
+ err = w.indexer.SubmitTx(c, tx)
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | |
| return w.indexer.SubmitTx(ctx, tx) | |
| }) | |
| err = w.indexer.SubmitTx(c, tx) | |
| if err != nil { | |
| return err | |
| } |
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | ||
| sacpsInAmount, sacpOutAmount, err = getSACPAmounts(ctx, sacps, w.indexer) | ||
| return err | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error {
- sacpsInAmount, sacpOutAmount, err = getSACPAmounts(ctx, sacps, w.indexer)
- return err
- })
+ sacpsInAmount, sacpOutAmount, err = getSACPAmounts(c, sacps, w.indexer)
+ if err != nil {
+ return nil, err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | |
| sacpsInAmount, sacpOutAmount, err = getSACPAmounts(ctx, sacps, w.indexer) | |
| return err | |
| }) | |
| sacpsInAmount, sacpOutAmount, err = getSACPAmounts(c, sacps, w.indexer) | |
| if err != nil { | |
| return nil, err | |
| } |
btc/cpfp.go
Outdated
| // Update the fee of all batches that got bumped | ||
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | ||
| newBatches := []Batch{} | ||
| for _, batch := range pendingBatches { | ||
| batch.Tx.Fee = int64(requiredFeeRate) * int64(batch.Tx.Weight) / blockchain.WitnessScaleFactor | ||
| newBatches = append(newBatches, batch) | ||
| } | ||
|
|
||
| return w.cache.UpdateBatches(ctx, newBatches...) | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error {
- newBatches := []Batch{}
- for _, batch := range pendingBatches {
- batch.Tx.Fee = int64(requiredFeeRate) * int64(batch.Tx.Weight) / blockchain.WitnessScaleFactor
- newBatches = append(newBatches, batch)
- }
- return w.cache.UpdateBatches(ctx, newBatches...)
- })
+ newBatches := []Batch{}
+ for _, batch := range pendingBatches {
+ batch.Tx.Fee = int64(requiredFeeRate) * int64(batch.Tx.Weight) / blockchain.WitnessScaleFactor
+ newBatches = append(newBatches, batch)
+ }
+ err = w.cache.UpdateBatches(c, newBatches...)
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Update the fee of all batches that got bumped | |
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | |
| newBatches := []Batch{} | |
| for _, batch := range pendingBatches { | |
| batch.Tx.Fee = int64(requiredFeeRate) * int64(batch.Tx.Weight) / blockchain.WitnessScaleFactor | |
| newBatches = append(newBatches, batch) | |
| } | |
| return w.cache.UpdateBatches(ctx, newBatches...) | |
| }) | |
| // Update the fee of all batches that got bumped | |
| newBatches := []Batch{} | |
| for _, batch := range pendingBatches { | |
| batch.Tx.Fee = int64(requiredFeeRate) * int64(batch.Tx.Weight) / blockchain.WitnessScaleFactor | |
| newBatches = append(newBatches, batch) | |
| } | |
| err = w.cache.UpdateBatches(c, newBatches...) | |
| if err != nil { | |
| return err | |
| } |
btc/cpfp.go
Outdated
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | ||
| batches, err = w.cache.ReadPendingBatches(ctx) | ||
| return err | ||
| }) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts might cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error {
- batches, err = w.cache.ReadPendingBatches(ctx)
- return err
- })
+ batches, err = w.cache.ReadPendingBatches(c)
+ if err != nil {
+ return err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | |
| batches, err = w.cache.ReadPendingBatches(ctx) | |
| return err | |
| }) | |
| batches, err = w.cache.ReadPendingBatches(c) | |
| if err != nil { | |
| return err | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 19
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- btc/batcher.go (1 hunks)
- btc/cpfp.go (1 hunks)
- btc/rbf.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- btc/batcher.go
Additional context used
golangci-lint
btc/cpfp.go
363-363: func
reconstructCPFPBatchesis unused(unused)
Additional comments not posted (6)
btc/cpfp.go (6)
409-420: LGTM!The function
removeDoubleSpendsappears to be implemented correctly.
101-106: Avoid nested context timeouts.Nested context timeouts can cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - return w.indexer.SubmitTx(ctx, tx) - }) - if err != nil { - return err - } + err = w.indexer.SubmitTx(c, tx) + if err != nil { + return fmt.Errorf("failed to submit CPFP transaction: %w", err) + }Likely invalid or redundant comment.
253-259: Avoid nested context timeouts.Nested context timeouts can cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { - spendUTXOs, spendUTXOsMap, balanceOfScripts, err = getUTXOsForSpendRequest(ctx, w.indexer, spendRequests) - return err - }) - if err != nil { - return nil, err - } + spendUTXOs, spendUTXOsMap, balanceOfScripts, err = getUTXOsForSpendRequest(c, w.indexer, spendRequests) + if err != nil { + return nil, fmt.Errorf("failed to get UTXOs for spend request: %w", err) + }Likely invalid or redundant comment.
41-45: Avoid nested context timeouts.Nested context timeouts can cause unintended delays. Consider refactoring to reduce nesting.
- batches, err := w.cache.ReadPendingBatches(c) - if err != nil { - w.logger.Error("failed to read pending batches", zap.Error(err)) - return err - } + batches, err := w.cache.ReadPendingBatches(c) + if err != nil { + return fmt.Errorf("failed to read pending batches: %w", err) + }Likely invalid or redundant comment.
140-144: Avoid nested context timeouts.Nested context timeouts can cause unintended delays. Consider refactoring to reduce nesting.
- batches, err := w.cache.ReadPendingBatches(c) - if err != nil { - w.logger.Error("failed to read pending batches", zap.Error(err)) - return err - } + batches, err = w.cache.ReadPendingBatches(c) + if err != nil { + return fmt.Errorf("failed to read pending batches: %w", err) + }Likely invalid or redundant comment.
126-130: Avoid nested context timeouts.Nested context timeouts can cause unintended delays. Consider refactoring to reduce nesting.
- err = w.cache.SaveBatch(c, batch) - if err != nil { - w.logger.Error("failed to save CPFP batch", zap.Error(err)) - return ErrSavingBatch - } + err = w.cache.SaveBatch(c, batch) + if err != nil { + return fmt.Errorf("failed to save CPFP batch: %w", err) + }Likely invalid or redundant comment.
| func reconstructCPFPBatches([]Batch, Batch, btcutil.Address) error { | ||
| // TODO: Verify that the trailing batch can trace back to the funding UTXOs from the wallet address | ||
| // This is essential to ensure that all the pending transactions are moved to the estimated | ||
| // fee rate and the trailing batch is the only one that needs to be bumped | ||
| // Current implementation assumes that the trailing batch is the last batch in the list | ||
| // It maintains only one thread of CPFP transactions | ||
| return nil |
There was a problem hiding this comment.
Implement the reconstructCPFPBatches function.
The function is currently a placeholder with a TODO comment. Implement the function to ensure proper functionality.
Do you want me to help implement this function or open a GitHub issue to track this task?
Tools
golangci-lint
363-363: func
reconstructCPFPBatchesis unused(unused)
| // if !utxo.Status.Confirmed { | ||
| // ucUtxos = append(ucUtxos, utxo) | ||
| // } |
There was a problem hiding this comment.
Ensure proper error wrapping.
When returning errors, consider wrapping them to provide more context.
- return err
+ return fmt.Errorf("failed to get raw transaction bytes: %w", err)Committable suggestion was skipped due to low confidence.
| func (w *batcherWallet) createCPFPBatch(c context.Context) error { | ||
|
|
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts can cause unintended delays. Consider refactoring to reduce nesting.
- requests, err := w.cache.ReadPendingRequests(c)
- if err != nil {
- w.logger.Error("failed to read pending requests", zap.Error(err))
- return err
- }
+ requests, err := w.cache.ReadPendingRequests(c)
+ if err != nil {
+ return fmt.Errorf("failed to read pending requests: %w", err)
+ }Committable suggestion was skipped due to low confidence.
| err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error { | ||
| sacpsInAmount, sacpOutAmount, err = getSACPAmounts(ctx, sacps, w.indexer) | ||
| return err | ||
| }) | ||
|
|
||
| // Estimate the new fee | ||
| newFeeEstimate := (int(trueSize) * (feeRate)) + feeOverhead + bufferFee - int(sacpsInAmount-sacpOutAmount) | ||
|
|
||
| // If the new fee estimate exceeds the current fee, rebuild the CPFP transaction | ||
| if newFeeEstimate > fee+feeOverhead { | ||
| var txBytes []byte | ||
| if txBytes, err = GetTxRawBytes(tx); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Improve logging for recursion depth exceeded.
The logging should include the error context for better debugging.
- w.logger.Debug(
- ErrBuildCPFPDepthExceeded.Error(),
+ w.logger.Debug("recursion depth exceeded",Committable suggestion was skipped due to low confidence.
| totalSize := int(0) | ||
| feeDelta := int(0) | ||
|
|
||
| for _, batch := range batches { | ||
| size := batch.Tx.Weight / blockchain.WitnessScaleFactor | ||
| feeRate := int(batch.Tx.Fee) / size | ||
| if feeRate > maxFeeRate { | ||
| maxFeeRate = feeRate | ||
| } | ||
| if reqFeeRate > feeRate { | ||
| feeDelta += (reqFeeRate - feeRate) * size | ||
| } | ||
| totalSize += size |
There was a problem hiding this comment.
Avoid redundant variable declaration.
The variable totalSize is declared but not used. Consider removing it.
- totalSize := int(0)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| totalSize := int(0) | |
| feeDelta := int(0) | |
| for _, batch := range batches { | |
| size := batch.Tx.Weight / blockchain.WitnessScaleFactor | |
| feeRate := int(batch.Tx.Fee) / size | |
| if feeRate > maxFeeRate { | |
| maxFeeRate = feeRate | |
| } | |
| if reqFeeRate > feeRate { | |
| feeDelta += (reqFeeRate - feeRate) * size | |
| } | |
| totalSize += size | |
| feeDelta := int(0) | |
| for _, batch := range batches { | |
| size := batch.Tx.Weight / blockchain.WitnessScaleFactor | |
| feeRate := int(batch.Tx.Fee) / size | |
| if feeRate > maxFeeRate { | |
| maxFeeRate = feeRate | |
| } | |
| if reqFeeRate > feeRate { | |
| feeDelta += (reqFeeRate - feeRate) * size | |
| } |
| if depth < 0 { | ||
| w.logger.Debug( | ||
| ErrBuildCPFPDepthExceeded.Error(), | ||
| zap.Any("utxos", utxos), | ||
| zap.Any("spendRequests", spendRequests), | ||
| zap.Any("sendRequests", sendRequests), | ||
| zap.Any("sacps", sacps), | ||
| zap.Any("sequencesMap", sequencesMap), | ||
| zap.Int("fee", fee), | ||
| zap.Int("feeOverhead", feeOverhead), | ||
| zap.Int("feeRate", feeRate), | ||
| zap.Int("depth", depth), | ||
| ) | ||
| return nil, ErrBuildCPFPDepthExceeded | ||
| } |
There was a problem hiding this comment.
Fix the recursion depth check.
The recursion depth check might cause unintended behavior. Consider changing the condition to depth <= 0.
- if depth < 0 {
+ if depth <= 0 {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if depth < 0 { | |
| w.logger.Debug( | |
| ErrBuildCPFPDepthExceeded.Error(), | |
| zap.Any("utxos", utxos), | |
| zap.Any("spendRequests", spendRequests), | |
| zap.Any("sendRequests", sendRequests), | |
| zap.Any("sacps", sacps), | |
| zap.Any("sequencesMap", sequencesMap), | |
| zap.Int("fee", fee), | |
| zap.Int("feeOverhead", feeOverhead), | |
| zap.Int("feeRate", feeRate), | |
| zap.Int("depth", depth), | |
| ) | |
| return nil, ErrBuildCPFPDepthExceeded | |
| } | |
| if depth <= 0 { | |
| w.logger.Debug( | |
| ErrBuildCPFPDepthExceeded.Error(), | |
| zap.Any("utxos", utxos), | |
| zap.Any("spendRequests", spendRequests), | |
| zap.Any("sendRequests", sendRequests), | |
| zap.Any("sacps", sacps), | |
| zap.Any("sequencesMap", sequencesMap), | |
| zap.Int("fee", fee), | |
| zap.Int("feeOverhead", feeOverhead), | |
| zap.Int("feeRate", feeRate), | |
| zap.Int("depth", depth), | |
| ) | |
| return nil, ErrBuildCPFPDepthExceeded | |
| } |
| // Update the fee of all batches that got bumped | ||
|
|
||
| newBatches := []Batch{} | ||
| for _, batch := range pendingBatches { | ||
| batch.Tx.Fee = int64(requiredFeeRate) * int64(batch.Tx.Weight) / blockchain.WitnessScaleFactor | ||
| newBatches = append(newBatches, batch) | ||
| } | ||
| err = w.cache.UpdateBatches(c, newBatches...) | ||
| if err != nil { | ||
| w.logger.Error("failed to update CPFP batches", zap.Error(err)) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts can cause unintended delays. Consider refactoring to reduce nesting.
- err = w.cache.UpdateBatches(c, newBatches...)
- if err != nil {
- w.logger.Error("failed to update CPFP batches", zap.Error(err))
- return err
- }
+ err = w.cache.UpdateBatches(c, newBatches...)
+ if err != nil {
+ return fmt.Errorf("failed to update CPFP batches: %w", err)
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Update the fee of all batches that got bumped | |
| newBatches := []Batch{} | |
| for _, batch := range pendingBatches { | |
| batch.Tx.Fee = int64(requiredFeeRate) * int64(batch.Tx.Weight) / blockchain.WitnessScaleFactor | |
| newBatches = append(newBatches, batch) | |
| } | |
| err = w.cache.UpdateBatches(c, newBatches...) | |
| if err != nil { | |
| w.logger.Error("failed to update CPFP batches", zap.Error(err)) | |
| return err | |
| } | |
| // Update the fee of all batches that got bumped | |
| newBatches := []Batch{} | |
| for _, batch := range pendingBatches { | |
| batch.Tx.Fee = int64(requiredFeeRate) * int64(batch.Tx.Weight) / blockchain.WitnessScaleFactor | |
| newBatches = append(newBatches, batch) | |
| } | |
| err = w.cache.UpdateBatches(c, newBatches...) | |
| if err != nil { | |
| return fmt.Errorf("failed to update CPFP batches: %w", err) | |
| } |
| zap.Int("TxIns", len(tx.TxIn)), | ||
| zap.Int("TxOuts", len(tx.TxOut)), | ||
| zap.String("TxData", hex.EncodeToString(txBytes)), |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts can cause unintended delays. Consider refactoring to reduce nesting.
- err = withContextTimeout(c, DefaultAPITimeout, func(ctx context.Context) error {
- sacpsInAmount, sacpOutAmount, err = getSACPAmounts(ctx, sacps, w.indexer)
- return err
- })
- if err != nil {
- return nil, err
- }
+ sacpsInAmount, sacpOutAmount, err = getSACPAmounts(c, sacps, w.indexer)
+ if err != nil {
+ return nil, fmt.Errorf("failed to get SACP amounts: %w", err)
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| zap.Int("TxIns", len(tx.TxIn)), | |
| zap.Int("TxOuts", len(tx.TxOut)), | |
| zap.String("TxData", hex.EncodeToString(txBytes)), | |
| zap.Int("TxIns", len(tx.TxIn)), | |
| zap.Int("TxOuts", len(tx.TxOut)), | |
| zap.String("TxData", hex.EncodeToString(txBytes)), | |
| sacpsInAmount, sacpOutAmount, err = getSACPAmounts(c, sacps, w.indexer) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get SACP amounts: %w", err) | |
| } |
| latestBatch, err := w.cache.ReadLatestBatch(c) | ||
| if err != nil { | ||
| // If no batch is found, create a new RBF batch. | ||
| if err == ErrStoreNotFound { | ||
| return w.createNewRBFBatch(c, pendingRequests, 0, 0) | ||
| } | ||
| return fmt.Errorf("failed to read latest batch: %w", err) |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts can cause unintended delays. Consider refactoring to reduce nesting.
- latestBatch, err := w.cache.Read
<!-- This is an auto-generated comment by CodeRabbit -->| pendingRequests, err := w.cache.ReadPendingRequests(c) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read pending requests: %w", err) | ||
| } |
There was a problem hiding this comment.
Avoid nested context timeouts.
Nested context timeouts can cause unintended delays. Consider refactoring to reduce nesting.
- pendingRequests, err := w.cache.ReadPendingRequests(c)
- if err != nil {
- return fmt.Errorf("failed to read pending requests: %w", err)
- }
+ pendingRequests, err = w.cache.ReadPendingRequests(c)
+ if err != nil {
+ return fmt.Errorf("failed to read pending requests: %w", err)
+ }Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
Bug Fixes
Tests