Feature/add srd selection by yancyribbens · Pull Request #22 · p2pderivatives/rust-bitcoin-coin-selection

yancyribbens · 2023-01-11T12:07:59Z

I have renamed and moved the random function to it's own module. This feature is similar to SRD in core. Also, I've added effective_value which is used for the sum calculation instead of value. Lastly I did some minor refactoring so that the unit sizes used in the test match core.

src/lib.rs

src/single_random_draw.rs

murchandamus · 2023-01-11T18:50:55Z

BTW, I added this repository to my watch list, so if you ask something here, I should also see.

src/single_random_draw.rs

Tibo-lg

I'm ok with using a struct though I'm not sure if it gives you more than a trait.

src/lib.rs

yancyribbens · 2023-01-26T18:07:07Z

@Tibo-lg I think the only downside to using a Trait instead of a struct is the increased compile time. A monomorphised type has to be constructed at compile time, although the run-time should be the same, so either is probably fine.

src/fee.rs

yancyribbens · 2023-02-21T15:12:44Z

I believe this PR is ready to merge. Any objections @Tibo-lg ?

murchandamus · 2023-02-21T21:46:52Z

src/fee.rs

+
+    #[test]
+    fn get_effective_value() {
+        let fee_rate = 10;


If you are going with integers for the feerate, you could consider using ṩ/kvB as the base unit, since it allows you to be more precise than just full satoshi increments. That way you can create transactions with feerates at 1.103 ṩ/vB, instead of needing to pay 2 ṩ/vB if 1 ṩ/vB is insufficient for the priority you want to imbue the transaction with.

I was also about to ask about how you avoid dust outputs, but then I saw the last commit. 👍

If you are going with integers for the feerate, you could consider using ṩ/kvB as the base unit

Thanks, that's a good point. Wouldn't it be better to use a float for rate?

Otherwise, here would be:

let kv_bytes = weight / 4 /1024;
which means we lose a lot of precision with int division OR kvbytes is a float. It doesn't work to have kvbytes be a float and feerate as an int then.

We don’t use “kibivirtualbytes”, but “kilovirtualbytes“:

1 MvB = 1000 kvB = 1 000 000 vB

We never use floats for consensus relevant calculations in Bitcoin Core to avoid any sort of rounding issues. YMMV.

How about something like this: d1d38b1

/// Takes as input the fee_rate in $ per Kilovirtualbytes /// and a size in Kilovirtualbytes and returns the fee amount. /// /// * fee_rate: $ per kilovirtualbytes $KvB /// * size: kilovirtualbytes KvB /// * fee: $

I just noticed this PR rust-bitcoin/rust-bitcoin#1627. Maybe it would be better to build off FeeRate as defined by Rust Bitcoin.

How about something like this: d1d38b1

/// Takes as input the fee_rate in $ per Kilovirtualbytes /// and a size in Kilovirtualbytes and returns the fee amount. /// /// * fee_rate: $ per kilovirtualbytes $KvB /// * size: kilovirtualbytes KvB /// * fee: $

It’s not a dollar symbol but an “s” with a dot above and below: ṩ (u+1e69). The symbol for kilo is “k”, not “K”, the latter is the symbol for Kelvin, so it’s kvB, and that makes the feerate ṩ/kvB (which without the slash would be a multiplication rather than a division).

src/fee.rs

src/single_random_draw.rs

murchandamus · 2023-06-28T15:30:04Z

Hey, this keeps popping up in my notifications but doesn’t seem to be ready for review. I’ll stop following notifications here for the time being. Please feel free to ping me here again when this PR is ready for review.

yancyribbens · 2023-06-29T11:08:45Z

Hey, this keeps popping up in my notifications but doesn’t seem to be ready for review. I’ll stop following notifications here for the time being. Please feel free to ping me here again when this PR is ready for review.

Sorry for all the noise. I think it's about ready now, however there seems to be some unrelated failure with the nightly toolchain I'm trying to figure out. Anyway, I think at this point I'll just close this PR and open a fresh one once I figure out what's going on with CI.

yancyribbens · 2023-07-03T10:33:38Z

@xekyo a new PR is now open here #23 - cheers.

yancyribbens force-pushed the feature/add-srd-selection branch 3 times, most recently from 4d4426f to bb0275b Compare January 11, 2023 12:18

yancyribbens mentioned this pull request Jan 11, 2023

Examine all possible combinations #21

Closed

yancyribbens requested a review from Tibo-lg January 11, 2023 12:24

yancyribbens force-pushed the feature/add-srd-selection branch from bb0275b to 679ad2e Compare January 11, 2023 13:19

murchandamus reviewed Jan 11, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/single_random_draw.rs Outdated Show resolved Hide resolved

Tibo-lg reviewed Jan 12, 2023

View reviewed changes

src/single_random_draw.rs Outdated Show resolved Hide resolved

yancyribbens force-pushed the feature/add-srd-selection branch 3 times, most recently from 72736d4 to b149195 Compare January 15, 2023 23:14

yancyribbens force-pushed the feature/add-srd-selection branch 2 times, most recently from 77f88e0 to d07e713 Compare January 24, 2023 17:24

Tibo-lg reviewed Jan 25, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

yancyribbens force-pushed the feature/add-srd-selection branch 3 times, most recently from ce627d8 to bd45a30 Compare January 26, 2023 18:04

yancyribbens force-pushed the feature/add-srd-selection branch 6 times, most recently from 684474a to 54f01e7 Compare January 31, 2023 09:16

Tibo-lg reviewed Jan 31, 2023

View reviewed changes

src/fee.rs Outdated Show resolved Hide resolved

yancyribbens force-pushed the feature/add-srd-selection branch 4 times, most recently from 4447ebd to a4dd20f Compare February 2, 2023 18:13

yancyribbens force-pushed the feature/add-srd-selection branch 13 times, most recently from 684b498 to 0fcf9b9 Compare February 11, 2023 15:55

yancyribbens force-pushed the feature/add-srd-selection branch 5 times, most recently from d1c63d7 to 48fc157 Compare February 21, 2023 15:10

murchandamus reviewed Feb 21, 2023

View reviewed changes

Tibo-lg reviewed Feb 22, 2023

View reviewed changes

src/fee.rs Outdated Show resolved Hide resolved

src/single_random_draw.rs Show resolved Hide resolved

yancyribbens force-pushed the feature/add-srd-selection branch 2 times, most recently from bca3356 to 882788c Compare February 22, 2023 16:01

yancyribbens added 3 commits July 3, 2023 12:25

Update MSRV

776648d

Disable benchmarks

2735178

Add single random draw selection algorithm

3ab168d

yancyribbens mentioned this pull request Jul 3, 2023

Feature/add srd selection module #23

Merged

Conversation

yancyribbens commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

murchandamus commented Jan 11, 2023

Uh oh!

Uh oh!

Tibo-lg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yancyribbens commented Jan 26, 2023

Uh oh!

Uh oh!

yancyribbens commented Feb 21, 2023

Uh oh!

murchandamus Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murchandamus Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

yancyribbens Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

murchandamus Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

yancyribbens Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

yancyribbens Feb 24, 2023

Choose a reason for hiding this comment

Uh oh!

Tibo-lg Feb 24, 2023

Choose a reason for hiding this comment

Uh oh!

murchandamus May 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

murchandamus commented Jun 28, 2023

Uh oh!

yancyribbens commented Jun 29, 2023

Uh oh!

yancyribbens commented Jul 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yancyribbens commented Jan 11, 2023 •

edited

Loading

murchandamus Feb 21, 2023 •

edited

Loading