Implement lowest fee metric correctly #13

LLFourn · 2023-12-19T23:05:43Z

This replaces #11. This first commit just fixes the metric to make the tests fail. Note the previous calculation was overthinking it. The fee metric is just inputs - outputs + long_term_feerate * change_weight.

Next steps:

Make ci actually run the tests and get them to fail.
Fix the metric lower bound

if is_target_met true it should continue to be true if you add a drain from a ChangePolicy. We should try and guarantee we never add a drain when it would invalidate the selection.

It may improve best score allowing us to avoid ever adding the children to the heap if their bound function doesn't think they can improve on it.

That proptest is not hitting for some reason.

LLFourn · 2023-12-22T04:21:56Z

This now includes a fix for the lowest fee metric. The bound function uses the simplification from #14 to precisely target the amount of negative effective value it could use to remove the change output which is the position of the possible lowest fee. This is where most of complexity of the code comes from and unfortunately the logic of this branch isn't being hit by the proptests. I tried playing around with the parameters of proptest and running a lot more times to hit it but it refused. I had to write a manual test to target this branch.

Also I made a change to branch and bound in 17cc8f2

We now score the branch before bounding its children. This allows the parent branch to improve the best score which may exclude its children (this leads to less iterations to find the best solution in one of the tests).

If we only add a drain when it wouldn't increase waste then it is impossible to decrease waste by adding a new input -- but the logic of the bound function has to account for this situation. So by using min_value_and_waste we never test it!

LLFourn · 2023-12-22T06:08:33Z

This is where most of complexity of the code comes from and unfortunately the logic of this branch isn't being hit by the proptests. I tried playing around with the parameters of proptest and running a lot more times to hit it but it refused.

I figured out why the tests weren't hitting it and fixed that: 9e1cecd

If we only add a drain when it wouldn't increase waste then it is
impossible to decrease waste by adding a new input -- but the logic of
the bound function has to account for this situation. So by using
min_value_and_waste we never test it!

evanlinjin

Thanks for taking this forward and fixing my screw-up. I'm liking how this is looking.

Most of my comments are nits, except for the change to the assert! statement in lowest_fee.rs - please have a second look at it!

README.md

evanlinjin · 2024-01-15T06:07:22Z

src/bnb.rs

+        let mut return_val = None;
+        if !branch.is_exclusion {
+            if let Some(score) = self.metric.score(&selector) {
+                let better = match self.best {
+                    Some(best_score) => score < best_score,
+                    None => true,
+                };
+                if better {
+                    self.best = Some(score);
+                    return_val = Some(score);
+                }
+            };
        }

-        let score = match self.metric.score(&selector) {
-            Some(score) => score,
-            None => return Some(None),
-        };
-
-        if let Some(best_score) = &self.best {
-            if score >= *best_score {
-                return Some(None);
-            }
-        }
-        self.best = Some(score);
-        Some(Some((selector, score)))
+        self.insert_new_branches(&selector);
+        Some(return_val.map(|score| (selector, score)))


Nit: I prefer the way it was written before as it was more readable for me.

evanlinjin · 2024-01-15T06:15:10Z

src/metrics/lowest_fee.rs

@@ -67,7 +67,7 @@ impl BnbMetric for LowestFee {

                if let Some((_, low_sats_per_wu_candidate)) = cs.unselected().next_back() {
                    let ev = low_sats_per_wu_candidate.effective_value(self.target.feerate);
-                    if ev < 0.0 {
+                    if ev < -0.0 {


Nit: Would this make a difference? In the f32 docs, it states that 0.0 == -0.0 when it comes to comparisons.

If it does make a difference, please have a comment as it is not immediately apparent.

evanlinjin · 2024-01-15T06:53:30Z

tests/lowest_fee.rs

+        let mut cs = cs.clone();
+        cs.select(0);
+        cs.select(2);
+        cs.excess(target, Drain::none());


Nit: do we need this? Was the intention to print the best solution's excess to check that it is greater than 0 for the test?

src/coin_selector.rs

src/metrics/lowest_fee.rs

tests/lowest_fee.rs

evanlinjin

ACK 6ae0fdf

f991504 chore: bump to v0.2 (志宇) Pull request description: # Changelog * Fix lowest fee metric (#13) * Simplify change policy (#14) Top commit has no ACKs. Tree-SHA512: a92bbc99ef5c1ab06fdb019916e3c88970ce989a7dd36471d486f4601b2ecc439a0787432994e9b14f0f8fbdfb031c3e5db9c5723068245db1b5f981cfc9f1e9

LLFourn force-pushed the fix-lowest-fee branch 3 times, most recently from 42f85c7 to cb70133 Compare December 21, 2023 07:55

LLFourn changed the title ~~Make lowest fee test fail by implementing score correctly~~ Implement lowest fee metric correctly Dec 22, 2023

LLFourn force-pushed the fix-lowest-fee branch from f242bff to 628c01e Compare December 22, 2023 03:35

LLFourn added 2 commits December 22, 2023 14:38

Rethink is_target_met

0c66696

if is_target_met true it should continue to be true if you add a drain from a ChangePolicy. We should try and guarantee we never add a drain when it would invalidate the selection.

Make lowest fee test fail by implementing score correctly

0aef6ff

LLFourn force-pushed the fix-lowest-fee branch 2 times, most recently from 38229e5 to dc46db3 Compare December 22, 2023 03:43

LLFourn added 3 commits December 22, 2023 15:20

Fix lowest_fee metric

e30246d

Score branches before adding children

17cc8f2

It may improve best score allowing us to avoid ever adding the children to the heap if their bound function doesn't think they can improve on it.

Write lowest fee test that hits important branch

7360052

That proptest is not hitting for some reason.

LLFourn force-pushed the fix-lowest-fee branch from dc46db3 to 7360052 Compare December 22, 2023 04:21

LLFourn requested a review from evanlinjin December 22, 2023 04:22

evanlinjin reviewed Jan 15, 2024

View reviewed changes

This was referenced Jan 15, 2024

Consider making Drain an enum, or using Option::<Drain>::None to represent no drain output(s). #16

Open

Make Changeless metric more useful when combined. #17

Open

docs: fix typos and use better wording

6ae0fdf

evanlinjin approved these changes Jan 15, 2024

View reviewed changes

evanlinjin merged commit 4023034 into bitcoindevkit:master Jan 15, 2024
5 checks passed

evanlinjin mentioned this pull request Jan 15, 2024

Release v0.2.0 #18

Merged

LLFourn mentioned this pull request Jan 25, 2024

Make LowestFee metric better #6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement lowest fee metric correctly #13

Implement lowest fee metric correctly #13

LLFourn commented Dec 19, 2023

LLFourn commented Dec 22, 2023 •

edited

Loading

LLFourn commented Dec 22, 2023

evanlinjin left a comment

evanlinjin Jan 15, 2024

evanlinjin Jan 15, 2024

evanlinjin Jan 15, 2024

evanlinjin left a comment

Implement lowest fee metric correctly #13

Implement lowest fee metric correctly #13

Conversation

LLFourn commented Dec 19, 2023

LLFourn commented Dec 22, 2023 • edited Loading

LLFourn commented Dec 22, 2023

evanlinjin left a comment

Choose a reason for hiding this comment

evanlinjin Jan 15, 2024

Choose a reason for hiding this comment

evanlinjin Jan 15, 2024

Choose a reason for hiding this comment

evanlinjin Jan 15, 2024

Choose a reason for hiding this comment

evanlinjin left a comment

Choose a reason for hiding this comment

LLFourn commented Dec 22, 2023 •

edited

Loading