sweepbatcher: make sure feerate does not decrease #833
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hieblmi
approved these changes
Oct 15, 2024
sweepbatcher/sweep_batch.go
Outdated
| } | ||
|
|
||
| // Update batch's fee rate to be greater than or equal to | ||
| // minFeeRate of the sweep. |
Collaborator
There was a problem hiding this comment.
Could you add the reason for it?
Collaborator
Author
There was a problem hiding this comment.
Done:
// Update batch's fee rate to be greater than or equal to
// minFeeRate of the sweep. Make sure batch's fee rate does not
// decrease (otherwise it won't pass RBF rules and won't be
// broadcasted) and that it is not lower that minFeeRate of
// other sweeps (so it is applied).|
|
||
| batcher := NewBatcher(lnd.WalletKit, lnd.ChainNotifier, lnd.Signer, | ||
| testMuSig2SignSweep, testVerifySchnorrSig, lnd.ChainParams, | ||
| batcherStore, sweepStore, WithCustomFeeRate(customFeeRate)) |
sweepbatcher/sweep_batcher_test.go
Outdated
| require.Len(t, batch.sweeps, 1) | ||
| require.Equal(t, feeRateMedium, batch.rbfCache.FeeRate) | ||
|
|
||
| // Now decreate the fee of sweep1. |
| setFeeRate(swapHash1, feeRateLow) | ||
| require.NoError(t, batcher.AddSweep(&sweepReq1)) | ||
|
|
||
| // Tick tock next block. |
| <-lnd.TxPublishChannel | ||
|
|
||
| // Make sure the fee rate is still feeRateMedium. | ||
| require.Equal(t, feeRateMedium, batch.rbfCache.FeeRate) |
sweepbatcher/sweep_batcher_test.go
Outdated
| require.Len(t, batch.sweeps, 2) | ||
| require.Equal(t, feeRateMedium, batch.rbfCache.FeeRate) | ||
|
|
||
| // Now update fee rate of second sweep (which is not promary) to |
Previous behaviour was to overwrite batch's feerate with minFeeRate of its primary sweep, which could be lower that previus batch's feerate or lower that feerate of some other sweep. Instead, batch's feerate only grows and never declines and is at least as high as the highest feerate of its sweeps. Added a test to verify this.
starius
force-pushed
the
sweepbatcher-feerate-grows
branch
from
dabc74a to
864d7f2
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previous behaviour was to overwrite batch's feerate with minFeeRate of its primary sweep, which could be lower that previus batch's feerate or lower that feerate of some other sweep.
Instead, batch's feerate only grows and never declines and is at least as high as the highest feerate of its sweeps.
Added a test to verify this.
Pull Request Checklist
release_notes.mdif your PR contains major features, breaking changes or bugfixes