📝 hebasto opened a pull request: "ci: Temporarily disable `WalletMigration` benchmark"
(https://github.com/bitcoin/bitcoin/pull/32306)
The `WalletMigration` benchmark is currently failing on CI.

This PR temporarily disables it until the issue is resolved.

An alternative to https://github.com/bitcoin/bitcoin/pull/32302.

💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2816244694)
> `-legacy` argument shouldn't be removed from `bitcoin-wallet`/ `wallettool.cpp`? (leaving `-descriptors` `true` by default):

Good catch! Added a commit to disable legacy wallet creation from the wallet tool.

💬 hebasto commented on pull request "ci: Temporarily revert "Drop bench -priority-level in win cross CI"":
(https://github.com/bitcoin/bitcoin/pull/32302#issuecomment-2816244798)
> Can we add a different change...

An alternative has been suggested in https://github.com/bitcoin/bitcoin/pull/32306.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2051118275)
Let me know if you think the final commit in the latest push is an improvement.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2051093928)
That is the pattern for functional tests, see for example:

https://github.com/bitcoin/bitcoin/blob/3ee7062ee33c94c717131b8e7a8c01b37a1e9728/test/functional/feature_config_args.py#L488-L503

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2051100542)
Changed the function name from `_run_test_internal` to `_verify_startup_failure` and added comment, hopefully that clarifies this aspect. I also find the error message quite explanatory:
https://github.com/bitcoin/bitcoin/blob/fc88c2ec297dc93ba06008bd5ae10798e9f6aeac/test/functional/feature_framework_startup_failures.py#L59-L60

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2051130798)
Done. Also makes it easier to squash/drop the new final commit of introducing multiple `BitcoinTestFramework`-subclasses.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2051095762)
https://github.com/bitcoin/bitcoin/blob/3ee7062ee33c94c717131b8e7a8c01b37a1e9728/test/functional/test_runner.py#L371-L375

https://github.com/bitcoin/bitcoin/blob/3ee7062ee33c94c717131b8e7a8c01b37a1e9728/test/functional/test_runner.py#L389-L391

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2051097217)
Tried it out, but then one runs into the issue that it looks like `subprocess.TimeoutExpired` could be raised anywhere in that whole function, which I prefer avoiding.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2051118701)
Taken!

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2051108994)
Improved comment:
"Only proceed if num_nodes > 0 (child test processes), as 0 (parent) is not supported by BitcoinTestFramework.setup_network()."

Could explore making `BitcoinTestFramework` more permissive in this aspect but would rather defer that.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2051121901)
I think it leaves flexibility for adding more tests which use a subset of them.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2051126771)
It is rare for functional tests to only test one thing. But I agree "various" is a bit too loose, changed to "framework".

🤔 hodlinator reviewed a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2779505028)
Hi folks!

Asked @l0rinc what it would take for him to give this more than a lightweight A-C-K, and we went through the commits together, especially feature_framework_startup_failures.py. It made me agree some minor improvements are justified in order to make the code more readable & correct.

#### Added final commit - drop or squash?

We also agreed that it would be worth experimenting with breaking up the one test class into multiple to clarify things further. Let me know what you think
...

💬 achow101 commented on pull request "torcontrol: Fix addrOnion outdated comment":
(https://github.com/bitcoin/bitcoin/pull/32282#issuecomment-2816265097)
ACK bcaa23a2b7090c355f7b048ad8cae719c23dea43

🚀 achow101 merged a pull request: "torcontrol: Fix addrOnion outdated comment"
(https://github.com/bitcoin/bitcoin/pull/32282)

💬 achow101 commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#issuecomment-2816284547)
ACK 5cb1241814b9c541c5e5e8daadbbea595f2960ae

✅ achow101 closed an issue: "feefrac_mul_div: Integer-overflow in FeeFrac::Div"
(https://github.com/bitcoin/bitcoin/issues/32294)

🚀 achow101 merged a pull request: "feefrac: avoid integer overflow in temporary"
(https://github.com/bitcoin/bitcoin/pull/32300)

💬 jonatack commented on pull request "[IBD] flush UTXOs in bigger batches based on dbcache size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2051237644)
LGTM, suggestion s/are/is/ and drop one of the two "provided"s

"(default: calculated from the `-dbcache` value, or 16777216 if none is provided)"