[1/5]sweep: store fee info for published sweeping transactions #8147
Conversation
yyforyongyu
force-pushed
the
sweeper-store-fees
branch
from
f4fb9ce to
0e541dc
Compare
yyforyongyu
force-pushed
the
sweeper-store-fees
branch
3 times, most recently
from
e2455f6 to
8955673
Compare
yyforyongyu
force-pushed
the
sweeper-store-fees
branch
from
8955673 to
c0f91b6
Compare
WalkthroughThe update introduces significant improvements to transaction handling and logging within the system, notably through enhanced serialization/deserialization methods and the introduction of a Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
yyforyongyu
changed the title
yyforyongyu
force-pushed
the
sweeper-store-fees
branch
from
c0f91b6 to
6c786c4
Compare
yyforyongyu
changed the title
yyforyongyu
force-pushed
the
sweeper-store-fees
branch
from
6c786c4 to
821aba1
Compare
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between 2f14680 and 821aba1b18da05a87f49f9ec0703cb81d13287ae.Files selected for processing (10)
- channeldb/revocation_log.go (6 hunks)
- channeldb/revocation_log_test.go (3 hunks)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- sweep/store.go (6 hunks)
- sweep/store_mock.go (1 hunks)
- sweep/store_test.go (4 hunks)
- sweep/sweeper.go (3 hunks)
- sweep/sweeper_test.go (9 hunks)
- sweep/txgenerator.go (8 hunks)
- sweep/walletsweep.go (1 hunks)
Files skipped from review due to trivial changes (1)
- docs/release-notes/release-notes-0.18.0.md
Additional comments: 42
sweep/store_mock.go (6)
- 11-11: The
MockSweeperStorenow embedsmock.Mockdirectly, which is a good use of composition to leverage the mock package's functionality for function call tracking.- 19-23: The
IsOurTxmethod correctly uses theCalledmethod for function call tracking. This is a standard practice when using thetestify/mockpackage.- 27-29: The
StoreTxmethod implementation is correct and follows the standard pattern for usingmock.Mockto track function calls and arguments.- 34-36: The
ListSweepsmethod correctly uses theCalledmethod for function call tracking, and properly handles the return value conversion. This is consistent with best practices for using thetestify/mockpackage.- 39-49: The
GetTxmethod correctly uses theCalledmethod for function call tracking. The conditional check fornilbefore attempting to type assert the return value is a good practice to avoid panics.- 53-56: The
DeleteTxmethod implementation is correct, following the standard pattern for usingmock.Mockto track function calls and arguments.sweep/store_test.go (9)
- 17-23: The setup for the
TestStorefunction, including the creation of a test database and theSweeperStore, is correctly done using therequire.NoErrorfor error handling, which is a concise way to handle test failures.- 33-38: The use of
StoreTxwithinTestStoreto store transactions and the subsequent error checking withrequire.NoErroris correctly implemented, ensuring that the store operation does not produce an error.- 56-66: The verification of whether transactions are recognized as owned by using
IsOurTxand asserting the result withrequire.Trueis correctly implemented, ensuring that the stored transactions are correctly identified.- 71-72: The test correctly asserts that an unknown hash should not be recognized as owned by using
IsOurTxandrequire.False.- 83-87: The test for
ListSweepscorrectly asserts the length of the returned transaction list and verifies each returned transaction ID against the expected set, usingrequire.Trueffor assertion.- 94-116:
TestTxRecordcorrectly tests the serialization and deserialization ofTxRecord, ensuring that the process is reversible and the original record is recovered. The use ofrequire.NoErrorandrequire.Equalfor assertions is appropriate.- 120-147:
TestGetTxcorrectly tests the retrieval of a stored transaction record usingGetTx, including both a successful retrieval and an error case for a non-existing transaction. The use ofrequire.NoError,require.Equal, andrequire.ErrorIsfor assertions is appropriate.- 154-183:
TestGetTxCompatiblecorrectly tests the backward compatibility ofGetTxwith old transaction record formats. The use ofrequire.NoErrorandrequire.Truefor assertions is appropriate.- 186-221:
TestDeleteTxcorrectly tests the deletion of a transaction record usingDeleteTx, including both a successful deletion and a no-error case for a non-existing transaction. The use ofrequire.NoErrorandrequire.ErrorIsfor assertions is appropriate.sweep/store.go (8)
- 43-56: The definition of the
TxRecordstruct is correct and includes all necessary fields to represent a transaction record in the database. The inclusion ofTxid,FeeRate,Fee, andPublishedfields aligns with the PR's objectives to enhance transaction-related information storage.- 58-78: The
toTlvStreammethod forTxRecordcorrectly creates a TLV stream with appropriate types for each field. This method is essential for serializingTxRecordinstances in a format that can be stored in the database.- 81-91: The
serializeTxRecordfunction correctly serializes aTxRecordto a writer using the TLV format. This function is crucial for storing transaction records in the database.- 93-111: The
deserializeTxRecordfunction correctly deserializes aTxRecordfrom a reader using the TLV format. This function is crucial for retrieving transaction records from the database.- 120-131: The extension of the
SweeperStoreinterface to includeStoreTx,GetTx, andDeleteTxmethods is correctly implemented, aligning with the PR's objectives to enhance transaction-related functionalities.- 254-269: The implementation of the
StoreTxmethod insweeperStorecorrectly serializes theTxRecordand stores it in the database. The use ofkvdb.Updatefor database operations is appropriate.- 326-371: The implementation of the
GetTxmethod insweeperStorecorrectly handles both new and old record formats, ensuring backward compatibility. The method properly deserializes theTxRecordfrom the database.- 373-382: The implementation of the
DeleteTxmethod insweeperStoreis correct, usingkvdb.Updateto remove a transaction record from the database.sweep/walletsweep.go (1)
- 322-322: The modification to the
createSweepTxfunction call to include an additional return value placeholder_before theerrvariable reflects a change in the function signature to return the transaction fee. This change is consistent with the modifications intxgenerator.go.sweep/txgenerator.go (4)
- 147-147: The change in the signature of
createSweepTxto return an additionalbtcutil.Amountrepresenting the transaction fee is correctly implemented. This change allows callers to access the fee of the created sweep transaction.- 195-195: Replacing a generic error message with
ErrLocktimeConflictwhen inputs have conflicting locktimes is a good practice for error specificity and handling.- 236-236: The error handling for the case where the input amount is insufficient to cover the outputs and fees is correctly implemented, providing a detailed error message.
- 323-323: The return statement of
createSweepTxcorrectly includes the transaction fee as part of the return values, aligning with the updated function signature.channeldb/revocation_log_test.go (3)
- 140-140: The function
WriteTlvStreamis used here following its renaming fromwriteTlvStream. Ensure all references to this function across the codebase are updated to reflect this change.- 160-160: The function
ReadTlvStreamis used here following its renaming fromreadTlvStream. Verify that all occurrences of this function in the codebase have been correctly updated to the new name.- 183-183: Usage of
ReadTlvStreamhere is consistent with its renaming. Confirm that all calls to this function throughout the project have been updated accordingly.channeldb/revocation_log.go (4)
- 342-342: Renaming
writeTlvStreamtoWriteTlvStreammakes this function exported. Ensure that this change is intentional and that the function's increased visibility is necessary for its intended use cases.- 369-369: Renaming
writeTlvStreamtoWriteTlvStreamis reflected here. Confirm that the function's broader accessibility is required and properly documented for external use.- 406-406: The renaming of
readTlvStreamtoReadTlvStreamis observed here. Verify that exposing this function is deliberate and that its use outside the package is justified and documented.- 442-442: Usage of
ReadTlvStreamhere follows its renaming. Ensure that this change in visibility is necessary and that any external use of the function is appropriately documented.sweep/sweeper.go (1)
- 1163-1190: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1166-1215]
The use of
StoreTxbefore and after the publication of the transaction is a critical step for tracking the transaction's state accurately. However, it's important to ensure that the transaction ID (Txid) and the publication status (Published) are correctly managed to reflect the transaction's lifecycle accurately. The current implementation correctly updates these fields, which is essential for the integrity of the transaction tracking process.The handling of the
TxRecordbefore and after the transaction publication is correctly implemented, ensuring accurate tracking of the transaction's lifecycle.sweep/sweeper_test.go (6)
- 19-19: The import of the
channeldbpackage is correctly added to support the new functionality of theSweeperStore.- 106-112: The error handling in
createSweeperTestContextfunction has been updated to userequire.NoErrorinstead ofif err != nil { t.Fatal(err) }, aligning with best practices for concise and readable test code.- 710-711: The modification of the
storedeclaration from*MockSweeperStoretoSweeperStoreand the creation of a new store usingchanneldb.MakeTestDBandNewSweeperStorefunctions are correctly implemented to enhance the functionality of theSweeperStore.- 773-779: The use of
require.NoErrorfor error handling in test functions is a good practice, ensuring that tests fail fast and clearly when an unexpected error occurs.- 803-804: The logic for handling test cases related to sweeper restarts, remote spends, and confirmed transactions appears to be correctly implemented, covering important scenarios for the sweeper's functionality.
- 818-825: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [821-841]
The tests for sweeper restarts with confirmed transactions and the handling of different fee preferences and pending inputs demonstrate thorough testing of the sweeper's behavior in various scenarios.
sweep/store.go
Outdated
| hash := sweepTx.TxHash() | ||
|
|
||
| return txHashesBucket.Put(hash[:], []byte{}) | ||
| return txHashesBucket.Put(txid[:], []byte{}) |
There was a problem hiding this comment.
So will we start to fully garbage collect these hashes at this point?
There was a problem hiding this comment.
I don't see us using DeleteTx in this PR ?
There was a problem hiding this comment.
DeleteTx is not used yet, plan is to use it in two places a) for neutrino backends where we'd rely on stored info to do naive fee bumps and b) maybe a new RPC call so the users can GC it.
| @@ -315,7 +320,7 @@ func createSweepTx(inputs []input.Input, outputs []*wire.TxOut, | |||
| estimator.parentsWeight, | |||
| ) | |||
|
|
|||
| return sweepTx, nil | |||
| return sweepTx, txFee, nil | |||
There was a problem hiding this comment.
Do we also want to return information w.r.t: the fee desired by the input, vs the total fee that was satisficed by the sweeper?
There was a problem hiding this comment.
Don't think we can do it here - it just creates a tx using the specified feerate, so by the time we arrived here, the feerate has already been determined by previous steps.
channeldb/revocation_log.go
Outdated
| // reader. | ||
| func readTlvStream(r io.Reader, s *tlv.Stream) (tlv.TypeMap, error) { | ||
| func ReadTlvStream(r io.Reader, s *tlv.Stream) (tlv.TypeMap, error) { |
There was a problem hiding this comment.
When would we need these vs the normal/existing main entry point of stream creation?
There was a problem hiding this comment.
I also think we should either generalize these functions or use similar ones in different packages, because the revovation_log stands for its own.
There was a problem hiding this comment.
Removed this commit as it's no longer needed.
| // TxRecord specifies a record of a tx that's stored in the database. | ||
| type TxRecord struct { | ||
| // Txid is the sweeping tx's txid. | ||
| Txid chainhash.Hash |
There was a problem hiding this comment.
Can use tlv.RecordT here now to declare the types inline. These are also all primitive records, so no extra code would be needed.
There was a problem hiding this comment.
This is not part of the serialized tlv stream but a comment which highlights this would be good
There was a problem hiding this comment.
I wonder if the design could be improved, because we don't really need the Txid only as a key saving the record, so maybe supply it as a function parameter rather than having it as part of the record ?
There was a problem hiding this comment.
Tried tlv.RecordT and came up with something like this,
// TxRecord specifies a record of a tx that's stored in the database.
type TxRecord struct {
// FeeRate is the fee rate of the sweeping tx, unit is sats/kw.
feeRate tlv.RecordT[tlv.TlvType0, uint64]
// Fee is the fee of the sweeping tx, unit is sat.
fee tlv.RecordT[tlv.TlvType1, uint64]
// Published indicates whether the tx has been published.
published tlv.RecordT[tlv.TlvType2, bool]
}
func encode(w io.Writer, tr *TxRecord) error {
recordProducers := []tlv.RecordProducer{
&tr.feeRate,
&tr.fee,
&tr.published,
}
records := make([]tlv.Record, 0, len(recordProducers))
for _, producer := range recordProducers {
records = append(records, producer.Record())
}
tlvStream, err := tlv.NewStream(records...)
if err != nil {
return err
}
// Encode the tlv stream.
var buf bytes.Buffer
if err := tlvStream.Encode(&buf); err != nil {
return err
}
// Write the tlv stream.
if _, err = w.Write(buf.Bytes()); err != nil {
return err
}
return nil
}The problem is I cannot unpack the record producers easily like extraData.PackRecords(recordProducers...), which is used in other places that are more related to the wire messages. Also feel a bit weird to read and write via feeRate.Val. The core issue tho, is we are not using primitive types but bigsize here to save bytes, which we'll need to update the tlv package to have this type.
| // TxRecord specifies a record of a tx that's stored in the database. | ||
| type TxRecord struct { | ||
| // Txid is the sweeping tx's txid. | ||
| Txid chainhash.Hash |
There was a problem hiding this comment.
Do we also want to store historical information w.r.t the inputs as well? Given that during the course of fee bumping for a given input, it may have been bundled in several distinct transactions before confirmation.
There was a problem hiding this comment.
yeah the historical info is implicitly stored, as each sweeping tx is stored, we can find the input by looking at the tx's inputs. Also we will not bump a given input, but always the tx, due to the reason(RBF tx, not inputs) mentioned in #8424.
sweep/store.go
Outdated
| } | ||
|
|
||
| // Read the tlv stream. | ||
| if _, err := channeldb.ReadTlvStream(r, tlvStream); err != nil { |
There was a problem hiding this comment.
Is this all that diff from doing:
stream := `NewStream(...)`
stream.Encode(w)
?
There was a problem hiding this comment.
yeah for some reason thought we'll need to encode the length of the tlv stream too, which was accomplished by ReadTlvStream, now removed.
| } | ||
|
|
||
| // DeleteTx removes the given tx from db. | ||
| func (s *MockSweeperStore) DeleteTx(txid chainhash.Hash) error { | ||
| return nil | ||
| args := s.Called(txid) |
There was a problem hiding this comment.
Do these tests pass as is? Given there were no expectations declared, so in my exp, the tests fails as it declares that it got an unexpected call.
There was a problem hiding this comment.
yeah exactly, I was confused too, then realized none of the sweeper store's methods were hit in the sweeper's unit tests...they are properly mocked in the following PRs tho.
sweep/txgenerator.go
Outdated
| @@ -20,6 +20,10 @@ var ( | |||
| // allowed in a single sweep tx. If more need to be swept, multiple txes | |||
| // are created and published. | |||
| DefaultMaxInputsPerTx = 100 | |||
|
|
|||
| // ErrLocktimeConflict is returned when inputs have different | |||
There was a problem hiding this comment.
Maybe add a comment that the nlocktime is always commited to by the signature.
sweep/store.go
Outdated
| hash := sweepTx.TxHash() | ||
|
|
||
| return txHashesBucket.Put(hash[:], []byte{}) | ||
| return txHashesBucket.Put(txid[:], []byte{}) |
There was a problem hiding this comment.
I don't see us using DeleteTx in this PR ?
| @@ -1467,10 +1467,12 @@ func (s *UtxoSweeper) CreateSweepTx(inputs []input.Input, feePref FeePreference, | |||
| return nil, err | |||
| } | |||
|
|
|||
| return createSweepTx( | |||
| tx, _, err := createSweepTx( | |||
There was a problem hiding this comment.
I wonder why we do not save infos to the sweep for htlcSuccessResolver on the remote transaction. It seems like we do not register anything with the sweeper there ?
There was a problem hiding this comment.
Good question - I think we only do CreateSweepTx there, not sure why it's not offered to the sweeper, maybe something to fix after the fee bumper PR.
There was a problem hiding this comment.
Yes I think we should add a TODO and integrate it into the sweeper flow as soon as the sweeper subsystem is RBF-aware.
There was a problem hiding this comment.
Will address it in the 6th PR.
channeldb/revocation_log.go
Outdated
| // reader. | ||
| func readTlvStream(r io.Reader, s *tlv.Stream) (tlv.TypeMap, error) { | ||
| func ReadTlvStream(r io.Reader, s *tlv.Stream) (tlv.TypeMap, error) { |
There was a problem hiding this comment.
I also think we should either generalize these functions or use similar ones in different packages, because the revovation_log stands for its own.
| // TxRecord specifies a record of a tx that's stored in the database. | ||
| type TxRecord struct { | ||
| // Txid is the sweeping tx's txid. | ||
| Txid chainhash.Hash |
There was a problem hiding this comment.
This is not part of the serialized tlv stream but a comment which highlights this would be good
| // TxRecord specifies a record of a tx that's stored in the database. | ||
| type TxRecord struct { | ||
| // Txid is the sweeping tx's txid. | ||
| Txid chainhash.Hash |
There was a problem hiding this comment.
I wonder if the design could be improved, because we don't really need the Txid only as a key saving the record, so maybe supply it as a function parameter rather than having it as part of the record ?
| @@ -315,7 +320,7 @@ func createSweepTx(inputs []input.Input, outputs []*wire.TxOut, | |||
| estimator.parentsWeight, | |||
| ) | |||
|
|
|||
| return sweepTx, nil | |||
| return sweepTx, txFee, nil | |||
There was a problem hiding this comment.
If we didn't add a change output, the fee is slightly higher than txFee
| inputs, nil, s.currentOutputScript, uint32(currentHeight), | ||
| feeRate, s.cfg.MaxFeeRate.FeePerKWeight(), s.cfg.Signer, | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("create sweep tx: %v", err) | ||
| } | ||
|
|
||
| tr := &TxRecord{ | ||
| Txid: tx.TxHash(), | ||
| FeeRate: uint64(feeRate), |
There was a problem hiding this comment.
The effective feerate may be slightly different than feeRate because we may not have a change output
There was a problem hiding this comment.
Noted and addressed.
sweep/store.go
Outdated
| } | ||
|
|
||
| return nil | ||
| }, func() {}) |
There was a problem hiding this comment.
need to populate the reset method and refresh tr?
yyforyongyu
force-pushed
the
sweeper-store-fees
branch
from
821aba1 to
6914bd8
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between a3bf2c7 and 6914bd8f9b42d93f879ad3124275b1abe64450a9.Files selected for processing (10)
- channeldb/revocation_log.go (6 hunks)
- channeldb/revocation_log_test.go (3 hunks)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- sweep/store.go (6 hunks)
- sweep/store_mock.go (1 hunks)
- sweep/store_test.go (4 hunks)
- sweep/sweeper.go (3 hunks)
- sweep/sweeper_test.go (9 hunks)
- sweep/txgenerator.go (8 hunks)
- sweep/walletsweep.go (1 hunks)
Additional comments: 36
sweep/store_mock.go (5)
- 11-11: The introduction of
mock.MockinMockSweeperStoreis a significant improvement for flexibility and testability. This change allows for more dynamic behavior in tests, enabling the simulation of various scenarios without hardcoding responses.- 19-24: The
IsOurTxmethod now leveragesmock.Mockfor determining if a transaction is ours. This change enhances testability by allowing dynamic responses based on input. Ensure that tests usingIsOurTxare updated to set expectations onmock.Mockaccordingly.- 27-29: The addition of the
StoreTxmethod toMockSweeperStorealigns with the PR objectives to enhance transaction management. This method should be utilized in tests to simulate storing transactions and to verify the behavior of components interacting withSweeperStore.- 34-37: The
ListSweepsmethod's modification to return a list ofchainhash.Hashand an error is a good practice, ensuring that error handling is consistent across the interface. This change should be reflected in tests, ensuring they handle the potential error return value.- 39-50: The addition of
GetTxandDeleteTxmethods is crucial for the enhanced transaction management capabilities described in the PR objectives. These methods allow for querying and deleting transactions based on their hash, improving the system's ability to manage transactions dynamically.sweep/store_test.go (5)
- 45-75: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [17-87]
The refactoring of the
TestStorefunction simplifies the test setup and improves readability. The direct creation of a store, storing transactions, and verifying their presence are straightforward and effective. Ensure that all scenarios previously covered by nested functions are still adequately tested.
- 91-117: The
TestTxRecordfunction correctly tests the serialization and deserialization ofTxRecord. This test ensures that theserializeTxRecordanddeserializeTxRecordfunctions work as expected, which is crucial for the integrity of transaction records stored in the database.- 119-152: The
TestGetTxfunction effectively tests the retrieval of transaction records from the store, including the scenario where a non-existing transaction is queried. This test is essential for validating theGetTxmethod's behavior in both successful and error scenarios.- 154-184: The
TestGetTxCompatiblefunction tests the compatibility of theGetTxmethod with old data formats, ensuring that the system can handle legacy data without issues. This test is important for maintaining backward compatibility and ensuring smooth transitions between data formats.- 186-222: The
TestDeleteTxfunction tests the deletion of transaction records from the store, including the scenario where a non-existing transaction is attempted to be deleted. This test ensures that theDeleteTxmethod behaves correctly, allowing for proper management of transaction records.sweep/store.go (8)
- 43-56: The
TxRecordstruct is well-defined, including all necessary fields for managing transaction records (Txid,FeeRate,Fee,Published). This struct is crucial for the enhanced transaction management capabilities described in the PR objectives.- 58-79: The
toTlvStreammethod for convertingTxRecordinto a TLV representation is correctly implemented, ensuring that transaction records can be serialized efficiently. The choice of TLV for serialization supports extensibility and backward compatibility.- 81-91: The
serializeTxRecordfunction correctly serializes aTxRecordbased on the TLV format. This function is essential for storing transaction records in the database in a structured and efficient manner.- 93-111: The
deserializeTxRecordfunction correctly deserializes aTxRecordfrom the TLV format. This function is crucial for retrieving transaction records from the database and ensuring their integrity.- 120-131: The modifications to the
SweeperStoreinterface, including the addition ofStoreTx,GetTx, andDeleteTxmethods, align with the PR objectives to enhance transaction management capabilities. These methods provide a comprehensive interface for managing transaction records.- 254-269: The
StoreTxmethod implementation correctly stores transaction records in the database, utilizing the TLV serialization format. This method is key to the enhanced transaction management capabilities.- 326-371: The
GetTxmethod implementation correctly retrieves transaction records from the database, handling both new and old data formats. This method ensures that transaction records can be queried efficiently, supporting backward compatibility.- 373-382: The
DeleteTxmethod implementation correctly removes transaction records from the database. This method is essential for managing the lifecycle of transaction records and ensuring that the database remains clean and efficient.sweep/walletsweep.go (1)
- 322-322: The addition of an underscore (_) in the
createSweepTxfunction call indicates that the function's signature has been modified to return an additional value, which is being intentionally ignored here. Please verify that ignoring this new return value is intentional and does not omit handling crucial information that could affect the transaction's outcome or fee management.sweep/txgenerator.go (2)
- 24-26: Introduced
ErrLocktimeConflictto handle conflicting nLockTime values in the same transaction. This addition improves error specificity and handling, making it clearer why a transaction might fail under certain conditions.- 297-312: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [147-323]
The
createSweepTxfunction signature has been modified to return an additional value,btcutil.Amount, representing the fees. This change enhances the function's utility by providing detailed fee information, which can be crucial for transaction management and tracking. It's important to ensure that all calls to this function correctly handle the new return value to avoid ignoring critical fee information.docs/release-notes/release-notes-0.18.0.md (1)
- 299-301: The addition to the release notes under the "Database" section clearly outlines the new functionality added to the SweeperStore, including storing feerate, fees paid, and publication status for sweeping transactions. This change is well-documented and aligns with the PR objectives.
Ensure that all relevant details about the implementation and its impact on the system are included and accurately described. If there are any additional nuances or considerations that users should be aware of regarding this update, consider adding those for a more comprehensive overview.
channeldb/revocation_log_test.go (4)
- 140-140: The function
WriteTlvStreamis correctly used here following the renaming convention. This change improves consistency and aligns with Go's naming conventions for exported functions.- 160-160: Similarly, the function
ReadTlvStreamis used correctly, reflecting the renaming and ensuring consistency in the codebase for exported functions.- 183-183: The usage of
ReadTlvStreamin the context of handling an error scenario (EOF) is a good practice, as it tests the robustness of the error handling in the serialization/deserialization logic.- 137-143: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-273]
Overall, the tests in
channeldb/revocation_log_test.goare well-structured and thorough, covering a wide range of scenarios including serialization/deserialization, error handling, and database operations. The renaming ofwriteTlvStreamandreadTlvStreamtoWriteTlvStreamandReadTlvStreamis consistently applied, improving the readability and maintainability of the code.channeldb/revocation_log.go (2)
- 342-342: The function
writeTlvStreamhas been renamed toWriteTlvStream. This change makes the function publicly accessible, which could be beneficial for serialization tasks outside of this package. However, it's important to ensure that this change aligns with the overall design principles of the codebase and does not inadvertently expose internal functionality that should remain private.- 369-369: Similarly, the function
readTlvStreamhas been renamed toReadTlvStream, making it publicly accessible. As with the previous comment, it's crucial to verify that this change is consistent with the intended use cases and does not expose more functionality than necessary.sweep/sweeper.go (1)
- 1486-1490: The
CreateSweepTxfunction has been modified to use thecreateSweepTxhelper function for creating the sweep transaction. This change is consistent with the rest of the modifications and leverages the existing logic for sweep transaction creation. The use ofcreateSweepTxensures that the sweep transaction adheres to the specified fee rate and other parameters, improving code reuse and maintainability.No specific issues or improvements are identified for this change, as it aligns with the overall goal of enhancing sweep transaction creation and management. The change appears to be correctly implemented and logically sound.
sweep/sweeper_test.go (7)
- 19-19: The import of
channeldbis added as part of the changes. Ensure that this new dependency is utilized appropriately within the test suite and that it does not introduce any unwanted coupling between thesweeppackage and thechanneldbpackage.- 106-112: The initialization of
storeusingchanneldb.MakeTestDBandNewSweeperStorewithincreateSweeperTestContextis a significant change. It's crucial to verify that thestoreis correctly integrated with the rest of the test setup and that it interacts as expected with theUtxoSweeperinstance. Additionally, ensure that the error handling withrequire.NoErroris appropriate for the test's design, as it will halt the test immediately if an error is encountered, which is usually desirable in setup phases.- 710-711: Using
require.NoErrorfor error handling in theTestRestartfunction is a good practice for test code, as it simplifies the error checking logic. However, ensure that this change is consistent with the overall error handling strategy in the test suite. It's also important to verify that the context in whichrequire.NoErroris used is appropriate, and that failing the test immediately upon encountering an error is the desired behavior in these cases.- 773-774: The change to use
require.NoErrorin theTestRestartRemoteSpendfunction follows the same considerations as mentioned previously for theTestRestartfunction. Ensure consistency and appropriateness of immediate test failure upon error detection.- 778-779: Similar to previous comments, the use of
require.NoErrorin handling the initialization ofinput2in theTestRestartRemoteSpendfunction should be verified for consistency and correctness within the test suite's error handling strategy.- 803-804: The comment indicating the re-offering of input 0 in the
TestRestartRemoteSpendfunction provides clarity on the test's intention. It's good practice to include such comments, especially in complex test scenarios involving state changes and retries. Ensure that the comment accurately reflects the test logic and that the subsequent operations align with the described behavior.- 840-841: The comment in the
TestRestartConfirmedfunction, similar to the previous one, aids in understanding the test flow, especially regarding the re-offering of inputs after a restart. This practice enhances the readability and maintainability of the test code.
yyforyongyu
force-pushed
the
sweeper-store-fees
branch
2 times, most recently
from
685ef00 to
b491ba6
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between a3bf2c7 and b491ba66ad905a384af0392ae2e81bd050f1a3d0.Files selected for processing (8)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- sweep/store.go (6 hunks)
- sweep/store_mock.go (1 hunks)
- sweep/store_test.go (4 hunks)
- sweep/sweeper.go (3 hunks)
- sweep/sweeper_test.go (9 hunks)
- sweep/txgenerator.go (9 hunks)
- sweep/walletsweep.go (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- docs/release-notes/release-notes-0.18.0.md
- sweep/store.go
- sweep/store_test.go
- sweep/sweeper.go
- sweep/sweeper_test.go
- sweep/walletsweep.go
Additional comments: 6
sweep/store_mock.go (4)
- 11-11: The introduction of
mock.MockinMockSweeperStorestruct is a significant change. Ensure that all methods ofMockSweeperStoreproperly utilize themockpackage's capabilities for mocking behavior and expectations. This change should enhance testability by allowing more flexible and detailed mock setups.- 19-24: The
IsOurTxmethod now leveragesmock.Mockfor determining if a transaction is ours. This change should make the method more testable and flexible. However, ensure that the corresponding tests are updated to set expectations on this method correctly to avoid unexpected test failures.- 34-37: The
ListSweepsmethod now returns a list ofchainhash.Hashand an error. This change aligns with the need for more detailed error handling. Ensure that the method's consumers are updated to handle the potential error return value.- 39-50: The addition of
GetTxandDeleteTxmethods enhances theMockSweeperStore's capabilities for querying and deleting transactions. Ensure these methods are adequately tested, particularly their interaction with themockframework. Proper expectation setup and behavior assertion in unit tests are crucial.sweep/txgenerator.go (2)
- 24-30: The introduction of
ErrLocktimeConflictis a good practice for handling specific error scenarios with more clarity. This makes error handling more descriptive and easier to manage. Ensure that this error is properly handled wherevercreateSweepTxis called.- 305-320: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [151-331]
The modifications to the
createSweepTxfunction, including the addition ofbtcutil.Amountfor fees in the return signature and the introduction ofErrLocktimeConflict, significantly improve the function's error handling and clarity. It's important to ensure that all callers ofcreateSweepTxare updated to handle the new return values correctly. Additionally, the enhanced error messages and the detailed handling of input scripts contribute to better maintainability and readability of the code. Ensure that the changes are covered by unit tests, particularly the new error scenarios and the handling of fees.
| @@ -1467,10 +1467,12 @@ func (s *UtxoSweeper) CreateSweepTx(inputs []input.Input, feePref FeePreference, | |||
| return nil, err | |||
| } | |||
|
|
|||
| return createSweepTx( | |||
| tx, _, err := createSweepTx( | |||
There was a problem hiding this comment.
Yes I think we should add a TODO and integrate it into the sweeper flow as soon as the sweeper subsystem is RBF-aware.
yyforyongyu
force-pushed
the
sweeper-store-fees
branch
from
b491ba6 to
40dfafa
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between b234658 and 40dfafa4b6fbea35274a57d46bc41bfd5e8c46f4.Files selected for processing (8)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- sweep/store.go (6 hunks)
- sweep/store_mock.go (1 hunks)
- sweep/store_test.go (4 hunks)
- sweep/sweeper.go (3 hunks)
- sweep/sweeper_test.go (9 hunks)
- sweep/txgenerator.go (9 hunks)
- sweep/walletsweep.go (1 hunks)
Additional comments: 26
sweep/store_mock.go (5)
- 11-11: The embedding of
mock.MockinMockSweeperStorestruct is a good practice for leveraging the powerful mocking capabilities provided by the testify library. This change should facilitate easier testing of components that depend onSweeperStore.- 19-24: The method
IsOurTxcorrectly usesmock.Mock'sCalledmethod to simulate behavior based on the input. Ensure that when using this mock in tests, expectations are set correctly for this method to avoid unexpected test failures.- 26-29: The
StoreTxmethod implementation usingmock.Mockis correct. Similar toIsOurTx, it's important to set up expectations for this method in tests whereMockSweeperStoreis used to ensure the mock behaves as intended.- 39-50: The
GetTxmethod's implementation with conditional return based on the mock's response is well done. This allows for flexible testing scenarios. As with other methods, ensure that tests using this mock properly define expectations forGetTx.- 52-56: The
DeleteTxmethod correctly utilizesmock.Mock. It's crucial to set up expectations for this method in tests to simulate various scenarios, such as successful deletion or errors.sweep/store_test.go (4)
- 91-117: The
TestTxRecordfunction correctly tests the serialization and deserialization ofTxRecord. This is crucial for ensuring data integrity when storing and retrieving transaction records. The test is concise and covers the expected behavior.- 119-152:
TestGetTxeffectively tests the retrieval of transaction records and the behavior when a record does not exist. This test is important for validating theGetTxmethod's correctness and error handling.- 154-184:
TestGetTxCompatibletests backward compatibility with old data formats, which is essential for ensuring that the system can handle legacy data without issues. This test demonstrates good foresight in maintaining system robustness during upgrades.- 186-222:
TestDeleteTxchecks the deletion functionality of transaction records, including the behavior when attempting to delete a non-existent record. This test is comprehensive and ensures that theDeleteTxmethod functions as expected.sweep/store.go (3)
- 42-56: The introduction of the
TxRecordstruct is a significant enhancement, providing a structured way to store transaction records with relevant metadata. This change improves data handling and facilitates future enhancements.- 126-137: The updates to the
SweeperStoreinterface, including the addition ofStoreTx,GetTx, andDeleteTxmethods, are well thought out. These changes enhance the interface's capabilities, making it more versatile and suited for transaction management.- 329-394: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [260-391]
The implementation of the
StoreTx,IsOurTx,ListSweeps,GetTx, andDeleteTxmethods insweeperStoreis correct and aligns with the updatedSweeperStoreinterface. Ensure comprehensive tests are in place for these methods to validate their behavior under various scenarios.sweep/walletsweep.go (1)
- 322-322: The addition of an underscore as a parameter in the
createSweepTxfunction call suggests that a return value is being intentionally ignored. Ensure that this ignored value is not critical to the operation or error handling of theCraftSweepAllTxfunction. Ignoring important return values can lead to subtle bugs or missed error handling opportunities.sweep/txgenerator.go (5)
- 24-30: The introduction of
ErrLocktimeConflictis a good practice for handling specific error scenarios clearly. This makes error handling more precise and informative for callers of the function.- 151-151: The change in the
createSweepTxfunction signature to return an additionalbtcutil.Amount(representing the transaction fee) is a significant improvement. It allows callers to access more detailed information about the transaction, which can be critical for decision-making processes. Ensure that all callers of this function properly handle the additional return value.- 199-199: The handling of
ErrLocktimeConflictwithincreateSweepTxensures that transactions with conflicting nLockTime values are not created, preserving the integrity of the transaction. This is a crucial check for the correctness of the transaction generation process.Also applies to: 224-224
- 240-240: The error handling for the case where the required output plus the transaction fee exceeds the total input is clear and informative. This check is essential for ensuring that the transaction does not attempt to spend more than what is available, preventing potential issues in the transaction creation process.
- 264-267: The logic to add the dust amount to the fee if the change amount is below the dust limit is a sensible approach to handling small change amounts. This ensures that the transaction does not create outputs that would be uneconomical to spend, optimizing the use of blockchain space.
docs/release-notes/release-notes-0.18.0.md (1)
- 324-327: The addition to the release notes clearly explains the expansion of the
SweeperStorefunctionality, including the new types of information it stores (feerate, fees paid, and publication status). This update should help users understand the improvements made to sweeping transaction management in version 0.18.0.sweep/sweeper.go (1)
- 1486-1490: The addition of a detailed comment explaining the rationale behind setting the
currentBlockHeightas the transaction locktime in theCreateSweepTxfunction is a good practice. It enhances code readability and provides valuable context for future maintainers or contributors. No action is required, but it's worth acknowledging the effort to improve code documentation.sweep/sweeper_test.go (6)
- 19-19: The import of
channeldbis added as part of the changes. Ensure that this new dependency is utilized appropriately within the test file and that it does not introduce unnecessary coupling between the test and the database implementation.- 45-45: The change from
*MockSweeperStoretoSweeperStoreinterface for thestorefield insweeperTestContextstruct is a good practice. It allows for more flexibility in testing by enabling the use of any store implementation that satisfies theSweeperStoreinterface. This change enhances the test's modularity and maintainability.- 106-112: The initialization of
storeusingNewSweeperStorewithcdbandchainas parameters is a significant change. It's crucial to verify that theNewSweeperStorefunction is correctly implemented and that it interacts with thechanneldbas expected. Additionally, ensure that error handling is properly done for theNewSweeperStorecall.Verification successful
To further verify the implementation of
NewSweeperStoreand its interaction withchanneldb, as well as to assess its error handling, we need to inspect the function's body in./sweep/store.go. Specifically, we should look at how it uses thedb kvdb.Backendparameter for database interactions and how it manages errors.
The implementation of
NewSweeperStoreinsweep/store.gocorrectly interacts with the database through thekvdb.Backendinterface, specifically by managing database buckets in a manner consistent with good practices. Error handling within the function is properly implemented, with errors being returned to the caller in cases of failure during database operations. This confirms that the function is correctly implemented and interacts withchanneldbas expected, addressing the concerns raised in the review comment.* 710-711: Using `require.NoError` for error handling in tests is a good practice as it simplifies the test code and makes it more readable. It immediately fails the test if an error is encountered, which is appropriate for setup steps where the test cannot proceed if an error occurs. * 766-779: The setup for the `TestRestartRemoteSpend` test, including the creation of inputs and the use of `require.NoError` for error handling, is correctly implemented. It's important to ensure that the test logic accurately simulates the scenario of a restart with a remote spend and that the assertions made are relevant to the scenario being tested. * 818-825: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [821-841]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify NewSweeperStore function implementation and interaction with channeldb. grep -R "func NewSweeperStore" . | grep -v "_test.go" # Expecting to find the implementation of NewSweeperStore and its interaction with channeldb.Length of output: 137
Script:
#!/bin/bash # Inspect the implementation of NewSweeperStore in ./sweep/store.go cat ./sweep/store.goLength of output: 9675
The
TestRestartConfirmedtest setup, including the sweeping of an input and the restart of the sweeper, is well-structured. It's crucial to verify that the test accurately represents the scenario of a restart with a confirmed sweep transaction and that the assertions reflect the expected outcomes in such a scenario.
sweep/sweeper.go
Outdated
| } | ||
|
|
||
| // Create sweep tx. | ||
| tx, err := createSweepTx( | ||
| tx, fee, err := createSweepTx( | ||
| inputs, nil, s.currentOutputScript, uint32(currentHeight), | ||
| feeRate, s.cfg.MaxFeeRate.FeePerKWeight(), s.cfg.Signer, | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("create sweep tx: %v", err) | ||
| } | ||
|
|
||
| tr := &TxRecord{ | ||
| Txid: tx.TxHash(), | ||
| FeeRate: uint64(feeRate), | ||
| Fee: uint64(fee), | ||
| } | ||
|
|
||
| // Add tx before publication, so that we will always know that a spend | ||
| // by this tx is ours. Otherwise if the publish doesn't return, but did | ||
| // publish, we loose track of this tx. Even republication on startup | ||
| // doesn't prevent this, because that call returns a double spend error | ||
| // then and would also not add the hash to the store. | ||
| err = s.cfg.Store.NotifyPublishTx(tx) | ||
| err = s.cfg.Store.StoreTx(tr) | ||
| if err != nil { | ||
| return fmt.Errorf("notify publish tx: %v", err) | ||
| return fmt.Errorf("store tx: %w", err) | ||
| } | ||
|
|
||
| // Reschedule the inputs that we just tried to sweep. This is done in |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1166-1215]
The implementation of TxRecord handling within the sweep function introduces several considerations:
- Correctness and Logic: The logic for creating a
TxRecord, storing it before and after publication, and marking it as published appears correct. This ensures that the transaction is tracked both before and after it's successfully broadcasted. - Performance: Storing the transaction record before and after publication might have a slight impact on performance due to the additional database operations. However, this is necessary for accurate tracking and is unlikely to be a significant concern.
- Error Handling: Proper error handling is implemented for database operations (
StoreTx). This is crucial for ensuring that failures in storing the transaction record do not go unnoticed. - Maintainability: The separation of concerns could be improved. The
sweepfunction is responsible for too many tasks: creating the sweep transaction, handling database operations, and publishing the transaction. Consider refactoring to improve maintainability and readability.
Overall, the changes are logically sound and serve the purpose of enhancing transaction tracking within the SweeperStore. However, a slight refactoring could improve code maintainability and readability.
Consider refactoring the sweep function to separate the concerns of transaction creation, database operations, and transaction publication. This could involve extracting the database operations into separate functions or methods, which would improve the maintainability and readability of the code.
| // toTlvStream converts TxRecord into a tlv representation. | ||
| func (t *TxRecord) toTlvStream() (*tlv.Stream, error) { | ||
| const ( | ||
| // A set of tlv type definitions used to serialize TxRecord. | ||
| // We define it here instead of the head of the file to avoid | ||
| // naming conflicts. | ||
| // | ||
| // NOTE: A migration should be added whenever the existing type | ||
| // changes. | ||
| // | ||
| // NOTE: Txid is stored as the key, so it's not included here. | ||
| feeRateType tlv.Type = 0 | ||
| feeType tlv.Type = 1 | ||
| boolType tlv.Type = 2 | ||
| ) | ||
|
|
||
| return tlv.NewStream( | ||
| tlv.MakeBigSizeRecord(feeRateType, &t.FeeRate), | ||
| tlv.MakeBigSizeRecord(feeType, &t.Fee), | ||
| tlv.MakePrimitiveRecord(boolType, &t.Published), | ||
| ) | ||
| } | ||
|
|
||
| // serializeTxRecord serializes a TxRecord based on tlv format. | ||
| func serializeTxRecord(w io.Writer, tx *TxRecord) error { | ||
| // Create the tlv stream. | ||
| tlvStream, err := tx.toTlvStream() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Encode the tlv stream. | ||
| var buf bytes.Buffer | ||
| if err := tlvStream.Encode(&buf); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Write the tlv stream. | ||
| if _, err = w.Write(buf.Bytes()); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The serialization and deserialization methods for TxRecord using TLV encoding are correctly implemented. These methods are crucial for ensuring data integrity and compatibility. It's important to add comprehensive tests for these methods to cover various edge cases and data values.
Suggest adding more tests for serializeTxRecord and deserializeTxRecord to cover edge cases and different data values.
| log.Debugf("Inserting nursery tx %v in hash list "+ | ||
| "(height=%v)", hash, byteOrder.Uint32(k)) | ||
|
|
||
| return txHashesBucket.Put(hash[:], []byte{}) | ||
| // Create the transaction record. Since this is an old record, | ||
| // we can assume it's already been published. Although it's | ||
| // possible to calculate the fees and fee rate used here, we | ||
| // skip it as it's unlikely we'd perform RBF on these old | ||
| // sweeping transactions. | ||
| tr := &TxRecord{ | ||
| Txid: hash, | ||
| Published: true, | ||
| } | ||
|
|
||
| // Serialize tx record. | ||
| var b bytes.Buffer | ||
| err = serializeTxRecord(&b, tr) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return txHashesBucket.Put(tr.Txid[:], b.Bytes()) | ||
| }) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [177-251]
The migrateTxHashes function is a thoughtful addition for transitioning old records to the new format, ensuring backward compatibility. However, consider adding more detailed logging for each migrated transaction to aid in debugging and verification.
+ log.Debugf("Migrating transaction: %v", hash)
yyforyongyu
force-pushed
the
sweeper-store-fees
branch
from
40dfafa to
ae5353d
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (8)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- sweep/store.go (6 hunks)
- sweep/store_mock.go (1 hunks)
- sweep/store_test.go (4 hunks)
- sweep/sweeper.go (3 hunks)
- sweep/sweeper_test.go (9 hunks)
- sweep/txgenerator.go (9 hunks)
- sweep/walletsweep.go (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- docs/release-notes/release-notes-0.18.0.md
- sweep/store.go
- sweep/store_mock.go
- sweep/sweeper.go
- sweep/sweeper_test.go
- sweep/txgenerator.go
- sweep/walletsweep.go
Additional comments: 4
sweep/store_test.go (4)
- 91-117: The
TestTxRecordfunction correctly tests the serialization and deserialization ofTxRecord. It's well-implemented and follows best practices for unit testing by asserting both the process of serialization and the equality of the original and deserialized objects.This test function is a good example of how to effectively test serialization logic.
- 119-152: The
TestGetTxfunction is well-structured and tests the retrieval of transaction records effectively. It covers both the case of successfully retrieving an existing transaction and the case of failing to retrieve a non-existent transaction, which is good for ensuring the method behaves as expected under different scenarios.Good job on covering both success and failure scenarios in this test.
- 154-184: The
TestGetTxCompatiblefunction effectively tests the compatibility with old data formats, ensuring that the system can handle legacy data without issues. This is crucial for maintaining backward compatibility and ensuring a smooth transition for existing data.Well done on ensuring backward compatibility with old data formats.
- 186-222: The
TestDeleteTxfunction correctly tests the deletion of transaction records, covering both the successful deletion of an existing record and the case where attempting to delete a non-existent transaction does not result in an error. This ensures that theDeleteTxmethod behaves as expected in different scenarios.Good coverage of both successful deletion and handling of non-existent transactions.
| }, | ||
| }) | ||
|
|
||
| err = store.NotifyPublishTx(&tx2) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| tr2 := &TxRecord{ | ||
| Txid: tx2.TxHash(), | ||
| } | ||
|
|
||
| err = store.StoreTx(tr2) | ||
| require.NoError(t, err) | ||
|
|
||
| // Recreate the sweeper store | ||
| store, err = createStore() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| store, err = NewSweeperStore(cdb, &chain) | ||
| require.NoError(t, err) | ||
|
|
||
| // Assert that both txes are recognized as our own. | ||
| ours, err := store.IsOurTx(tx1.TxHash()) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if !ours { | ||
| t.Fatal("expected tx to be ours") | ||
| } | ||
| require.NoError(t, err) | ||
| require.True(t, ours, "expected tx to be ours") | ||
|
|
||
| ours, err = store.IsOurTx(tx2.TxHash()) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if !ours { | ||
| t.Fatal("expected tx to be ours") | ||
| } | ||
| require.NoError(t, err) | ||
| require.True(t, ours, "expected tx to be ours") | ||
|
|
||
| // An different hash should be reported as not being ours. | ||
| var unknownHash chainhash.Hash | ||
| ours, err = store.IsOurTx(unknownHash) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if ours { | ||
| t.Fatal("expected tx to be not ours") | ||
| } | ||
| require.NoError(t, err) | ||
| require.False(t, ours, "expected tx to not be ours") | ||
|
|
||
| txns, err := store.ListSweeps() | ||
| require.NoError(t, err, "unexpected error") |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [17-87]
The refactoring of TestStore and the addition of new test cases significantly improve the test coverage for the SweeperStore. However, it's important to ensure that edge cases, such as attempting to store a duplicate transaction record or retrieving a non-existent transaction, are also tested. This will help in making the tests more robust and comprehensive.
Consider adding tests for these edge cases to ensure the SweeperStore behaves as expected under various conditions.
|
@yyforyongyu, remember to re-request review from reviewers when ready |
yyforyongyu
force-pushed
the
sweeper-store-fees
branch
from
ae5353d to
14ec634
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- sweep/store.go (6 hunks)
- sweep/store_mock.go (1 hunks)
- sweep/store_test.go (4 hunks)
- sweep/sweeper.go (3 hunks)
- sweep/sweeper_test.go (9 hunks)
- sweep/txgenerator.go (9 hunks)
- sweep/walletsweep.go (1 hunks)
Additional comments: 29
sweep/store_mock.go (6)
- 11-11: The embedding of
mock.Mockin theMockSweeperStorestruct is a standard practice for creating mocks using the testify/mock package. This change enables the mock store to inherit methods frommock.Mock, allowing it to simulate the behavior of theSweeperStoreinterface.- 19-24: The implementation of the
IsOurTxmethod usings.Called(hash)is correct and follows the testify/mock conventions for simulating method calls and responses. This change allows the mock store to simulate checking if a transaction is ours based on its hash.- 27-29: The
StoreTxmethod correctly usess.Called(tr)to simulate storing a transaction. This change aligns with the PR's objective to enhance transaction management by introducing the ability to store transactions before publishing.- 34-37: The
ListSweepsmethod's implementation usings.Called()and returningargs.Get(0).([]chainhash.Hash)andargs.Error(1)is appropriate for simulating the retrieval of all sweeps. This change supports the enhanced transaction tracking introduced in the PR.- 39-50: The
GetTxmethod correctly simulates querying the database for a transaction matching a given ID. The use ofs.Called(hash)and the conditional return based on the query result align with the PR's objectives to improve database access for transaction retrieval.- 53-56: The
DeleteTxmethod's implementation usings.Called(txid)to simulate deleting a transaction is correct. This change is part of the PR's enhancements to transaction management, allowing for the deletion of transactions based on IDs.sweep/store_test.go (5)
- 45-75: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [17-87]
The refactoring of the
TestStorefunction simplifies the test setup and improves clarity. The direct creation of a store, storing transactions, and verifying their presence in the store are well-implemented. However, it's important to ensure that edge cases, such as attempting to store a duplicate transaction record or retrieving a non-existent transaction, are also tested to make the tests more robust and comprehensive.Consider adding tests for these edge cases to ensure the
SweeperStorebehaves as expected under various conditions.
- 91-117: The
TestTxRecordfunction correctly tests the serialization and deserialization ofTxRecordusing TLV encoding. This test is crucial for ensuring data integrity and compatibility. Comprehensive tests for these methods covering various edge cases and data values are essential.Ensure comprehensive test coverage for
serializeTxRecordanddeserializeTxRecordto cover edge cases and different data values.
- 119-152: The
TestGetTxfunction properly tests theGetTxmethod's behavior, including successful retrieval of a transaction record and handling of non-existent transactions. This addition enhances the test coverage for the new database access functionality introduced in the PR.- 154-184: The
TestGetTxCompatiblefunction tests the compatibility with old data formats, ensuring that the system can successfully query old transaction records. This test is important for maintaining backward compatibility and ensuring a smooth transition to the new data format.- 186-222: The
TestDeleteTxfunction correctly tests theDeleteTxmethod's behavior, including successful deletion of a transaction record and handling deletion of non-existent transactions. This test supports the PR's enhancements to transaction management by ensuring the deletion functionality works as expected.sweep/store.go (5)
- 42-56: The introduction of the
TxRecordstruct is a key part of the PR's enhancements to theSweeperStore. It allows for comprehensive transaction tracking by storing fee information and publication status. This change aligns with the PR's objectives and is correctly implemented.- 58-101: The methods
toTlvStream,serializeTxRecord, anddeserializeTxRecordfor TLV encoding ofTxRecordare correctly implemented. These methods are crucial for ensuring data integrity and compatibility. It's important to add comprehensive tests for these methods to cover various edge cases and data values.Ensure comprehensive test coverage for TLV encoding methods to cover edge cases and different data values.
- 126-137: The addition of the
StoreTx,GetTx, andDeleteTxmethods to theSweeperStoreinterface is a significant enhancement to transaction management. These methods facilitate improved database access for transaction storage, retrieval, and deletion, aligning with the PR's objectives.- 231-254: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [177-251]
The
migrateTxHashesfunction is a thoughtful addition for transitioning old records to the new format, ensuring backward compatibility. However, consider adding more detailed logging for each migrated transaction to aid in debugging and verification.Consider adding detailed logging for each migrated transaction in the
migrateTxHashesfunction to aid in debugging and verification.
- 329-394: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [260-391]
The implementation of the
StoreTx,GetTx, andDeleteTxmethods in thesweeperStorestruct is correct and aligns with the PR's enhancements to transaction management. These methods properly handle serialization/deserialization ofTxRecordusing TLV encoding and ensure correct database operations for storing, retrieving, and deleting transactions.sweep/walletsweep.go (1)
- 322-322: The addition of an underscore (_) argument in the
createSweepTxfunction call suggests that a return value is being ignored. It's important to ensure that ignoring this return value is intentional and does not impact the correctness of the program. If this return value contains error information or other critical data, it should be handled appropriately.Please verify that ignoring this return value is intentional and does not impact the correctness of the program. If this return value contains error information or other critical data, consider handling it appropriately.
sweep/txgenerator.go (2)
- 24-30: The introduction of
ErrLocktimeConflictto handle conflictingnLockTimevalues in the same transaction is a good practice. It ensures that transactions with incompatible lock times are not combined, which could lead to invalid transactions. This change aligns with the need for precise transaction management inlnd.- 305-320: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [151-331]
The modifications to the
createSweepTxfunction, including the addition ofbtcutil.Amountfor transaction fee calculation and the enhanced error handling, significantly improve the function's clarity and functionality. The explicit handling ofErrLocktimeConflictand the detailed fee calculation, including the consideration of dust amounts for change outputs, are particularly noteworthy. These changes contribute to more reliable and efficient transaction processing inlnd.However, it's important to ensure that the changes made to the function signature and the error handling logic are consistently applied across all calls to
createSweepTxthroughout the codebase. Additionally, the handling of dust amounts and the calculation of change outputs should be carefully reviewed to ensure they align with the intended behavior and do not introduce any unintended side effects.Consider adding unit tests to cover the new error conditions (
ErrLocktimeConflict) and the updated fee calculation logic, including the handling of dust amounts for change outputs. This will help ensure the robustness and reliability of the changes.Verification successful
The verification process has confirmed that all calls to
createSweepTxhave been updated to match the new function signature, which includes seven parameters. The handling ofErrLocktimeConflictis consistent and does not conflict with other error handling logic within the context ofcreateSweepTx. Although the script output does not directly verify the calculation of dust amounts and change outputs, the initial context and the provided code snippet suggest that these aspects are part of the function's logic and have been addressed.Given the information provided and the results of the script execution, it appears that the modifications to the
createSweepTxfunction, including the addition ofbtcutil.Amountfor transaction fee calculation and the enhanced error handling, have been correctly implemented across the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all calls to createSweepTx have been updated to match the new function signature. ast-grep --lang go --pattern $'createSweepTx($_, $_, $_, $_, $_, $_, $_)' # Verify that the handling of ErrLocktimeConflict is consistent and does not conflict with other error handling logic. ast-grep --lang go --pattern $'ErrLocktimeConflict' # Verify the calculation of dust amounts and change outputs in createSweepTx. ast-grep --lang go --pattern $'createSweepTx($$$)'Length of output: 1826
docs/release-notes/release-notes-0.18.0.md (1)
- 349-351: The addition of functionality to store feerate, fees paid, and publication status for sweeping transactions in the
SweeperStoreis a significant enhancement. It's crucial to ensure that the documentation accurately reflects the implementation details and provides clear information to the users about how this new feature can be utilized or affects their interaction with the system.sweep/sweeper.go (3)
- 1163-1190: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1166-1215]
The introduction of the
TxRecordstruct and its usage in thesweepfunction for storing and updating transaction records in the database is a significant improvement for tracking transaction details. However, there are a few considerations and potential improvements:
Error Handling and Transaction Rollback: When storing the transaction record in the database (
s.cfg.Store.StoreTx(tr)), consider what should happen if the subsequent steps fail (e.g., ifPublishTransactionfails). It might be beneficial to have a mechanism to roll back or mark the transaction record as not published if subsequent steps fail after the initial store operation.Transaction Record Overwrite: The code comments at lines 1206-1214 mention that storing the transaction record again after publishing will behave as an overwrite. Ensure that this behavior is intentional and consistent with the overall design. If overwriting is not desirable in some cases, additional logic might be needed to handle those scenarios.
Potential Refactoring for Clarity: The process of creating, storing, updating, and publishing the transaction could be encapsulated into a more cohesive unit of work or service. This could improve readability and maintainability by separating the concerns of transaction preparation and database interaction.
Use of
PublishedField: ThePublishedfield in theTxRecordstruct is a good way to track the publication status of a transaction. Ensure that this field is used consistently throughout the codebase to check the transaction status where needed.Overall, these changes lay a solid foundation for improved transaction management. Just ensure that error handling and transaction status tracking are robust and consistent across different scenarios.
- 1163-1190: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1166-1490]
The call to
createSweepTxwithin thesweepandCreateSweepTxfunctions is central to the operation of constructing and publishing sweep transactions. A few points to consider for refinement:
Error Handling: Ensure that errors returned by
createSweepTxare handled appropriately. For instance, if the creation of the sweep transaction fails due to insufficient funds or other reasons, the system should log this error and decide whether to retry or abort the sweep attempt.Fee Rate Management: The function uses
feeRateands.cfg.MaxFeeRate.FeePerKWeight()to determine the fee rate for the sweep transaction. It's important to ensure that the fee rate logic aligns with the network conditions and user preferences to avoid overpaying or underpaying fees.Output Script Reuse: The
s.currentOutputScriptis used as the sweep destination. Consider the implications of reusing the same output script across different sweep transactions, especially in terms of privacy and security. It might be beneficial to generate a new output script for each sweep transaction if not already done.Lock Time Consideration: The
currentBlockHeightparameter is used as the lock time for the sweep transaction. Ensure that this approach is consistent with the intended behavior, especially for inputs that might have specific time-lock requirements.By addressing these points, you can further refine the sweep transaction creation process, enhancing its reliability and efficiency.
- 1163-1190: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [6-6]
The use of
math/randfor generating randomness inDefaultNextAttemptDeltaFuncis noted. While this is generally acceptable for non-security-critical randomness, such as calculating a delay for transaction retry, it's important to ensure that the randomness source meets the requirements of its use case. If the randomness has any security implications or if more unpredictable randomness is desired, consider usingcrypto/randinstead. However, for retry delays as implemented here,math/randseems to be an appropriate choice given its use case.sweep/sweeper_test.go (6)
- 19-19: The import of
channeldbis correctly added to support the new functionality related to storing fee information for sweeping transactions.- 45-45: Changing the type of
storefrom*MockSweeperStoretoSweeperStorealigns with the PR's objective to enhance theSweeperStorefunctionality. This change is necessary for integrating the new methodsGetTxandDeleteTxfor database access.- 106-112: The modification in the
createSweeperTestContextfunction to initialize thestorewith a real instance ofSweeperStoreusingchanneldb.MakeTestDBandNewSweeperStoreis crucial for testing the interaction with the actual database. This change ensures that the tests can verify the behavior of storing and retrieving transaction fee information accurately.- 710-711: The use of
require.NoErrorfor error handling in theTestRestartfunction is a good practice. It simplifies the test code by immediately failing the test if an error occurs, making the tests more readable and maintainable.- 766-779: Similarly, in the
TestRestartRemoteSpendfunction, replacing custom error handling withrequire.NoErroris an improvement. It enhances the test's readability and ensures consistency in how errors are handled across the test suite.- 818-825: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [821-841]
In the
TestRestartConfirmedfunction, the consistent use ofrequire.NoErrorfor error handling maintains the readability and maintainability of the test code. This approach is beneficial for quickly identifying the cause of test failures.
To properly reflect what the method really does. We also changes the method signature so only a hash is used.
Which will be used to make the sweeper RBF-aware.
This commit modifies the sweeper store to save a `TxRecord` in db instead of an empty byte slice. This record will later be used to bring RBF-awareness to our sweeper.
yyforyongyu
force-pushed
the
sweeper-store-fees
branch
from
14ec634 to
b9e6ef2
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (8)
- docs/release-notes/release-notes-0.18.0.md (1 hunks)
- sweep/store.go (6 hunks)
- sweep/store_mock.go (1 hunks)
- sweep/store_test.go (4 hunks)
- sweep/sweeper.go (3 hunks)
- sweep/sweeper_test.go (9 hunks)
- sweep/txgenerator.go (9 hunks)
- sweep/walletsweep.go (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- sweep/store_test.go
- sweep/sweeper.go
- sweep/sweeper_test.go
- sweep/txgenerator.go
- sweep/walletsweep.go
Additional comments: 15
sweep/store_mock.go (6)
- 11-11: Embedding
mock.MockinMockSweeperStoreis a good practice for simplifying the mocking of method calls and their responses. This change enhances testability and maintainability.- 19-24: The
IsOurTxmethod correctly usess.Called(hash)to simulate the behavior of this method in tests. This change is consistent with the use of the testify mock package and allows for flexible test setups.- 27-29: The
StoreTxmethod implementation usings.Called(tr)is correct and aligns with the intended use of the testify mock package. This method is crucial for testing the behavior of storing transactions.- 34-37: The
ListSweepsmethod correctly returns a slice ofchainhash.Hashand an error, using the testify mock'sargs.Getandargs.Errormethods. This change ensures that the method can be effectively mocked in tests.- 39-50: The
GetTxmethod's implementation is correct, usings.Called(hash)to simulate database queries in tests. The method correctly handles the case where the transaction is found (tr != nil) and when it's not found, returningniland an error respectively.- 53-56: The
DeleteTxmethod usess.Called(txid)to simulate the deletion of a transaction in tests. This approach is consistent with the use of the testify mock package and allows for testing the deletion behavior effectively.sweep/store.go (7)
- 42-56: The
TxRecordstruct is well-defined, with clear documentation for each field. IncludingTxid,FeeRate,Fee, andPublishedfields covers the necessary details for transaction records. This struct is crucial for the new functionality introduced in this PR.- 58-79: The
toTlvStreammethod in theTxRecordstruct correctly creates a TLV stream for serialization. The use oftlv.MakeBigSizeRecordandtlv.MakePrimitiveRecordfor different types is appropriate. This method is essential for the TLV encoding ofTxRecord.- 81-101: The
serializeTxRecordfunction correctly serializes aTxRecordto a writer using TLV format. The use of a buffer to encode the TLV stream before writing it to the provided writer is a good practice for handling potential encoding errors before any data is written.- 103-118: The
deserializeTxRecordfunction correctly deserializes aTxRecordfrom a reader using TLV format. This function is crucial for readingTxRecorddata from the database and ensuring data integrity through the TLV encoding scheme.- 260-275: The
StoreTxmethod insweeperStorecorrectly serializes theTxRecordand stores it in the database. The use ofkvdb.Updatefor database operations and handling serialization within the transaction scope is appropriate. This method aligns with the PR's objectives to enhance transaction management.- 332-379: The
GetTxmethod implementation is correct, handling both the case where the transaction is found and when it's not found (ErrTxNotFound). The method also correctly deserializes the transaction record if found. This method is essential for retrieving transaction records based on their IDs.- 381-391: The
DeleteTxmethod correctly removes a transaction record from the database using its ID. The use ofkvdb.Updateensures that the deletion is performed within a transaction, which is a good practice for database operations.docs/release-notes/release-notes-0.18.0.md (2)
- 355-357: The addition detailing the expanded functionality of the SweeperStore to include storage of feerate, fees paid, and publication status for sweeping transactions is clear and well-placed within the "Database" section. This enhancement aligns with the PR objectives and provides valuable information for users.
- 352-361: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-385]
The release notes are well-structured and provide a comprehensive overview of the changes in the release. Each section is clearly defined, and the inclusion of pull request links is helpful for users seeking more detailed information. The document effectively communicates the enhancements, fixes, and updates included in this release.
yyforyongyu
merged this pull request
into
lightningnetwork:elle-new-sweeper
|
Merged to side branch |
This PR prepares for #8424 by storing fee info in
SweeperStore. Due to the current data format used, no migration is needed to store this extra info. In addition, two methods,GetTxandDeleteTxare added to access the database.Summary by CodeRabbit
SweeperStorefunctionality to include detailed transaction information such as fee rate, fees paid, and publication status.TxRecordstruct for better transaction record management in the database.ErrLocktimeConflictto handle transactions with conflictingnLockTimevalues.SweeperStoreinterface and its mock implementation to enhance testability and functionality.sweepfunction logic inUtxoSweeperfor improved transaction tracking.SweeperStoreto ensure reliability and compatibility with new changes.