Implement proper RBF API so you can satisfy rule 4 #19
Conversation
This file contains 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
Removes `min_fee` constraint in favour of proper rule 4 handling (min_fee was pretty useless). See for rule 4: https://github.com/bitcoin/bitcoin/blob/master/doc/policy/mempool-replacements.md#current-replace-by-fee-policy LowestFee BnB metric updated to bound correctly for this. A few other coin_selector API changes
LLFourn
commented
Jan 24, 2024
| @@ -14,7 +14,7 @@ fn test_wv(mut rng: impl RngCore) -> impl Iterator<Item = Candidate> { | |||
| value, | |||
| weight: rng.gen_range(0..100), | |||
| input_count: rng.gen_range(1..2), | |||
| is_segwit: rng.gen_bool(0.5), | |||
| is_segwit: false, | |||
There was a problem hiding this comment.
Note we never want to choose this randomly in the proptests because we cannot accurately lower bound it.
Removes `min_fee` constraint in favour of proper rule 4 handling (min_fee was pretty useless). See for rule 4: https://github.com/bitcoin/bitcoin/blob/master/doc/policy/mempool-replacements.md#current-replace-by-fee-policy LowestFee BnB metric updated to bound correctly for this. A few other coin_selector API changes
LLFourn
changed the title
Merged
evanlinjin
approved these changes
Jan 30, 2024
evanlinjin
added a commit
that referenced
this pull request
Mar 31, 2024
c650306 fix: `CoinSelector::implied_fee` (志宇) 4d0ae4c docs: fix typos (志宇) 2e26aa5 fix: rm `Drain::none` method (志宇) 8eb582b [lowest-fee] ignore failure when selection is not possible (LLFourn) 064996c [lowest-fee] Fix off by one in test (LLFourn) 956685a [changeless] Revert number of candidates (LLFourn) c6f0682 Remove typo in lowest_fee comment (LLFourn) 76e67a3 Move ChangePolicy to drain.rs (LLFourn) 7e82c3f Remove base_weight, put weight in Target (LLFourn) Pull request description: Fixes #1 On top of #19 - CoinSelector no longer tracks anything but input weight - Previously the value of the target outputs was in `Target` but the weights were accounted for in CoinSelector. Now they're in all in target. - This allows us to actually figure out how many outputs there are and therefore the actual weight of the transaction accounting for the varint for the number of outputs. This wasn't what the issue had in mind but it was easier to take the `base_weight` out of `CoinSelector` and put it in `Target` rather than put `Target` in `CoinSelector`. Getting rid of `base_weight` is a more crucial change than expected because rust bitcoin changed what `Transaction::weight` returns for empty output transactions recently so using it to determine `base_weight` will get different answers between versions (this breaks our weight tests but this PR will fix it I think if we uprade dev deps). We only need to know the total weight of the outputs and how many there are now to get the right answers for weight. ACKs for top commit: evanlinjin: ACK c650306 Tree-SHA512: bd2d6bba15a172b56451d13a9560b6221a6f005417857e1cf7f0cd7022cebd0c2e4d6ff78dac9b99690771297351f90eb3fea26f85aa34a2841c91cfe0d73f9c
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Removes
min_feeconstraint in favour of proper rule 4 handling (min_fee was pretty useless).See for rule 4:
https://github.com/bitcoin/bitcoin/blob/master/doc/policy/mempool-replacements.md#current-replace-by-fee-policy
LowestFee BnB metric updated to bound correctly for this.
A few other coin_selector API changes
Fixes: #12