💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3063245757)
Rebased for #32631

🤔 BrandonOdiwuor reviewed a pull request: "cmake: Create subdirectories in build tree in advance"
(https://github.com/bitcoin/bitcoin/pull/32773#pullrequestreview-3011416264)
Tested ACK 0b7d038972315b5537cc42038094902ebd5dd8cf

Confirmed that the subdirectories are created in the build tree before the symlinks preventing accidental COPY_ON_ERRORs like `invalid_signer.py`

Commit: 0b7d038972315b5537cc42038094902ebd5dd8cf
<img width="1274" height="103" alt="Screenshot 2025-07-11 at 20 51 32" src="https://github.com/user-attachments/assets/16f03539-b0e1-4d62-941b-33a34061355a" />

Branch: Master
<img width="1311" height="110" alt="Screenshot 2025-07-11 at 20 50
...

💬 instagibbs commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2201478752)
looks right, 20 maximally sized sigs should fit under limits

💬 JeremyRubin commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#issuecomment-3063383775)
i get that this is an overall improvement to the type safety, but I think that it's gone about sort of the wrong way since it leaves a lot of data-structures that are supposed to be made homogeneous as hetergeneous still. E.g.,

```
std::set<GenTxid> m_tx_inventory_to_send;
```

is _supposed_ to be `std::variant<std::set<Txid>, std::set<WTxid>>`.

You can create a templated wrapper to encapsulate a generic container interface for this.

💬 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.

💬 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)`?

💬 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.

💬 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?

💬 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`?

💬 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`?

✅ achow101 closed an issue: "Why bitcoin use so many RecursiveMutex"
(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.

💬 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.

💬 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
...

💬 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?

💬 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?

💬 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.

💬 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?

💬 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`.

🤔 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.