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

💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709766961)
@instagibbs @glozow Let me try giving an explanation here. If this helps you, I'll rework it into a comment in the code.

Think of the set of transactions in every work items as falling within one of four classes (roughly from "more likely to be included" to "less likely to be included"):
* **I**: definitely included transactions, (= `item.inc`).
* **P**: possibly included, and including any of them will definitely increase the included set's feerate (even when other lower-fee P transactions
...

🤔 tdb3 reviewed a pull request: "doc, chainparams: 29775 follow-ups"
(https://github.com/bitcoin/bitcoin/pull/30604#pullrequestreview-2228232965)
Concept ACK

💬 tdb3 commented on pull request "doc, chainparams: 29775 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1709789971)
Looks like `feature_config_args.py` needs to be updated to reflect the updated log. Something like:

```diff
diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py
index bb20e2baa85..201112ba0d5 100755
--- a/test/functional/feature_config_args.py
+++ b/test/functional/feature_config_args.py
@@ -378,7 +378,7 @@ class ConfArgsTest(BitcoinTestFramework):

def test_testnet3_deprecation_msg(self):
self.log.info("Test testnet3 deprecation
...

💬 tdb3 commented on pull request "doc, chainparams: 29775 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1709771804)
I'm partial to omitting a specific release number (e.g. and maybe including it just in release notes instead), but It's not something I feel super strongly about.

💬 zawy12 commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2276135560)
There's usually a "timewarp" attack if

1. monotonic timestamps are not enforced,
2. timespan limits are used (1/4 & 4x in BTC), and
3. miner has >50% hashrate.

It doesn't depend on the 2015/2016 "hole" in bitcoin's measurement of nActualtimespan.

The difficulty fix seems to still have a hole if nActualtimespan can be negative. Granted, the attack below requires 16 weeks. The fix is an improvement because the 3 conditions above usually require only 2.5 difficulty adjustment windows t
...

💬 fanquake commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709821764)
Was curious how much difference using `[[likely]]` may/may-not have, as I've seen mixed discussions around it's usage. For a single call to `EvaluateDown` (from the profile_estimate pass):


💬 pablomartin4btc commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2276180981)
> > "Migrate Wallet" is shown when there are no wallets available at all.
>
> I think that's ok.

Yeah, it was just an observation (mainly to compare when the menu is disabled), thanks!