👍 theStack approved a pull request: "Replace non-standard CLZ builtins with c++20's bit_width"
(https://github.com/bitcoin/bitcoin/pull/29057#pullrequestreview-1776458843)
Code-review ACK 273e4dc8da41f113b21a7ece54988b7bddf3612e

Unit tests succeeded locally :heavy_check_mark:

Seems reasonable to keep `CountBits` in a first step with its tests and fuzzers for reassurance and then replace all of its call-sites. Happy to re-review this PR or a follow-up, depending on where you decide to put the next commit.

💬 mzumsande commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1423315657)
Heh, I actually changed it from ? to * last thing before opening the PR, because * was already used in a few other places for netinfo and ? would be a new symbol. But maybe ? is more natural here, @jonatack do you have an opinion?

💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1423327495)
I see what happened. Knapsack is finding a changeless solution in the first attempt where it should look for something with change because `min_change_target` was set to zero

🤔 murchandamus reviewed a pull request: "wallet: skip BnB when SFFO is enabled"
(https://github.com/bitcoin/bitcoin/pull/28994#pullrequestreview-1776489114)
ACK 18669d753a15f997f7363dcaf0abf230b851b224

💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1423332614)
Setting `min_change_target` to 0 makes Knapsack always look for changeless solutions

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1423339033)
Fixed. Thanks.

🤔 murchandamus reviewed a pull request: "wallet: skip BnB when SFFO is enabled"
(https://github.com/bitcoin/bitcoin/pull/28994#pullrequestreview-1776504690)
ACK 18669d753a15f997f7363dcaf0abf230b851b224

💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1423344023)
Good, now I’d expect:

- SRD randomly returning two or three of the inputs
- Knapsack return { COIN+coc, 0.5×COIN }
- BnB return { COIN+coc }

Since the feerate is higher than the LFTR, two or three inputs with change are worse than one input without change, even when dropping coc into fees. Ergo, BnB not being the first solution proves that BnB wasn’t run.

👋 mzumsande's pull request is ready for review: "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo"
(https://github.com/bitcoin/bitcoin/pull/29058)

💬 conduition commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1851367175)
> Yes and it doesn't bother me because they are financial txs.

@Retropex Can I ask your opinion on a hypothetical? I want to understand your perspective by revisiting a scenario i posed earlier.

> If i publish a chain of 1000 transactions pointlessly bouncing coins around my own wallet addresses, is that spam? What about a single big (in terms of weight) transaction which only pays from myself to myself? The whole BTC network (including myself) would be better off if i didn't do that. I'm
...

💬 mmgen commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1851389569)
The Bitcoin whitepaper is embedded in the blockchain in a [transaction](https://blockstream.info/tx/54e48e5f5c656b26c3bca14a8c95aa583d07ebe84dde3b7dd4a78f4e4186e713) with 946 1-satoshi multisig outputs.

What’s to prevent any arbitrary data from being embedded in this way, and in other ways this PR doesn’t address?

And at the risk of belaboring the obvious, I'll repeat: any mempool-based approach to filter transactions is futile, because transactions don’t require the mempool to get into a
...

💬 petertodd commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1851494243)
On Mon, Dec 11, 2023 at 10:39:42PM -0800, The MMGen Project wrote:
> The Bitcoin whitepaper is embedded in the blockchain in a [transaction](https://blockstream.info/tx/54e48e5f5c656b26c3bca14a8c95aa583d07ebe84dde3b7dd4a78f4e4186e713) with 946 1-satoshi multisig outputs.
>
> What’s to prevent any arbitrary data from being embedded in this way, and in other ways this PR doesn’t address?

It doesn't even stop https://github.com/petertodd/python-bitcoinlib/blob/master/examples/publish-text.py, whi
...

💬 S3RK commented on pull request "tests, bench: Fix issue with CWallet::LoadWallet() being called in the wrong places":
(https://github.com/bitcoin/bitcoin/pull/29055#issuecomment-1851504017)
ACK bd7f5d33e3b9e01cd600afe1682b7afa935f68a3

💬 TheCharlatan commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1851506064)
Re https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1850970368

> But what else do you want beyond that? And how does removing sock.cpp from the util library help, for example?

Maybe removing `sock.cpp` is a bad example, since it is already impossible to link against it. `sock.cpp` uses modules that are already not part of the kernel, so trying to link it in will result in errors. Then again, if `sock.cpp` uses common modules, why is it in `util` in the first place? To provide ano
...

💬 hebasto commented on pull request "Replace non-standard CLZ builtins with c++20's bit_width":
(https://github.com/bitcoin/bitcoin/pull/29057#issuecomment-1851535346)
Concept ACK.

💬 vasild commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1423659099)
> This has nothing to do with C++20

ok, rephrase (I mentioned C++20 because C++17 does not have `char8_t`):

What would be an example where it is broken by calling `string()` (1.) and ok by converting `wchar_t`->`char8_t`->`char` (2.2)?

💬 naumenkogs commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1851561925)
Looking at the diff between the two versions, I see these changes:
1. which peers are taken into account (`FEELER`, `ADDRFETCH`, `BLOCKRELAY` are also considered)
2. the 199-bug is brought back
3. fixed addition overflow; an alternative would divide each element by 2 and then add without overflow risk, it saves us the sloppiness `int64_max/2`; but i don't care that much)

Not sure i see `keeps warning condition for a large median time offset as it is on master`.

Generally, while I think
...

💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1851608976)
ACK https://github.com/bitcoin/bitcoin/commit/576bee88fd36e207b7288077626947a1fce0fc33

💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1851611159)
> ACK [18669d7](https://github.com/bitcoin/bitcoin/commit/18669d753a15f997f7363dcaf0abf230b851b224)

guessing you meant to ack https://github.com/bitcoin/bitcoin/commit/576bee88fd36e207b7288077626947a1fce0fc33 ? :wink:

💬 hebasto commented on pull request "Replace non-standard CLZ builtins with c++20's bit_width":
(https://github.com/bitcoin/bitcoin/pull/29057#issuecomment-1851613999)
> ... but I wanted to allow our tests/fuzzers to run with this change first to help convince reviewers that `std::bit_width` is a drop-in replacement for`CountBits`.

I'm convinced :)