Implementation for BNB Issue#1 #29

NeoZ666 · 2024-06-20T15:45:57Z

No description provided.

delcin-raj · 2024-06-23T07:34:31Z

make sure to run "cargo fmt" before committing changes

NeoZ666 · 2024-06-23T10:28:06Z

Done

NeoZ666 · 2024-06-26T06:32:25Z

Ran the cargo fmt command once again, please check.

delcin-raj

Add tests from core

delcin-raj · 2024-06-26T07:26:21Z

src/lib.rs

 ) -> Result<SelectionOutput, SelectionError> {
- unimplemented!()
+ let mut selected_inputs: Vec<usize> = vec![];
+ let bnb_tries = 1000000;


this should be a constant.

delcin-raj · 2024-06-26T07:27:34Z

src/lib.rs

+ };
+ Ok(selection_output)
+ }
+ None => select_coin_srd(inputs, options, &mut rand::thread_rng()),


Should be Err(NoSolutionFound)

The reasoning behind this is when the user calls the high level API, srd will be invoked regardless of the result of bnb.
So when a user calls bnb in particular signalling no solution found is better than returning the result of srd

src/lib.rs

delcin-raj · 2024-06-26T07:50:42Z

src/lib.rs

+ selected_inputs,
+ acc_eff_value,
+ depth + 1,
+ bnp_tries - 1,


bnp_tries - 2

because every recursive call should decrement this by one.
This variable is immutable

delcin-raj · 2024-06-26T07:50:55Z

src/lib.rs

+ selected_inputs,
+ new_effective_values,
+ depth + 1,
+ bnp_tries - 1,


bnp_tries - 2

delcin-raj · 2024-06-26T07:52:28Z

src/lib.rs

@@ -358,6 +483,11 @@ fn effective_value(output: &OutputGroup, feerate: f32) -> u64 {
 .saturating_sub(calculate_fee(output.weight, feerate))
 }

+fn generate_random_bool(rng: &mut ThreadRng) -> bool {


Remove this function

delcin-raj · 2024-06-26T07:53:36Z

src/lib.rs

@@ -248,17 +375,15 @@ pub fn select_coin_fifo(
 pub fn select_coin_srd(


Undo the changes to this function

delcin-raj · 2024-06-26T07:54:03Z

src/lib.rs

@@ -541,7 +706,7 @@ mod test {
 fn test_insufficient_funds() {
 let inputs = setup_basic_output_groups();
 let options = setup_options(7000); // Set a target value higher than the sum of all inputs
- let result = select_coin_srd(&inputs, options);
+ let result = select_coin_srd(&inputs, options, &mut rand::thread_rng());


undo the changes

delcin-raj · 2024-06-26T07:54:09Z

src/lib.rs

 }

 fn test_successful_selection() {
 let mut inputs = setup_basic_output_groups();
 let mut options = setup_options(2500);
- let mut result = select_coin_srd(&inputs, options);
+ let mut result = select_coin_srd(&inputs, options, &mut rand::thread_rng());


undo the changes

aruokhai · 2024-07-10T14:27:58Z

src/lib.rs

+ } else if acc_eff_value >= target_for_match {
+ return Some(selected_inputs.to_vec());
+ } else if bnp_tries <= 0 || depth >= inputs_in_desc_value.len() {
+ return None;
+ } else {


A series of if clauses without the usage ofif-else clause would be more readable and also the else clause is redundant.

delcin-raj · 2024-08-15T10:15:18Z

src/lib.rs

- unimplemented!()
+ rng: &mut ThreadRng,
+) -> Option<Vec<usize>> {
+ let target_for_match = options.target_value


This value could be computed only once as it depends on the options
No need to recompute this during every recursive call.
This can be included in the function parameter

delcin-raj · 2024-08-15T10:16:56Z

src/lib.rs

+ let target_for_match = options.target_value
+ + calculate_fee(options.base_weight, options.target_feerate)
+ + options.cost_per_output;
+ let match_range = options.cost_per_input + options.cost_per_output;


delcin-raj · 2024-08-15T10:19:01Z

src/lib.rs

+ if rng.gen_bool(0.5) {
+ // exploring the inclusion branch
+ // first include then omit
+ let new_effective_values =


it's a single value

delcin-raj · 2024-08-15T10:19:50Z

src/lib.rs

- selected_inputs: &[usize],
- effective_value: u64,
+ selected_inputs: &mut Vec<usize>,
+ acc_eff_value: u64,
 depth: usize,
 bnp_tries: u32,
 options: &CoinSelectionOpt,


This argument options could potentially be discarded

delcin-raj · 2024-08-15T10:22:27Z

src/lib.rs

+ Some(_) => with_this,
+ None => {
+ selected_inputs.pop(); //poping out the selected utxo if it does not fit
+ let without_this = bnb(


Just return the result of the bnb call here.
No need for without_this variable

delcin-raj · 2024-08-15T10:23:48Z

src/lib.rs

- selected_inputs: &[usize],
- effective_value: u64,
+ selected_inputs: &mut Vec<usize>,
+ acc_eff_value: u64,
 depth: usize,
 bnp_tries: u32,


Correct it to bnb_tries

delcin-raj · 2024-09-02T15:38:23Z

src/lib.rs

 pub waste: WasteMetric,
 }

+/// Wrapped in a struct or else input for fn bnb takes too many arguments - 9/7, 


Fix this clippy error

delcin-raj · 2024-09-02T15:38:49Z

src/lib.rs

+ Ok(selection_output)
+ }
+ None => Err(SelectionError::NoSolutionFound),
+ }
 }

 /// Return empty vec if no solutions are found


clippy doc error

delcin-raj · 2024-09-06T06:47:54Z

Waiting for the updates...

delcin-raj

LGTM

delcin-raj · 2024-09-16T08:42:21Z

Commits are not signed.
Fix fmt and clippy errors.

Signed-off-by: neoz666 <[email protected]>

delcin-raj · 2024-09-21T13:53:21Z

src/lib.rs

+ }
+ }
+ } else {
+ /// Avoided creation of intermediate variable, for marginally better performance.


remove this comment, confusing to the people who don't have context

delcin-raj · 2024-09-21T13:54:43Z

src/lib.rs

+ Some(_) => with_this,
+ None => {
+ selected_inputs.pop(); // poping out the selected utxo if it does not fit
+ None // this may or may not be correct


This is correct, so remove the comment

Signed-off-by: neoz666 <[email protected]>

delcin-raj

LGTM

delcin-raj reviewed Jun 26, 2024

View reviewed changes

aruokhai reviewed Jul 10, 2024

View reviewed changes

delcin-raj reviewed Aug 15, 2024

View reviewed changes

delcin-raj requested changes Aug 15, 2024

View reviewed changes

NeoZ666 mentioned this pull request Aug 30, 2024

Implement BNB Selection #1

Closed

delcin-raj requested changes Sep 2, 2024

View reviewed changes

NeoZ666 force-pushed the main branch from 1ca7949 to 90a1ecb Compare September 10, 2024 20:35

delcin-raj approved these changes Sep 16, 2024

View reviewed changes

NeoZ666 force-pushed the main branch 4 times, most recently from 4f7dcd2 to a8f896b Compare September 16, 2024 15:30

NeoZ666 added 7 commits September 20, 2024 06:52

Formatted OK

cc835d2

Signed-off-by: neoz666 <[email protected]>

Changes made: bnb_tries, struct, vars

34664ac

Signed-off-by: neoz666 <[email protected]>

Updated with Sucess Test case

6547d59

Signed-off-by: neoz666 <[email protected]>

Updates

fab6521

Signed-off-by: neoz666 <[email protected]>

Updates made, clippy fixed

2a6f20f

Signed-off-by: neoz666 <[email protected]>

Updates

f3491ab

Signed-off-by: neoz666 <[email protected]>

Updates cargo fmt, clippy

8d9e970

Signed-off-by: neoz666 <[email protected]>

NeoZ666 force-pushed the main branch from e2e14f6 to 8d9e970 Compare September 20, 2024 01:24

NeoZ666 added 2 commits September 20, 2024 06:56

Made all changes

c80152c

Merge with upstream main

9077ad5

delcin-raj requested changes Sep 21, 2024

View reviewed changes

NeoZ666 added 2 commits September 21, 2024 20:36

Removed redundant comments

a35692d

Signed-off-by: neoz666 <[email protected]>

Merge branch 'main' of https://github.com/NeoZ666/rust-coinselect

7f2bbb0

delcin-raj approved these changes Sep 21, 2024

View reviewed changes

delcin-raj merged commit bf812ac into Bitshala-Incubator:main Sep 21, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation for BNB Issue#1 #29

Implementation for BNB Issue#1 #29

NeoZ666 commented Jun 20, 2024

delcin-raj commented Jun 23, 2024

NeoZ666 commented Jun 23, 2024

NeoZ666 commented Jun 26, 2024

delcin-raj left a comment

delcin-raj Jun 26, 2024

delcin-raj Jun 26, 2024

delcin-raj Jun 26, 2024

delcin-raj Jun 26, 2024

delcin-raj Jun 26, 2024

delcin-raj Jun 26, 2024

delcin-raj Jun 26, 2024

delcin-raj Jun 26, 2024

delcin-raj Jun 26, 2024

delcin-raj Jun 26, 2024

aruokhai Jul 10, 2024

delcin-raj Aug 15, 2024

delcin-raj Aug 15, 2024

delcin-raj Aug 15, 2024

delcin-raj Aug 15, 2024

delcin-raj Aug 15, 2024

delcin-raj Aug 15, 2024

delcin-raj Sep 2, 2024

delcin-raj Sep 2, 2024

NeoZ666 Sep 21, 2024

delcin-raj commented Sep 6, 2024

delcin-raj left a comment

delcin-raj commented Sep 16, 2024

delcin-raj Sep 21, 2024

NeoZ666 Sep 21, 2024

delcin-raj Sep 21, 2024

delcin-raj left a comment

		@@ -248,17 +375,15 @@ pub fn select_coin_fifo(
		pub fn select_coin_srd(

Implementation for BNB Issue#1 #29

Implementation for BNB Issue#1 #29

Conversation

NeoZ666 commented Jun 20, 2024

delcin-raj commented Jun 23, 2024

NeoZ666 commented Jun 23, 2024

NeoZ666 commented Jun 26, 2024

delcin-raj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delcin-raj commented Sep 6, 2024

delcin-raj left a comment

Choose a reason for hiding this comment

delcin-raj commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delcin-raj left a comment

Choose a reason for hiding this comment