💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1709509545)
Update: `bip324.h` also uses `std::array`

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709526856)
> the performance of the method seems to be important

How so? There is one callsite, which can be called at most once, to parse one optional debug-only option.

> Could we either return an std::optional<std::string_view>

How would that work? `std::string_view` is a non-owning view.

> we're always copying the result before returning

In 802374b4355bd1dec7a88bba6287c55f935699fe, we're always allocating exactly once. In your example, we'd be allocating at least once, and twice if we ne
...

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709538299)
```C++
input = util::RemovePrefixView(input, "0x");
```
This line gives me the heebie-jeebies if input is `std::string`. It might be okay as we are assigning a shorter string, but worst case, the assignment operator could free it's prior heap buffer and allocate a shorter buffer, before attempting to copy from the string_view which now might reference the deallocated buffer. Using `RemovePrefix()` should alleviate the problem.

💬 glozow commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709543030)
Question: why did you make this a templated function instead of adding a bool parameter? Is it a compile-time optimization?

💬 furszy commented on issue "ci: failure in `wallet_multiwallet.py --legacy-wallet` - (`void wallet::UnloadWallet(std::shared_ptr<CWallet> &&): Assertion 'it.second' failed.`)":
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2275892677)
> But the "unloadwallet RPC call at the same time as a shutdown" case I mentioned can happen because UnloadWallets which runs on shutdown makes a copy of the wallets vector, so if an unloadwallet RPC call happens at the same time as that they can both call UnloadWallet on the same wallet.

I agree that there is an issue there, but how do you link it to this failure? No shutdown should be executed along the "Concurrent wallet loading" test case. The scenario merely involves two or more threads
...

💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709562802)
Yeah, exactly. I think we want this function to be fast (and the rounding mode is always known at compile time anyway), while the `Div` functions are only needed in exceptional cases (negative fees, or >20 BTC fees) anyway.

💬 glozow commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709566877)
Makes sense!

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709568559)
Force pushed to use `std::cerr` since it seems everyone can get behind that, thanks for the suggestion.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709569185)
I've tested on macOS Monterey 12.7.6 (Intel) with Homebrew's `llvm@16`. Everything works flawlessly.

> Is there any way I could help here?

Mind providing full _verbose_ build logs for both Autotools and CMake please?

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2275904176)
Force pushed to [terminate with `std::abort()`](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709568559) in case of invalid `RANDOM_CTX_SEED` input.

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709581953)
LGTM

💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709591119)
I don't think so: https://godbolt.org/z/vYr4Tsbde

The implicitly-defined `operator!=` falls back to using `operator==`. Only the implicitly defined `operator<`, `operator>`, `operator<=`, `operator>=` falls back to using `operator<=>`.

💬 jonatack commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1709596999)
> Bitcoin Core had a strong preference to connect to peers that listen on port `8333`. Is this what you mean? That was removed at some point.

Yes, that preference for clearnet connections was removed in https://github.com/bitcoin/bitcoin/pull/23542.

💬 fjahr commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2275938528)
Concept ACK

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709617445)
I think this has helped a lot, "clusterlin: avoid recomputing potential set on every split (optimization)" now makes sense to me. Lots of interlocking parts, so having small bite-sizes helps.

Can you motivate why `reconsider_pot` is set as true on inclusion of `anc` and not on exclude?

When `anc` is included, it's included in both `inc` and `pot`, then:

`if (reconsider_pot) consider_inc = pot.transactions - inc.transactions;`

so no part of `anc` will be in `consider_inc`.

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709618160)
Will let you decide, there are multiple ways to do this, each would be slightly better in a different scenario.
Whichever produces the cleanest code, choose that :)

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2275958345)
ACK 855784d3a0026414159acc42fceeb271f8a28133

💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2275970516)
> This PR carves out the few `uint256S` callsites that may potentially prove a bit more controversial to change because they deal with user input and - potentially - backwards incompatible behaviour change.
>
> After this PR, the remaining work to remove `uint256S` completely is almost entirely mechanical and/or test related. I will open that PR once #30560 is merged because it builds on that.

I think the description can be clarified that there are two user-facing settings that are now str
...

💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709211453)
in c68d26a0e97d7544076f00a467a8eca16658077a

If we're meant to avoid jump ahead, should this be gated on `reconsider_pot` as well?

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2275982831)
> I think the description can be clarified that there are two user-facing settings that are now stricter checked.

Thanks, added a section on introduced behaviour change.