💬 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)"

💬 isaackielma commented on issue "[RFC] What security expectations does/should the RPC server have from credentialed RPC clients?":
(https://github.com/bitcoin/bitcoin/issues/32274#issuecomment-2816363732)
What if docs at [doc/JSON-RPC-interface.md](https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md) include an explicit warning stating that:

"RPC credentials grant full administrative control of the node and any resources the `bitcoind` process can access. Clients with valid credentials should be considered as trusted as the local user running `bitcoind`. If untrusted clients must be allowed to access RPC, strong isolation mechanisms (e.g., containers, separate machines, rest
...

📝 ta2ozg opened a pull request: "Update netaddress.cpp"
(https://github.com/bitcoin/bitcoin/pull/32307)
Sets an internal network address based on a user-defined identifier. This approach ensures deterministic internal address generation, while avoiding accidental mismatches due to case or formatting variations.

💬 shahsb commented on pull request "wallet: make coinbase that will mature on the next block available for selection":
(https://github.com/bitcoin/bitcoin/pull/32123#issuecomment-2816518575)
Concept ACK

💬 shahsb commented on pull request "wallet: make coinbase that will mature on the next block available for selection":
(https://github.com/bitcoin/bitcoin/pull/32123#discussion_r2051378269)
when are we unlocking this?