💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201577538)
In a similar test, I run `SelectCoinsBnB` twice and swap fee_rate and long_term_fee_rate, then assert that when long_term_fee_rate is greater less outputs (or equal) are found between the two runs.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201577538)
In a similar test, I run `SelectCoinsBnB` twice and swap fee_rate and long_term_fee_rate, then assert that when long_term_fee_rate is greater less outputs (or equal) are found between the two runs.
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201595887)
could not `cost_of_change` be randomly selected from a range `(0, MAX)`?
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201595887)
could not `cost_of_change` be randomly selected from a range `(0, MAX)`?
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201606218)
It looks like a downside to this brute force approach is it only allows pool sizes up to 16. Curious if you consider just pre-generating a solution which would allow any theoretical size.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201606218)
It looks like a downside to this brute force approach is it only allows pool sizes up to 16. Curious if you consider just pre-generating a solution which would allow any theoretical size.
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201612657)
I suppose `MAX` could be `MAX_MONEY` here?
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201612657)
I suppose `MAX` could be `MAX_MONEY` here?
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201638315)
doesn't this create pools that could sum to greater than `MAX_MONEY`?
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201638315)
doesn't this create pools that could sum to greater than `MAX_MONEY`?
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201659630)
I think there should be something that prevents `max_spendable` from being greater than `MAX_MONEY`?
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201659630)
I think there should be something that prevents `max_spendable` from being greater than `MAX_MONEY`?
✅ achow101 closed an issue: "Why bitcoin use so many RecursiveMutex"
(https://github.com/bitcoin/bitcoin/issues/32947)
(https://github.com/bitcoin/bitcoin/issues/32947)
💬 achow101 commented on issue "Why bitcoin use so many RecursiveMutex":
(https://github.com/bitcoin/bitcoin/issues/32947#issuecomment-3063584446)
Stop opening duplicate issues.
(https://github.com/bitcoin/bitcoin/issues/32947#issuecomment-3063584446)
Stop opening duplicate issues.
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#issuecomment-3063652119)
Concept ACK adding a test that shows BnB finds the best solution (min waste). The downside to this approach however is it limits solutions to 16 and theoretically BnB could find solutions of larger sets without hitting the maximum iteration limit. Code looks good though otherwise.
I think this should be prioritized to merge first over https://github.com/bitcoin/bitcoin/pull/32150 so that it can add more test safety to that refactor.
(https://github.com/bitcoin/bitcoin/pull/32894#issuecomment-3063652119)
Concept ACK adding a test that shows BnB finds the best solution (min waste). The downside to this approach however is it limits solutions to 16 and theoretically BnB could find solutions of larger sets without hitting the maximum iteration limit. Code looks good though otherwise.
I think this should be prioritized to merge first over https://github.com/bitcoin/bitcoin/pull/32150 so that it can add more test safety to that refactor.
💬 TheCharlatan commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3063715290)
> I think it makes more sense that some of our binaries should be command line binaries presenting stable, backward-compatible CLIs, and other binaries can be internal binaries that do useful things for us but users are not expected to use directly and we are free to change, rename, merge, or split up in the future.
It's not clear to me how the approach here really helps with that though. Essentially it just adds a tiny extra step for the user to use these binaries. I think the utility of the
...
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3063715290)
> I think it makes more sense that some of our binaries should be command line binaries presenting stable, backward-compatible CLIs, and other binaries can be internal binaries that do useful things for us but users are not expected to use directly and we are free to change, rename, merge, or split up in the future.
It's not clear to me how the approach here really helps with that though. Essentially it just adds a tiny extra step for the user to use these binaries. I think the utility of the
...
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2201763587)
Nit: worth checking that global usage is not larger than the sum of the per-peer usages, and that global total latency score is not larger than the sum of the per-peer latency scores?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2201763587)
Nit: worth checking that global usage is not larger than the sum of the per-peer usages, and that global total latency score is not larger than the sum of the per-peer latency scores?
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2201767042)
Are these `MIN_PEER` and `MAX_PEER` values needed in the `.h` file?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2201767042)
Are these `MIN_PEER` and `MAX_PEER` values needed in the `.h` file?
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2201780921)
That's unfortunate, as it means the simulation isn't exercising anything involving nontrivial latency scores.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2201780921)
That's unfortunate, as it means the simulation isn't exercising anything involving nontrivial latency scores.
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2201802869)
I haven't thought a lot about why, but what's the challenge when adding more than 9 inputs for the simulator?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2201802869)
I haven't thought a lot about why, but what's the challenge when adding more than 9 inputs for the simulator?
💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201723358)
In commit "optimization: migrate fixed-size obfuscation from `std::vector<std::byte>` to `uint64_t`" (342bb224bb96b09d9950a144e62e60ede8710191)
This is multiplying a `size_t` value (which is unsigned) by -1 which doesn't seem right. Probably `size_t` should be changed to `int` which is the size accepted by `std::rotr`.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201723358)
In commit "optimization: migrate fixed-size obfuscation from `std::vector<std::byte>` to `uint64_t`" (342bb224bb96b09d9950a144e62e60ede8710191)
This is multiplying a `size_t` value (which is unsigned) by -1 which doesn't seem right. Probably `size_t` should be changed to `int` which is the size accepted by `std::rotr`.
🤔 ryanofsky reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3011607393)
Code review 56903b682bf308243ce5329b7aa8363dc498d16a. Started reviewing this going through the whole PR, but mostly focusing on the second to last commit, and left some comments. Overall the change seems great for performance in the microbenchmarks , and the commits seem nicely broken up, and the naming / API changes all seem clean and make sense.
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3011607393)
Code review 56903b682bf308243ce5329b7aa8363dc498d16a. Started reviewing this going through the whole PR, but mostly focusing on the second to last commit, and left some comments. Overall the change seems great for performance in the microbenchmarks , and the commits seem nicely broken up, and the naming / API changes all seem clean and make sense.
💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201680165)
In commit "optimization: migrate fixed-size obfuscation from `std::vector<std::byte>` to `uint64_t`" (342bb224bb96b09d9950a144e62e60ede8710191)
IMO, it is haphazard for this code to define a `KEY_SIZE` constant and avoid hardcoding key sizes most places, but not define a key type and literally hardcode `uint64_t` and `Uint64` in other places.
It would seem better to either hardcode the key size consistently or not hardcode it. Would suggest not hardcoding it:
```diff
diff --git a/src/o
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201680165)
In commit "optimization: migrate fixed-size obfuscation from `std::vector<std::byte>` to `uint64_t`" (342bb224bb96b09d9950a144e62e60ede8710191)
IMO, it is haphazard for this code to define a `KEY_SIZE` constant and avoid hardcoding key sizes most places, but not define a key type and literally hardcode `uint64_t` and `Uint64` in other places.
It would seem better to either hardcode the key size consistently or not hardcode it. Would suggest not hardcoding it:
```diff
diff --git a/src/o
...
💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201798917)
In commit "refactor: encapsulate `vector`/`array` keys into `Obfuscation`" (74e33e7793bfee10a8aac3503b5c0a6db75aa787)
This method does not seem safe to be a public method since unlike the other public methods, it will gives different results depending on whether the this is big or little endian machine. It seems not very useful to expose and like a potential footgun.
Would suggest making this private since it only seems to be called by tests, and if the tests are important would recommend
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201798917)
In commit "refactor: encapsulate `vector`/`array` keys into `Obfuscation`" (74e33e7793bfee10a8aac3503b5c0a6db75aa787)
This method does not seem safe to be a public method since unlike the other public methods, it will gives different results depending on whether the this is big or little endian machine. It seems not very useful to expose and like a potential footgun.
Would suggest making this private since it only seems to be called by tests, and if the tests are important would recommend
...
💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201593284)
In commit "refactor: encapsulate `vector`/`array` keys into `Obfuscation`" (74e33e7793bfee10a8aac3503b5c0a6db75aa787)
Would suggest some changes to make usage of this Xor() function clearer:
- Instead taking separate span and size values then ignoring the size of the span, just take a span and use its actual size.
- Add an assert to ensure that size is not greater than 8 bytes.
- Rename from Xor to XorWord to make it obvious that this only useful for dealing with short spans not arbitrar
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201593284)
In commit "refactor: encapsulate `vector`/`array` keys into `Obfuscation`" (74e33e7793bfee10a8aac3503b5c0a6db75aa787)
Would suggest some changes to make usage of this Xor() function clearer:
- Instead taking separate span and size values then ignoring the size of the span, just take a span and use its actual size.
- Add an assert to ensure that size is not greater than 8 bytes.
- Rename from Xor to XorWord to make it obvious that this only useful for dealing with short spans not arbitrar
...
💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201705480)
In commit "refactor: encapsulate `vector`/`array` keys into `Obfuscation`" (74e33e7793bfee10a8aac3503b5c0a6db75aa787)
This seems more like a runtime deserialization error than something that is probably a programming error. Would suggest throwing `std::ios_base::failure` instead of `logic_error`.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201705480)
In commit "refactor: encapsulate `vector`/`array` keys into `Obfuscation`" (74e33e7793bfee10a8aac3503b5c0a6db75aa787)
This seems more like a runtime deserialization error than something that is probably a programming error. Would suggest throwing `std::ios_base::failure` instead of `logic_error`.