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

💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709648591)
No, we *always* want to consider transactions that were missing from `pot`, because those couldn't have been considered for the parent work item. `reconsider_pot` only controls whether we should reconsider the ones *already* in `pot`.

💬 ryanofsky 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-2275991612)
> but how do you link it to this failure?

It seems pretty clear to me the suggested fix in https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2274237242 will prevent this failure because it removes the assert and handles the case where UnloadWalllet is called by more than one thread, instead of aborting. I agree it would be nice to understand more about this failure, though, because maybe there are more problems and this fix isn't sufficient.

💬 maflcko 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-2276001890)
> I think we are missing something here. Probably the assertion failure is a consequence of another issue that triggers a shutdown during the concurrent wallet load/unload.

Yes, the shutdown happens. See for example https://cirrus-ci.com/task/5752995407724544 , which fails without an assert (by accident), but shuts down.

💬 achow101 commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1709670989)
Fixed during rebase.

💬 achow101 commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1709671154)
Done

💬 achow101 commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1709671373)
Removed

💬 achow101 commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2276010915)
> Tested [d45ac03](https://github.com/bitcoin-core/gui/commit/d45ac03a89cc500cd461f878f092cf8ec99e7760). The "Migrate Wallet" menu item is still disabled when no wallet is loaded.

Should be fixed now.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2276023703)
:tada: Thanks everyone!

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2276094102)
Rebased and I took @itornaza's patch with some changes, see inline comments on https://github.com/bitcoin/bitcoin/commit/2d28478d0791689070bd23df5b5640e9dae6f786.